Repository: GoogleChrome/lighthouse
Pull request #12697 (2021-06-24 22:43:26)

misc(scripts): improve collision check in collect-strings
ref https://github.com/GoogleChrome/lighthouse/pull/12690#discussion_r658323573

Moves the collision tracking code to after all the individual directories have been processed (instead of doing each independently). Result is all strings are checked for collisions together, instead of just amongst those in the same directory.

Also changed the assertion code to use jest `expect` and compare against an inline array of expected collisions, so we can easily see what changed.

google-cla[bot]: All (the pull request submitter and all commit authors) CLAs are signed, **but** one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project.  Please have them confirm that by leaving a comment that contains only `@googlebot I consent.` in this pull request.

*Note to project maintainer:* There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent.  In those cases, you can manually confirm consent of the commit author(s), and set the `cla` label to `yes` (if enabled on your project).

ℹ️ **Googlers: [Go here](https://goto.google.com/prinfo/https%3A%2F%2Fgithub.com%2FGoogleChrome%2Flighthouse%2Fpull%2F12697) for more info**.

<!-- need_author_consent -->
patrickhulce: @googlebot I consent.

Patch:
diff --git a/.npmignore b/.npmignore
index 6bcf25f46..ef71c7eed 100644
--- a/.npmignore
+++ b/.npmignore
@@ -8,7 +8,9 @@ build/
 coverage/
 dist/
 proto/
+docs/
 
+lighthouse-core/lib/sd-validation/
 lighthouse-core/scripts/*
 lighthouse-core/test/
 lighthouse-core/third_party/src/
@@ -51,6 +53,9 @@ yarn-error.log
 results.html
 *.lcov
 
+# large files
+changelog.md
+
 # dev files
 .DS_Store
 .editorconfig
diff --git a/lighthouse-core/audits/accessibility/aria-progressbar-name.js b/lighthouse-core/audits/accessibility/aria-progressbar-name.js
index e6b482f73..7a0f07d07 100644
--- a/lighthouse-core/audits/accessibility/aria-progressbar-name.js
+++ b/lighthouse-core/audits/accessibility/aria-progressbar-name.js
@@ -19,7 +19,7 @@ const UIStrings = {
   /** Title of an accessibility audit that evaluates if progressbar HTML elements do not have accessible names. This title is descriptive of the failing state and is shown to users when there is a failure that needs to be addressed. */
   failureTitle: 'ARIA `progressbar` elements do not have accessible names.',
   /** Description of a Lighthouse audit that tells the user *why* they should try to pass. This is displayed after a user expands the section to see more. No character length limits. 'Learn More' becomes link text to additional documentation. */
-  description: 'When an element doesn\'t have an accessible name, screen readers announce it with a generic name, making it unusable for users who rely on screen readers. [Learn more](https://web.dev/aria-name/).',
+  description: 'When a `progressbar` element doesn\'t have an accessible name, screen readers announce it with a generic name, making it unusable for users who rely on screen readers. [Learn more](https://web.dev/aria-name/).',
 };
 
 const str_ = i18n.createMessageInstanceIdFn(__filename, UIStrings);
diff --git a/lighthouse-core/lib/i18n/locales/en-US.json b/lighthouse-core/lib/i18n/locales/en-US.json
index 6add2bc73..95b1c960f 100644
--- a/lighthouse-core/lib/i18n/locales/en-US.json
+++ b/lighthouse-core/lib/i18n/locales/en-US.json
@@ -63,7 +63,7 @@
     "message": "ARIA `meter` elements have accessible names"
   },
   "lighthouse-core/audits/accessibility/aria-progressbar-name.js | description": {
-    "message": "When an element doesn't have an accessible name, screen readers announce it with a generic name, making it unusable for users who rely on screen readers. [Learn more](https://web.dev/aria-name/)."
+    "message": "When a `progressbar` element doesn't have an accessible name, screen readers announce it with a generic name, making it unusable for users who rely on screen readers. [Learn more](https://web.dev/aria-name/)."
   },
   "lighthouse-core/audits/accessibility/aria-progressbar-name.js | failureTitle": {
     "message": "ARIA `progressbar` elements do not have accessible names."
diff --git a/lighthouse-core/lib/i18n/locales/en-XL.json b/lighthouse-core/lib/i18n/locales/en-XL.json
index fce4eeae8..2c301c4be 100644
--- a/lighthouse-core/lib/i18n/locales/en-XL.json
+++ b/lighthouse-core/lib/i18n/locales/en-XL.json
@@ -63,7 +63,7 @@
     "message": "ÂŔÎÁ `meter` êĺêḿêńt̂ś ĥáv̂é âćĉéŝśîb́l̂é n̂ám̂éŝ"
   },
   "lighthouse-core/audits/accessibility/aria-progressbar-name.js | description": {
-    "message": "Ŵh́êń âń êĺêḿêńt̂ d́ôéŝń't̂ h́âv́ê án̂ áĉćêśŝíb̂ĺê ńâḿê, śĉŕêén̂ ŕêád̂ér̂ś âńn̂óûńĉé ît́ ŵít̂h́ â ǵêńêŕîć n̂ám̂é, m̂ák̂ín̂ǵ ît́ ûńûśâb́l̂é f̂ór̂ úŝér̂ś ŵh́ô ŕêĺŷ ón̂ śĉŕêén̂ ŕêád̂ér̂ś. [L̂éâŕn̂ ḿôŕê](https://web.dev/aria-name/)."
+    "message": "Ŵh́êń â `progressbar` él̂ém̂én̂t́ d̂óêśn̂'t́ ĥáv̂é âń âćĉéŝśîb́l̂é n̂ám̂é, ŝćr̂éêń r̂éâd́êŕŝ án̂ńôún̂ćê ít̂ ẃît́ĥ á ĝén̂ér̂íĉ ńâḿê, ḿâḱîńĝ ít̂ ún̂úŝáb̂ĺê f́ôŕ ûśêŕŝ ẃĥó r̂él̂ý ôń ŝćr̂éêń r̂éâd́êŕŝ. [Ĺêár̂ń m̂ór̂é](https://web.dev/aria-name/)."
   },
   "lighthouse-core/audits/accessibility/aria-progressbar-name.js | failureTitle": {
     "message": "ÂŔÎÁ `progressbar` êĺêḿêńt̂ś d̂ó n̂ót̂ h́âv́ê áĉćêśŝíb̂ĺê ńâḿêś."
diff --git a/lighthouse-core/scripts/i18n/collect-strings.js b/lighthouse-core/scripts/i18n/collect-strings.js
index fdafa9c82..06178ab85 100644
--- a/lighthouse-core/scripts/i18n/collect-strings.js
+++ b/lighthouse-core/scripts/i18n/collect-strings.js
@@ -11,7 +11,7 @@
 const fs = require('fs');
 const glob = require('glob');
 const path = require('path');
-const assert = require('assert').strict;
+const expect = require('expect');
 const tsc = require('typescript');
 const MessageParser = require('intl-messageformat-parser').default;
 const Util = require('../../../report/renderer/util.js');
@@ -517,15 +517,6 @@ function parseUIStrings(sourceStr, liveUIStrings) {
   return parsedMessages;
 }
 
-/** @type {Map<string, string>} */
-const seenStrings = new Map();
-
-/** @type {number} */
-let collisions = 0;
-
-/** @type {Array<string>} */
-const collisionStrings = [];
-
 /**
  * Collects all LHL messsages defined in UIString from Javascript files in dir,
  * and converts them into CTC.
@@ -584,28 +575,6 @@ function collectAllStringsInDir(dir) {
 
       const messageKey = `${relativeToRootPath} | ${key}`;
       strings[messageKey] = ctc;
-
-      // check for duplicates, if duplicate, add @description as @meaning to both
-      if (seenStrings.has(ctc.message)) {
-        ctc.meaning = ctc.description;
-        const seenId = seenStrings.get(ctc.message);
-        // TODO: `strings[seenId]` check shouldn't be necessary here ...
-        // see https://github.com/GoogleChrome/lighthouse/pull/12441/files#r630521367
-        if (seenId && strings[seenId]) {
-          if (!strings[seenId].meaning) {
-            strings[seenId].meaning = strings[seenId].description;
-            collisions++;
-          }
-
-          if (ctc.meaning === strings[seenId].meaning) {
-            throw new Error(`'${messageKey}' is an exact duplicate of '${seenId}' when placeholders are removed. Each strings' \`message\` or \`description\` must be different for the translation pipeline`);
-          }
-
-          collisionStrings.push(ctc.message);
-          collisions++;
-        }
-      }
-      seenStrings.set(ctc.message, messageKey);
     }
   }
 
@@ -628,6 +597,98 @@ function writeStringsToCtcFiles(locale, strings) {
   fs.writeFileSync(fullPath, JSON.stringify(output, null, 2) + '\n');
 }
 
+/**
+ * This function does three things:
+ *
+ *    - Add `meaning` property to ctc messages that have the same message so TC can disambiguate (otherwise it fails to import).
+ *    - Throw if the `meaning` of any collisions *also* collides (can't disambiguate messages).
+ *    - Throw if the known collisions has changed at all.
+ *
+ * @param {Record<string, CtcMessage>} strings
+ */
+function resolveMessageCollisions(strings) {
+  /** @type {Map<string, Array<[string, CtcMessage]>>} */
+  const stringsByMessage = new Map();
+
+  // Group all the strings by their message.
+  for (const entry of Object.entries(strings)) {
+    const collisions = stringsByMessage.get(entry[1].message) || [];
+    collisions.push(entry);
+    stringsByMessage.set(entry[1].message, collisions);
+  }
+
+  /** @type {Array<[string, CtcMessage]>} */
+  const allCollisions = [];
+  for (const group of stringsByMessage.values()) {
+    // If this message didn't collide with anything else, skip it.
+    if (group.length <= 1) continue;
+    allCollisions.push(...group);
+
+    // We have a message collision, time to check collisions on the `meaning` property.
+    /** @type {Map<string|undefined, Array<[string, CtcMessage]>>} */
+    const stringsByMeaning = new Map();
+    for (const [key, ctc] of group) {
+      ctc.meaning = ctc.description;
+
+      const collisions = stringsByMeaning.get(ctc.meaning) || [];
+      collisions.push([key, ctc]);
+      stringsByMeaning.set(ctc.meaning, collisions);
+    }
+
+    for (const meaningGroup of stringsByMeaning.values()) {
+      if (meaningGroup.length <= 1) continue;
+
+      const debugMeaningList = meaningGroup.map(entry => [entry[0], entry[1].meaning].join('\n'));
+      const debugCollisionsMessage = `${meaningGroup[0][1].message}\n\n${debugMeaningList.join('\n\n')}`;
+      throw new Error(`Each strings' \`message\` or \`description\` must be different for the translation pipeline. The following keys did not have unique \`meaning\` values:\n\n${debugCollisionsMessage}`);
+    }
+  }
+
+  // We survived fatal collisions, now check that the known collisions match our known list.
+  const collidingMessages = allCollisions.map(collision => collision[1].message).sort();
+
+  try {
+    expect(collidingMessages).toEqual([
+      '$MARKDOWN_SNIPPET_0$ elements do not have $MARKDOWN_SNIPPET_1$ text',
+      '$MARKDOWN_SNIPPET_0$ elements do not have $MARKDOWN_SNIPPET_1$ text',
+      '$MARKDOWN_SNIPPET_0$ elements have $MARKDOWN_SNIPPET_1$ text',
+      '$MARKDOWN_SNIPPET_0$ elements have $MARKDOWN_SNIPPET_1$ text',
+      'ARIA $MARKDOWN_SNIPPET_0$ elements do not have accessible names.',
+      'ARIA $MARKDOWN_SNIPPET_0$ elements do not have accessible names.',
+      'ARIA $MARKDOWN_SNIPPET_0$ elements do not have accessible names.',
+      'ARIA $MARKDOWN_SNIPPET_0$ elements do not have accessible names.',
+      'ARIA $MARKDOWN_SNIPPET_0$ elements have accessible names',
+      'ARIA $MARKDOWN_SNIPPET_0$ elements have accessible names',
+      'ARIA $MARKDOWN_SNIPPET_0$ elements have accessible names',
+      'ARIA $MARKDOWN_SNIPPET_0$ elements have accessible names',
+      'Consider uploading your GIF to a service which will make it available to embed as an HTML5 video.',
+      'Consider uploading your GIF to a service which will make it available to embed as an HTML5 video.',
+      'Consider uploading your GIF to a service which will make it available to embed as an HTML5 video.',
+      'Consider using a $LINK_START_0$plugin$LINK_END_0$ or service that will automatically convert your uploaded images to the optimal formats.',
+      'Consider using a $LINK_START_0$plugin$LINK_END_0$ or service that will automatically convert your uploaded images to the optimal formats.',
+      'Document has a valid $MARKDOWN_SNIPPET_0$',
+      'Document has a valid $MARKDOWN_SNIPPET_0$',
+      'Failing Elements',
+      'Failing Elements',
+      'Name',
+      'Name',
+      'Potential Savings',
+      'Potential Savings',
+      'URL',
+      'URL',
+      'When an element doesn\'t have an accessible name, screen readers announce it with a generic name, making it unusable for users who rely on screen readers. $LINK_START_0$Learn more$LINK_END_0$.',
+      'When an element doesn\'t have an accessible name, screen readers announce it with a generic name, making it unusable for users who rely on screen readers. $LINK_START_0$Learn more$LINK_END_0$.',
+      'When an element doesn\'t have an accessible name, screen readers announce it with a generic name, making it unusable for users who rely on screen readers. $LINK_START_0$Learn more$LINK_END_0$.',
+      'When an element doesn\'t have an accessible name, screen readers announce it with a generic name, making it unusable for users who rely on screen readers. $LINK_START_0$Learn more$LINK_END_0$.',
+    ]);
+  } catch (err) {
+    console.log('The number of duplicate strings have changed, update this assertion if that is expected, or reword strings');
+    console.log('copy/paste this to pass check:');
+    console.log(collidingMessages);
+    throw new Error(err.message);
+  }
+}
+
 // Test if called from the CLI or as a module.
 if (require.main === module) {
   /** @type {Record<string, CtcMessage>} */
@@ -639,10 +700,7 @@ if (require.main === module) {
     Object.assign(strings, moreStrings);
   }
 
-  if (collisions > 0) {
-    console.log(`MEANING COLLISION: ${collisions} string(s) have the same content.`);
-    assert.equal(collisions, 28, `The number of duplicate strings have changed, update this assertion if that is expected, or reword strings. Collisions: ${collisionStrings.join('\n')}`);
-  }
+  resolveMessageCollisions(strings);
 
   writeStringsToCtcFiles('en-US', strings);
   console.log('Written to disk!', 'en-US.ctc.json');


Test patch:
diff --git a/lighthouse-core/test/config/config-helpers-test.js b/lighthouse-core/test/config/config-helpers-test.js
index 7400e3582..8713b36ad 100644
--- a/lighthouse-core/test/config/config-helpers-test.js
+++ b/lighthouse-core/test/config/config-helpers-test.js
@@ -21,6 +21,7 @@ const Runner = require('../../runner.js');
 const Gatherer = require('../../gather/gatherers/gatherer.js');
 const ImageElementsGatherer = require('../../gather/gatherers/image-elements.js');
 const UserTimingsAudit = require('../../audits/user-timings.js');
+const {isNode12SmallIcu} = require('../test-utils.js');
 
 jest.mock('process', () => ({
   cwd: () => jest.fn(),
@@ -150,7 +151,7 @@ describe('.resolveSettings', () => {
   it('resolves the locale', () => {
     const settings = resolveSettings({locale: 'zh-CN'});
     // COMPAT: Node 12 only has 'en' by default.
-    if (process.versions.node.startsWith('12')) {
+    if (isNode12SmallIcu()) {
       expect(settings.locale).toEqual('en');
       return;
     }
diff --git a/lighthouse-core/test/config/config-test.js b/lighthouse-core/test/config/config-test.js
index e2b43d16e..0a05be625 100644
--- a/lighthouse-core/test/config/config-test.js
+++ b/lighthouse-core/test/config/config-test.js
@@ -14,6 +14,7 @@ const log = require('lighthouse-logger');
 const Gatherer = require('../../gather/gatherers/gatherer.js');
 const Audit = require('../../audits/audit.js');
 const i18n = require('../../lib/i18n/i18n.js');
+const {isNode12SmallIcu} = require('../test-utils.js');
 
 /* eslint-env jest */
 
@@ -829,7 +830,7 @@ describe('Config', () => {
       const locale = 'ar-XB';
       const config = new Config({settings: {locale}});
       // COMPAT: Node 12 only has 'en' by default.
-      if (process.versions.node.startsWith('12')) return;
+      if (isNode12SmallIcu()) return;
       assert.strictEqual(config.settings.locale, locale);
     });
 
@@ -838,7 +839,7 @@ describe('Config', () => {
       const flagsLocale = 'ar-XB';
       const config = new Config({settings: {locale: settingsLocale}}, {locale: flagsLocale});
       // COMPAT: Node 12 only has 'en' by default.
-      if (process.versions.node.startsWith('12')) return;
+      if (isNode12SmallIcu()) return;
       assert.strictEqual(config.settings.locale, flagsLocale);
     });
   });
diff --git a/lighthouse-core/test/lib/i18n/i18n-test.js b/lighthouse-core/test/lib/i18n/i18n-test.js
index cbb17f8a5..9a2f481cc 100644
--- a/lighthouse-core/test/lib/i18n/i18n-test.js
+++ b/lighthouse-core/test/lib/i18n/i18n-test.js
@@ -8,6 +8,7 @@
 const path = require('path');
 const i18n = require('../../../lib/i18n/i18n.js');
 const log = require('lighthouse-logger');
+const {isNode12SmallIcu} = require('../../test-utils.js');
 
 /* eslint-env jest */
 
@@ -158,7 +159,7 @@ describe('i18n', () => {
       expect(i18n.lookupLocale(invalidLocale)).toEqual('en');
 
       // COMPAT: Node 12 logs an extra warning that full-icu is not available.
-      if (process.versions.node.startsWith('12')) {
+      if (isNode12SmallIcu()) {
         expect(logListener).toBeCalledTimes(2);
         expect(logListener).toHaveBeenNthCalledWith(1, ['i18n',
           expect.stringMatching(/Requested locale not available in this version of node/)]);
@@ -176,7 +177,7 @@ describe('i18n', () => {
 
     it('falls back to root tag prefix if specific locale not available', () => {
       // COMPAT: Node 12 only has 'en' by default.
-      if (process.versions.node.startsWith('12')) {
+      if (isNode12SmallIcu()) {
         expect(i18n.lookupLocale('es-JKJK')).toEqual('en');
         return;
       }
diff --git a/lighthouse-core/test/lib/i18n/swap-locale-test.js b/lighthouse-core/test/lib/i18n/swap-locale-test.js
index 3c3b87dcf..170c45ad1 100644
--- a/lighthouse-core/test/lib/i18n/swap-locale-test.js
+++ b/lighthouse-core/test/lib/i18n/swap-locale-test.js
@@ -6,13 +6,14 @@
 'use strict';
 
 const swapLocale = require('../../../lib/i18n/swap-locale.js');
+const {isNode12SmallIcu} = require('../../test-utils.js');
 
 const lhr = require('../../results/sample_v2.json');
 
 /* eslint-env jest */
 describe('swap-locale', () => {
   // COMPAT: Node 12 only has 'en' by default. Skip these tests since they're all about swapping locales.
-  if (process.versions.node.startsWith('12')) {
+  if (isNode12SmallIcu()) {
     // Jest requires at least one test per suite.
     it('runs even if other locales are not supported', () => {
       /** @type {LH.Result} */
diff --git a/lighthouse-core/test/results/sample_v2.json b/lighthouse-core/test/results/sample_v2.json
index 0664ed9b7..d22c433a1 100644
--- a/lighthouse-core/test/results/sample_v2.json
+++ b/lighthouse-core/test/results/sample_v2.json
@@ -2649,7 +2649,7 @@
     "aria-progressbar-name": {
       "id": "aria-progressbar-name",
       "title": "ARIA `progressbar` elements have accessible names",
-      "description": "When an element doesn't have an accessible name, screen readers announce it with a generic name, making it unusable for users who rely on screen readers. [Learn more](https://web.dev/aria-name/).",
+      "description": "When a `progressbar` element doesn't have an accessible name, screen readers announce it with a generic name, making it unusable for users who rely on screen readers. [Learn more](https://web.dev/aria-name/).",
       "score": null,
       "scoreDisplayMode": "notApplicable"
     },
diff --git a/lighthouse-core/test/test-utils.js b/lighthouse-core/test/test-utils.js
index 82c0ab108..7246eeaef 100644
--- a/lighthouse-core/test/test-utils.js
+++ b/lighthouse-core/test/test-utils.js
@@ -271,6 +271,18 @@ function makeMocksForGatherRunner() {
   }));
 }
 
+/**
+ * Returns whether this is running in Node 12 with what we suspect is the default
+ * `small-icu` build. Limited to Node 12 so it's not accidentally hitting this
+ * path in Node 13+ in CI and we can be certain the `full-icu` path is being exercised.
+ * @return {boolean}
+ */
+function isNode12SmallIcu() {
+  // COMPAT: Remove when Node 12 is retired and `full-icu` is the default everywhere.
+  return process.versions.node.startsWith('12') &&
+    Intl.NumberFormat.supportedLocalesOf('es').length === 0;
+}
+
 module.exports = {
   getProtoRoundTrip,
   loadSourceMapFixture,
@@ -280,5 +292,6 @@ module.exports = {
   createDecomposedPromise,
   flushAllTimersAndMicrotasks,
   makeMocksForGatherRunner,
+  isNode12SmallIcu,
   ...mockCommands,
 };
diff --git a/report/test/renderer/i18n-test.js b/report/test/renderer/i18n-test.js
index 64a30e7e2..49c973759 100644
--- a/report/test/renderer/i18n-test.js
+++ b/report/test/renderer/i18n-test.js
@@ -8,6 +8,7 @@
 const assert = require('assert').strict;
 const Util = require('../../renderer/util.js');
 const I18n = require('../../renderer/i18n.js');
+const {isNode12SmallIcu} = require('../../../lighthouse-core/test/test-utils.js');
 
 // Require i18n to make sure Intl is polyfilled in Node without full-icu for testing.
 // When Util is run in a browser, Intl will be supplied natively (IE11+).
@@ -107,7 +108,7 @@ describe('util helpers', () => {
 
   it('formats numbers based on locale', () => {
     // COMPAT: Node 12 only has 'en' by default.
-    if (process.versions.node.startsWith('12')) return;
+    if (isNode12SmallIcu()) return;
 
     // Requires full-icu or Intl polyfill.
     const number = 12346.858558;
@@ -121,7 +122,7 @@ describe('util helpers', () => {
 
   it('uses decimal comma with en-XA test locale', () => {
     // COMPAT: Node 12 only has 'en' by default.
-    if (process.versions.node.startsWith('12')) return;
+    if (isNode12SmallIcu()) return;
 
     // Requires full-icu or Intl polyfill.
     const number = 12346.858558;