fix(core): report the tile offset a clamped upload replaced, and which snapshot it came from

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);

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 });

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;

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',