diff --git a/packages/config/src/validate.js b/packages/config/src/validate.js index 09d1dce05..6ce00cf02 100644 --- a/packages/config/src/validate.js +++ b/packages/config/src/validate.js @@ -234,10 +234,17 @@ export function validate(data, key = '/config') { } // fix invalid data + // A clamp silently replaces the caller's value with a valid-looking one, so the number that + // was actually received never reaches the logs. Record it — an out-of-range value usually + // means whatever produced it is broken, and the clamped result hides that. + let received, clampedTo; + if (keyword === 'minimum') { - set(data, path, Math.max(error.data, error.schema)); + [received, clampedTo] = [error.data, Math.max(error.data, error.schema)]; + set(data, path, clampedTo); } else if (keyword === 'maximum') { - set(data, path, Math.min(error.data, error.schema)); + [received, clampedTo] = [error.data, Math.min(error.data, error.schema)]; + set(data, path, clampedTo); } else if (keyword === 'required') { del(data, path.slice(0, -1)); } else if (!params.passingSchemas) { @@ -248,7 +255,9 @@ export function validate(data, key = '/config') { path = joinPropertyPath(path); // map one error per path - errors.set(path, { path, message }); + errors.set(path, clampedTo == null + ? { path, message } + : { path, message, received, clampedTo }); } // filter empty values as a result of scrubbing filterEmpty(data); diff --git a/packages/config/test/index.test.js b/packages/config/test/index.test.js index 7ac606ef5..da1a81790 100644 --- a/packages/config/test/index.test.js +++ b/packages/config/test/index.test.js @@ -78,7 +78,9 @@ describe('PercyConfig', () => { message: 'unknown property' }, { path: 'test.cov', - message: 'must be >= 100' + message: 'must be >= 100', + received: 99, + clampedTo: 100 }]); }); @@ -301,12 +303,18 @@ describe('PercyConfig', () => { let conf = { min: 5, max: 50 }; + // A clamp substitutes a value rather than rejecting it, so the error carries both halves — + // callers report them to explain why the accepted config differs from what was supplied. expect(PercyConfig.validate(conf)).toEqual([{ path: 'min', - message: 'must be >= 10' + message: 'must be >= 10', + received: 5, + clampedTo: 10 }, { path: 'max', - message: 'must be <= 20' + message: 'must be <= 20', + received: 50, + clampedTo: 20 }]); expect(conf).toEqual({ min: 10, max: 20 }); diff --git a/packages/core/src/percy.js b/packages/core/src/percy.js index f8cfa6625..7ec9e7a4f 100644 --- a/packages/core/src/percy.js +++ b/packages/core/src/percy.js @@ -706,19 +706,27 @@ export class Percy { let errors = PercyConfig.validate(comparison, '/comparison'); if (errors?.length > 0) { - this.log.warn('Invalid upload options:'); - for (let e of errors) this.log.warn(`- ${e.path}: ${e.message}`); + // Meta must be set before these warnings, not after: without it the lines carry no + // snapshot name, and a run that warns about several snapshots gives no way to tell which + // one each `tiles[n]` belongs to. + this.log.warn('Invalid upload options:', uploadMeta(options)); + + for (let e of errors) { + // Validation does not reject the upload — it repairs the value and continues, so the + // snapshot is still captured with a substituted one. Say which value was dropped and + // what replaced it, otherwise the only visible symptom is a wrong screenshot: a bad + // tile offset, for instance, leaves the stitched image misaligned with no further clue. + let repaired = e.clampedTo != null + ? ` (received ${e.received}, continuing with ${e.clampedTo})` + : ''; + + this.log.warn(`- ${e.path}: ${e.message}${repaired}`, uploadMeta(options)); + } } } // set meta for logging - options.meta = { - snapshot: { - name: options.name, - testCase: options.testCase, - tag: options.tag?.name - } - }; + options.meta = uploadMeta(options); // add client & environment info this.client.addClientInfo(options.clientInfo); @@ -980,4 +988,16 @@ export class Percy { } } +// Logging meta for a comparison upload. Shared so the validation warnings and the snapshot itself +// are attributed identically — they describe the same upload and have to be greppable together. +function uploadMeta(options) { + return { + snapshot: { + name: options.name, + testCase: options.testCase, + tag: options.tag?.name + } + }; +} + export default Percy; diff --git a/packages/core/test/percy.test.js b/packages/core/test/percy.test.js index 2001b65a1..be622f0e2 100644 --- a/packages/core/test/percy.test.js +++ b/packages/core/test/percy.test.js @@ -1298,6 +1298,51 @@ describe('Percy', () => { ]); }); + it('reports the out-of-range tile offset it replaced', async () => { + await percy.start(); + + // An App Automate fullpage capture returning a negative header height (PER-10200): the + // upload is repaired rather than rejected, so the received value is only ever visible here. + await percy.upload({ + name: 'Snapshot', + tag: { name: 'device' }, + tiles: [ + { sha: 'a'.repeat(64), headerHeight: 0, footerHeight: 879 }, + { sha: 'b'.repeat(64), headerHeight: -318, footerHeight: 0 } + ] + }); + + expect(logger.stderr).toEqual(jasmine.arrayContaining([ + '[percy] - tiles[1].headerHeight: must be >= 0 (received -318, continuing with 0)' + ])); + + // The snapshot is still uploaded — the warning is the only signal that it is misstitched. + expect(api.requests['/snapshots/4567/comparisons']).toHaveSize(1); + expect(logger.stdout).toEqual([ + '[percy] Percy has started!', + '[percy] Snapshot taken: Snapshot' + ]); + }); + + it('attributes upload validation warnings to their snapshot', async () => { + await percy.start(); + + await percy.upload({ + name: 'Snapshot', + tag: { name: 'device' }, + tiles: [{ sha: 'a'.repeat(64), headerHeight: -1 }] + }); + + // Without meta these lines are anonymous, so a run warning about several snapshots gives no + // way to tell which page each `tiles[n]` came from. + let warnings = logger.instance.query(log => ( + log.level === 'warn' && log.message.includes('headerHeight') + )); + + expect(warnings).toHaveSize(1); + expect(warnings[0].meta.snapshot.name).toEqual('Snapshot'); + }); + it('can cancel pending pushed snapshots', async () => { percy = await Percy.start({ token: 'PERCY_TOKEN',