Repository: GoogleChrome/lighthouse
Pull request #5884 (2018-08-21 01:26:15)

core: fix defaultConfig UIStrings and exit code for test
fix `module.exports` ordering issue introduced by #5858

Also makes the `i18n:checks` test fail if `collect-strings.js` throws an error


Patch:
diff --git a/.cz-config.js b/.cz-config.js
index eb2ea425c..61108b3bb 100644
--- a/.cz-config.js
+++ b/.cz-config.js
@@ -13,6 +13,7 @@ module.exports = {
     {value: 'new_audit',  name: 'new_audit: A new audit'},
     {value: 'core',       name: 'core:      Driver, gather, (non-new) audits, LHR JSON, etc'},
     {value: 'tests',      name: 'tests:     Tests, smokehouse, etc'},
+    {value: 'i18n',       name: 'i18n:      Internationalization'},
     {value: 'docs',       name: 'docs:      Documentation'},
     {value: 'deps',       name: 'deps:      Dependency bumps only'},
     {value: 'report',     name: 'report:    Report, UI, renderers'},
diff --git a/lighthouse-cli/bin.js b/lighthouse-cli/bin.js
index d02844d89..4baa04b15 100644
--- a/lighthouse-cli/bin.js
+++ b/lighthouse-cli/bin.js
@@ -107,6 +107,7 @@ function run() {
       }
     })
     .then(_ => {
+      // @ts-ignore TODO(bckenny): Sentry type checking
       Sentry.init({
         url,
         flags: cliFlags,
diff --git a/lighthouse-core/audits/dobetterweb/dom-size.js b/lighthouse-core/audits/dobetterweb/dom-size.js
index de0872d36..38cb9e6e0 100644
--- a/lighthouse-core/audits/dobetterweb/dom-size.js
+++ b/lighthouse-core/audits/dobetterweb/dom-size.js
@@ -101,6 +101,7 @@ class DOMSize extends Audit {
       {key: 'width', itemType: 'text', text: str_(UIStrings.columnDOMWidth)},
     ];
 
+    /** @type {Array<Object<string, LH.Audit.DetailsItem>>} */
     const items = [
       {
         totalNodes: Util.formatNumber(stats.totalDOMNodes),
diff --git a/lighthouse-core/audits/user-timings.js b/lighthouse-core/audits/user-timings.js
index 6429bfd7c..d3015f13d 100644
--- a/lighthouse-core/audits/user-timings.js
+++ b/lighthouse-core/audits/user-timings.js
@@ -56,6 +56,7 @@ class UserTimings extends Audit {
   static filterTrace(tabTrace) {
     /** @type {Array<MarkEvent|MeasureEvent>} */
     const userTimings = [];
+    /** @type {Record<string, number>} */
     const measuresStartTimes = {};
 
     // Get all blink.user_timing events
diff --git a/lighthouse-core/config/default-config.js b/lighthouse-core/config/default-config.js
index 251d62cd0..00e74f212 100644
--- a/lighthouse-core/config/default-config.js
+++ b/lighthouse-core/config/default-config.js
@@ -441,10 +441,10 @@ const defaultConfig = {
   },
 };
 
+module.exports = defaultConfig;
+
 // Use `defineProperty` so that the strings are accesible from original but ignored when we copy it
 Object.defineProperty(module.exports, 'UIStrings', {
   enumerable: false,
   get: () => UIStrings,
 });
-
-module.exports = defaultConfig;
diff --git a/lighthouse-core/gather/computed/metrics/lantern-metric.js b/lighthouse-core/gather/computed/metrics/lantern-metric.js
index 43a15c2b0..425ce6d06 100644
--- a/lighthouse-core/gather/computed/metrics/lantern-metric.js
+++ b/lighthouse-core/gather/computed/metrics/lantern-metric.js
@@ -85,6 +85,7 @@ class LanternMetricArtifact extends ComputedArtifact {
     const optimisticGraph = this.getOptimisticGraph(graph, traceOfTab);
     const pessimisticGraph = this.getPessimisticGraph(graph, traceOfTab);
 
+    /** @type {{flexibleOrdering?: boolean, label?: string}} */
     let simulateOptions = {label: `optimistic${metricName}`};
     const optimisticSimulation = simulator.simulate(optimisticGraph, simulateOptions);
 
diff --git a/lighthouse-core/gather/computed/metrics/metric.js b/lighthouse-core/gather/computed/metrics/metric.js
index 2c73c644e..13e616112 100644
--- a/lighthouse-core/gather/computed/metrics/metric.js
+++ b/lighthouse-core/gather/computed/metrics/metric.js
@@ -6,6 +6,7 @@
 'use strict';
 
 const ComputedArtifact = require('../computed-artifact');
+const TracingProcessor = require('../../../lib/traces/tracing-processor');
 
 /**
  * @fileOverview Encapsulates logic for choosing the correct metric computation method based on the
@@ -58,6 +59,8 @@ class ComputedMetric extends ComputedArtifact {
       traceOfTab: await computedArtifacts.requestTraceOfTab(trace),
     }, data);
 
+    TracingProcessor.assertHasToplevelEvents(augmentedData.traceOfTab.mainThreadEvents);
+
     switch (settings.throttlingMethod) {
       case 'simulate':
         return this.computeSimulatedMetric(augmentedData, computedArtifacts);
diff --git a/lighthouse-core/gather/computed/page-dependency-graph.js b/lighthouse-core/gather/computed/page-dependency-graph.js
index 0f991be11..95be2a63b 100644
--- a/lighthouse-core/gather/computed/page-dependency-graph.js
+++ b/lighthouse-core/gather/computed/page-dependency-graph.js
@@ -83,6 +83,8 @@ class PageDependencyGraphArtifact extends ComputedArtifact {
     const nodes = [];
     let i = 0;
 
+    TracingProcessor.assertHasToplevelEvents(traceOfTab.mainThreadEvents);
+
     const minimumEvtDur = MINIMUM_TASK_DURATION_OF_INTEREST * 1000;
     while (i < traceOfTab.mainThreadEvents.length) {
       const evt = traceOfTab.mainThreadEvents[i];
diff --git a/lighthouse-core/lib/dependency-graph/simulator/network-analyzer.js b/lighthouse-core/lib/dependency-graph/simulator/network-analyzer.js
index 2eaac3dd7..c2ad615fa 100644
--- a/lighthouse-core/lib/dependency-graph/simulator/network-analyzer.js
+++ b/lighthouse-core/lib/dependency-graph/simulator/network-analyzer.js
@@ -11,8 +11,11 @@ const NetworkRequest = require('../../network-request');
 // Assume that 40% of TTFB was server response time by default for static assets
 const DEFAULT_SERVER_RESPONSE_PERCENTAGE = 0.4;
 
-// For certain resource types, server response time takes up a greater percentage of TTFB (dynamic
-// assets like HTML documents, XHR/API calls, etc)
+/**
+ * For certain resource types, server response time takes up a greater percentage of TTFB (dynamic
+ * assets like HTML documents, XHR/API calls, etc)
+ * @type {Partial<Record<LH.Crdp.Page.ResourceType, number>>}
+ */
 const SERVER_RESPONSE_PERCENTAGE_OF_TTFB = {
   Document: 0.9,
   XHR: 0.9,
@@ -73,9 +76,11 @@ class NetworkAnalyzer {
     return summaryByKey;
   }
 
+  /** @typedef {{record: LH.Artifacts.NetworkRequest, timing: LH.Crdp.Network.ResourceTiming, connectionReused?: boolean}} RequestInfo */
+
   /**
    * @param {LH.Artifacts.NetworkRequest[]} records
-   * @param {function(any):any} iteratee
+   * @param {(e: RequestInfo) => number | number[] | undefined} iteratee
    * @return {Map<string, number[]>}
    */
   static _estimateValueByOrigin(records, iteratee) {
@@ -190,6 +195,7 @@ class NetworkAnalyzer {
   static _estimateRTTByOriginViaHeadersEndTiming(records) {
     return NetworkAnalyzer._estimateValueByOrigin(records, ({record, timing, connectionReused}) => {
       if (!Number.isFinite(timing.receiveHeadersEnd) || timing.receiveHeadersEnd < 0) return;
+      if (!record.resourceType) return;
 
       const serverResponseTimePercentage = SERVER_RESPONSE_PERCENTAGE_OF_TTFB[record.resourceType]
         || DEFAULT_SERVER_RESPONSE_PERCENTAGE;
diff --git a/lighthouse-core/lib/i18n.js b/lighthouse-core/lib/i18n.js
index 05512a2f3..274137464 100644
--- a/lighthouse-core/lib/i18n.js
+++ b/lighthouse-core/lib/i18n.js
@@ -175,6 +175,7 @@ function _formatPathAsString(pathInLHR) {
  */
 function getRendererFormattedStrings(locale) {
   const icuMessageIds = Object.keys(LOCALES[locale]).filter(f => f.includes('core/report/html/'));
+  /** @type {LH.I18NRendererStrings} */
   const strings = {};
   for (const icuMessageId of icuMessageIds) {
     const [filename, varName] = icuMessageId.split(' | ');
@@ -190,6 +191,7 @@ function getRendererFormattedStrings(locale) {
  * @param {Record<string, string>} fileStrings
  */
 function createMessageInstanceIdFn(filename, fileStrings) {
+  /** @type {Record<string, string>} */
   const mergedStrings = {...UIStrings, ...fileStrings};
 
   /** @param {string} icuMessage @param {*} [values] */
@@ -284,6 +286,7 @@ function replaceIcuMessageInstanceIds(lhr, locale) {
     }
   }
 
+  /** @type {LH.I18NMessages} */
   const icuMessagePaths = {};
   replaceInObject(lhr, icuMessagePaths);
   return icuMessagePaths;
diff --git a/lighthouse-core/lib/locales/index.js b/lighthouse-core/lib/locales/index.js
index aa4baed52..26c4cc7ce 100644
--- a/lighthouse-core/lib/locales/index.js
+++ b/lighthouse-core/lib/locales/index.js
@@ -5,6 +5,13 @@
  */
 'use strict';
 
+/** @fileoverview
+ * Define message file to be used for a given locale. A few aliases are defined below.
+ *
+ * Google locale inheritance rules: https://goto.google.com/ccssm
+ * CLDR language aliases: https://www.unicode.org/cldr/charts/latest/supplemental/aliases.html
+ */
+
 /** @typedef {Record<string, {message: string}>} LocaleMessages */
 
 // The keys within this const must exactly match the LH.Locale type in externs.d.ts
@@ -48,6 +55,7 @@ const locales = {
   'iw': require('./he.json'), // Alias of 'he'
   'ja': require('./ja.json'),
   'ko': require('./ko.json'),
+  'ln': require('./fr.json'), // Alias of 'fr'
   'lt': require('./lt.json'),
   'lv': require('./lv.json'),
   'mo': require('./ro.json'), // Alias of 'ro'
diff --git a/lighthouse-core/lib/traces/tracing-processor.js b/lighthouse-core/lib/traces/tracing-processor.js
index 6df2f3d27..05ff24d57 100644
--- a/lighthouse-core/lib/traces/tracing-processor.js
+++ b/lighthouse-core/lib/traces/tracing-processor.js
@@ -17,6 +17,20 @@ const SCHEDULABLE_TASK_TITLE_ALT2 = 'ThreadControllerImpl::RunTask';
 const LHError = require('../errors');
 
 class TraceProcessor {
+  /**
+   * There should *always* be at least one top level event, having 0 typically means something is
+   * drastically wrong with the trace and we should just give up early and loudly.
+   *
+   * @param {LH.TraceEvent[]} events
+   */
+  static assertHasToplevelEvents(events) {
+    const hasToplevelTask = events.some(TraceProcessor.isScheduleableTask);
+    if (!hasToplevelTask) {
+      throw new Error('Could not find any top level events');
+    }
+  }
+
+
   /**
    * Calculate duration at specified percentiles for given population of
    * durations.
@@ -175,12 +189,6 @@ class TraceProcessor {
       });
     }
 
-    // There should *always* be at least one top level event, having 0 typically means something is
-    // drastically wrong with the trace and would should just give up early and loudly.
-    if (!topLevelEvents.length) {
-      throw new Error('Could not find any top level events');
-    }
-
     return topLevelEvents;
   }
 
diff --git a/lighthouse-core/report/html/renderer/report-ui-features.js b/lighthouse-core/report/html/renderer/report-ui-features.js
index c3ea5bcb0..97d590c59 100644
--- a/lighthouse-core/report/html/renderer/report-ui-features.js
+++ b/lighthouse-core/report/html/renderer/report-ui-features.js
@@ -90,7 +90,7 @@ class ReportUIFeatures {
     this._setupHeaderAnimation();
     this._resetUIState();
     this._document.addEventListener('keydown', this.printShortCutDetect);
-    // @ts-ignore - tsc thinks document can't listen for `copy`
+    // @ts-ignore - TODO(bckenny): tsc thinks document can't listen for `copy`. Remove ignore in 3.1.
     this._document.addEventListener('copy', this.onCopy);
   }
 
@@ -447,6 +447,7 @@ class ReportUIFeatures {
    */
   getReportHtml() {
     this._resetUIState();
+    // @ts-ignore - technically documentElement can be null, but that's dumb - https://dom.spec.whatwg.org/#document-element
     return this._document.documentElement.outerHTML;
   }
 
diff --git a/lighthouse-core/report/html/renderer/util.js b/lighthouse-core/report/html/renderer/util.js
index b5c3ad0c3..b059a2c75 100644
--- a/lighthouse-core/report/html/renderer/util.js
+++ b/lighthouse-core/report/html/renderer/util.js
@@ -223,6 +223,7 @@ class Util {
    * @return {string}
    */
   static formatDateTime(date) {
+    /** @type {Intl.DateTimeFormatOptions} */
     const options = {
       month: 'short', day: 'numeric', year: 'numeric',
       hour: 'numeric', minute: 'numeric', timeZoneName: 'short',
@@ -444,6 +445,10 @@ class Util {
  */
 Util.numberDateLocale = 'en';
 
+/**
+ * Report-renderer-specific strings.
+ * @type {LH.I18NRendererStrings}
+ */
 Util.UIStrings = {
   /** Disclaimer shown to users below the metric values (First Contentful Paint, Time to Interactive, etc) to warn them that the numbers they see will likely change slightly the next time they run Lighthouse. */
   varianceDisclaimer: 'Values are estimated and may vary.',
diff --git a/lighthouse-core/runner.js b/lighthouse-core/runner.js
index c0eeb0d2b..117f115ed 100644
--- a/lighthouse-core/runner.js
+++ b/lighthouse-core/runner.js
@@ -368,6 +368,7 @@ class Runner {
       const ArtifactClass = require('./gather/computed/' + filename);
       const artifact = new ArtifactClass(computedArtifacts);
       // define the request* function that will be exposed on `artifacts`
+      // @ts-ignore - doesn't have an index signature, so can't be set dynamically.
       computedArtifacts['request' + artifact.name] = artifact.request.bind(artifact);
     });
 
diff --git a/lighthouse-core/scoring.js b/lighthouse-core/scoring.js
index 1768c782b..ddbab9fdb 100644
--- a/lighthouse-core/scoring.js
+++ b/lighthouse-core/scoring.js
@@ -50,6 +50,7 @@ class ReportScoring {
    * @return {Object<string, LH.Result.Category>}
    */
   static scoreAllCategories(configCategories, resultsByAuditId) {
+    /** @type {Record<string, LH.Result.Category>} */
     const scoredCategories = {};
 
     for (const [categoryId, configCategory] of Object.entries(configCategories)) {
diff --git a/lighthouse-core/scripts/i18n/assert-strings-collected.sh b/lighthouse-core/scripts/i18n/assert-strings-collected.sh
index c5d6a9ae6..df9a2e90d 100755
--- a/lighthouse-core/scripts/i18n/assert-strings-collected.sh
+++ b/lighthouse-core/scripts/i18n/assert-strings-collected.sh
@@ -30,7 +30,7 @@ cp "$collectedstringsPath" "$currentstringsPath"
 
 colorText "Collecting strings..." "$purple"
 set -x
-node "$lhroot_path/lighthouse-core/scripts/i18n/collect-strings.js"
+node "$lhroot_path/lighthouse-core/scripts/i18n/collect-strings.js" || exit 1
 set +x
 
 cp "$collectedstringsPath" "$freshstringsPath"
diff --git a/lighthouse-core/scripts/i18n/collect-strings.js b/lighthouse-core/scripts/i18n/collect-strings.js
index f67274823..4a5fc225e 100644
--- a/lighthouse-core/scripts/i18n/collect-strings.js
+++ b/lighthouse-core/scripts/i18n/collect-strings.js
@@ -103,6 +103,7 @@ function collectAllStringsInDir(dir, strings = {}) {
  */
 function writeStringsToLocaleFormat(locale, strings) {
   const fullPath = path.join(LH_ROOT, `lighthouse-core/lib/locales/${locale}.json`);
+  /** @type {Record<string, ICUMessageDefn>} */
   const output = {};
   const sortedEntries = Object.entries(strings).sort(([keyA], [keyB]) => keyA.localeCompare(keyB));
   for (const [key, defn] of sortedEntries) {
diff --git a/lighthouse-extension/app/src/lighthouse-ext-background.js b/lighthouse-extension/app/src/lighthouse-ext-background.js
index 4b8b46146..59d11c125 100644
--- a/lighthouse-extension/app/src/lighthouse-ext-background.js
+++ b/lighthouse-extension/app/src/lighthouse-ext-background.js
@@ -122,7 +122,9 @@ function createReportPageAsBlob(reportHtml) {
  */
 function saveSettings(settings) {
   const storage = {
+    /** @type {Record<string, boolean>} */
     [STORAGE_KEY]: {},
+    /** @type {Record<string, boolean>} */
     [SETTINGS_KEY]: {},
   };
 
@@ -149,6 +151,7 @@ function loadSettings() {
     chrome.storage.local.get([STORAGE_KEY, SETTINGS_KEY], result => {
       // Start with list of all default categories set to true so list is
       // always up to date.
+      /** @type {Record<string, boolean>} */
       const defaultCategories = {};
       background.getDefaultCategories().forEach(category => {
         defaultCategories[category.id] = true;


Test patch:
diff --git a/lighthouse-core/test/gather/computed/metrics/first-meaningful-paint-test.js b/lighthouse-core/test/gather/computed/metrics/first-meaningful-paint-test.js
index 51accdf4b..278a1bf29 100644
--- a/lighthouse-core/test/gather/computed/metrics/first-meaningful-paint-test.js
+++ b/lighthouse-core/test/gather/computed/metrics/first-meaningful-paint-test.js
@@ -26,6 +26,15 @@ describe('Metrics: FMP', () => {
   let trace;
   let devtoolsLog;
 
+  function addEmptyTask() {
+    const mainThreadEvt = trace.traceEvents.find(e => e.name === 'TracingStartedInPage');
+    trace.traceEvents.push({
+      ...mainThreadEvt,
+      cat: 'toplevel',
+      name: 'TaskQueueManager::ProcessTaskFromWorkQueue',
+    });
+  }
+
   beforeEach(() => {
     artifacts = Runner.instantiateComputedArtifacts();
     settings = {throttlingMethod: 'provided'};
@@ -60,6 +69,7 @@ describe('Metrics: FMP', () => {
 
   it('handles cases when there was a tracingStartedInPage after navStart', async () => {
     trace = lateTracingStartedTrace;
+    addEmptyTask();
     const result = await artifacts.requestFirstMeaningfulPaint({trace, devtoolsLog, settings});
     assert.equal(Math.round(result.timing), 530);
     assert.equal(result.timestamp, 29344070867);
@@ -67,6 +77,7 @@ describe('Metrics: FMP', () => {
 
   it('handles cases when there was a tracingStartedInPage after navStart #2', async () => {
     trace = badNavStartTrace;
+    addEmptyTask();
     const result = await artifacts.requestFirstMeaningfulPaint({trace, devtoolsLog, settings});
     assert.equal(Math.round(result.timing), 632);
     assert.equal(result.timestamp, 8886056891);
@@ -74,6 +85,7 @@ describe('Metrics: FMP', () => {
 
   it('handles cases when it appears before FCP', async () => {
     trace = preactTrace;
+    addEmptyTask();
     const result = await artifacts.requestFirstMeaningfulPaint({trace, devtoolsLog, settings});
     assert.equal(Math.round(result.timing), 878);
     assert.equal(result.timestamp, 1805797262960);
@@ -81,6 +93,7 @@ describe('Metrics: FMP', () => {
 
   it('handles cases when no FMP exists', async () => {
     trace = noFMPtrace;
+    addEmptyTask();
     const result = await artifacts.requestFirstMeaningfulPaint({trace, devtoolsLog, settings});
     assert.equal(Math.round(result.timing), 4461);
     assert.equal(result.timestamp, 2146740268666);
@@ -88,6 +101,7 @@ describe('Metrics: FMP', () => {
 
   it('handles cases when no FCP exists', async () => {
     trace = noFCPtrace;
+    addEmptyTask();
     const result = await artifacts.requestFirstMeaningfulPaint({trace, devtoolsLog, settings});
     assert.equal(Math.round(result.timing), 482);
     assert.equal(result.timestamp, 2149509604903);
diff --git a/lighthouse-core/test/gather/computed/page-dependency-graph-test.js b/lighthouse-core/test/gather/computed/page-dependency-graph-test.js
index b7686fda2..1cbc26f46 100644
--- a/lighthouse-core/test/gather/computed/page-dependency-graph-test.js
+++ b/lighthouse-core/test/gather/computed/page-dependency-graph-test.js
@@ -27,6 +27,8 @@ function createRequest(
   return {requestId, url, startTime, endTime, initiator, resourceType};
 }
 
+const TOPLEVEL_TASK_NAME = 'TaskQueueManager::ProcessTaskFromWorkQueue';
+
 /* eslint-env jest */
 describe('PageDependencyGraph computed artifact:', () => {
   let computedArtifacts;
@@ -34,7 +36,7 @@ describe('PageDependencyGraph computed artifact:', () => {
 
   function addTaskEvents(startTs, duration, evts) {
     const mainEvent = {
-      name: 'TaskQueueManager::ProcessTaskFromWorkQueue',
+      name: TOPLEVEL_TASK_NAME,
       tid: 1,
       ts: startTs * 1000,
       dur: duration * 1000,
@@ -149,6 +151,8 @@ describe('PageDependencyGraph computed artifact:', () => {
       const request4 = createRequest(4, '4', 10, {url: '2'});
       const networkRecords = [request1, request2, request3, request4];
 
+      addTaskEvents(0, 0, []);
+
       const graph = PageDependencyGraph.createGraph(traceOfTab, networkRecords);
       const nodes = [];
       graph.traverse(node => nodes.push(node));
@@ -202,6 +206,8 @@ describe('PageDependencyGraph computed artifact:', () => {
       const request4 = createRequest(4, '4', 10, {url: '2'});
       const networkRecords = [request1, request2, request3, request4];
 
+      addTaskEvents(0, 0, []);
+
       const graph = PageDependencyGraph.createGraph(traceOfTab, networkRecords);
       const nodes = [];
       graph.traverse(node => nodes.push(node));
@@ -261,6 +267,8 @@ describe('PageDependencyGraph computed artifact:', () => {
       const request2 = createRequest(2, '2', 5);
       const networkRecords = [request1, request2];
 
+      addTaskEvents(0, 0, []);
+
       const graph = PageDependencyGraph.createGraph(traceOfTab, networkRecords);
       const nodes = [];
       graph.traverse(node => nodes.push(node));
diff --git a/lighthouse-core/test/lib/dependency-graph/simulator/network-analyzer-test.js b/lighthouse-core/test/lib/dependency-graph/simulator/network-analyzer-test.js
index 2bdae4f4e..db6444f71 100644
--- a/lighthouse-core/test/lib/dependency-graph/simulator/network-analyzer-test.js
+++ b/lighthouse-core/test/lib/dependency-graph/simulator/network-analyzer-test.js
@@ -170,7 +170,7 @@ describe('DependencyGraph/Simulator/NetworkAnalyzer', () => {
 
     it('should infer from TTFB when available', () => {
       const timing = {receiveHeadersEnd: 1000};
-      const record = createRecord({startTime: 0, endTime: 1, timing});
+      const record = createRecord({startTime: 0, endTime: 1, timing, resourceType: 'Other'});
       const result = NetworkAnalyzer.estimateRTTByOrigin([record], {
         coarseEstimateMultiplier: 1,
       });