Require --on-diff=update even for newly added snapshots

This was done to auto-accept snapshots for new tests (like jest does),
but that resulted in new tests accidentally merging with no snapshot
entry.

Bug: 434252192
Change-Id: I7c8538af64c4ee8b97cd0389a368479180af9789
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/6916862
Reviewed-by: Paul Irish <paulirish@chromium.org>
Commit-Queue: Connor Clark <cjamcl@chromium.org>
Auto-Submit: Connor Clark <cjamcl@chromium.org>

Author: Connor Clark

front_end/models/ai_assistance/data_formatters/PerformanceInsightFormatter.snapshot.txt

@@ -843,7 +843,7 @@ no origins were preconnected.
 - https://developer.chrome.com/docs/lighthouse/performance/uses-rel-preconnect/
 === end content
 
-Title: PerformanceInsightFormatter Third Parties Insight serializes correctly when there are no results
+Title: PerformanceInsightFormatter ThirdParties serializes correctly when there are no results
 Content:
 ## Insight Title: 3rd parties
 
@@ -859,7 +859,7 @@ No 3rd party scripts were found on this page.
 - https://web.dev/articles/optimizing-content-efficiency-loading-third-party-javascript/
 === end content
 
-Title: PerformanceInsightFormatter Third Parties Insight serializes 3rd party scripts correctly
+Title: PerformanceInsightFormatter ThirdParties serializes 3rd party scripts correctly
 Content:
 ## Insight Title: 3rd parties
 

front_end/testing/README.md

@@ -92,7 +92,7 @@ The above test will fail if any DOM mutations are detected.
 
 For Karma unit tests, you can add regression tests for string output (similar to Jest snapshots) via SnapshotTester.
 
-> Note: Prefer unit tests that test behavior more directly when possible. Some good candidates for snapshot tests is code the formats long strings. A bad candidate would be a `JSON.stringify` of a complex object structure (prefer explicit asserts instead).
+> Note: Prefer unit tests that test behavior more directly when possible. Some good candidates for snapshot tests is code that formats long strings. A bad candidate would be a `JSON.stringify` of a complex object structure (prefer explicit asserts instead).
 
 Results for all snapshots for a test file `MyFile.test.ts` are written to `MyFile.snapshot.txt`.
 

front_end/testing/SnapshotTester.ts

@@ -74,10 +74,15 @@ export class SnapshotTester {
     this.#actual.set(title, actual);
 
     const expected = this.#expected.get(title);
-    if (!expected) {
-      // New tests always pass on the first run.
+    if (expected === undefined) {
       this.#newTests = true;
-      return;
+      if (SnapshotTester.#updateMode) {
+        return;
+      }
+
+      this.#anyFailures = true;
+      throw new Error(`snapshot assertion failed! new snapshot found (${
+          title}), must run \`npm run test -- --on-diff=update ...\` to accept it.`);
     }
 
     if (!SnapshotTester.#updateMode && actual !== expected) {
@@ -86,6 +91,16 @@ export class SnapshotTester {
     }
   }
 
+  async #postUpdate(): Promise<void> {
+    const url = new URL('/update-snapshot', import.meta.url);
+    url.searchParams.set('snapshotUrl', this.#snapshotUrl);
+    const content = this.#serializeSnapshotFileContent();
+    const response = await fetch(url, {method: 'POST', body: content});
+    if (response.status !== 200) {
+      throw new Error(`Unable to update snapshot ${url}`);
+    }
+  }
+
   async finish() {
     let didAnyTestNotRun = false;
     for (const title of this.#expected.keys()) {
@@ -95,21 +110,22 @@ export class SnapshotTester {
       }
     }
 
-    let shouldPostUpdate = SnapshotTester.#updateMode;
-    if (!this.#anyFailures && (didAnyTestNotRun || this.#newTests)) {
-      shouldPostUpdate = true;
+    const hasChanges = this.#anyFailures || didAnyTestNotRun || this.#newTests;
+    if (!hasChanges) {
+      return;
     }
 
-    if (!shouldPostUpdate) {
+    // If the update flag is on, post any and all changes (failures, new tests, removals).
+    if (SnapshotTester.#updateMode) {
+      await this.#postUpdate();
       return;
     }
 
-    const url = new URL('/update-snapshot', import.meta.url);
-    url.searchParams.set('snapshotUrl', this.#snapshotUrl);
-    const content = this.#serializeSnapshotFileContent();
-    const response = await fetch(url, {method: 'POST', body: content});
-    if (response.status !== 200) {
-      throw new Error(`Unable to update snapshot ${url}`);
+    // Note: this does not handle test filtering (.only, --grep). Need a reliable way
+    // to distinguish a deleted test from a test that was filtered out.
+    if (didAnyTestNotRun) {
+      throw new Error(
+          'Snapshots are out of sync (a test was likely deleted or renamed). Run with `--on-diff=update` to fix.');
     }
   }
 

test/unit/karma.conf.ts

@@ -279,7 +279,7 @@ function snapshotTesterFactory() {
       res.writeHead(200, {'Content-Type': 'application/json'});
       const updateMode = TestConfig.onDiff.update === true;
       res.end(JSON.stringify({updateMode}));
-      return res.end();
+      return;
     }
 
     if (req.url.startsWith('/update-snapshot')) {