Repository: GoogleChrome/lighthouse
Pull request #6358 (2018-10-22 17:04:03)

misc(proto): make NO_ERROR default in proto
**Summary**
Make NO_ERROR the default enum in proto definition and add runtimeError removal on default from the preprocessor.

**Related Issues/PRs**
addresses, but does not fix totally: #6336


Patch:
diff --git a/.travis.yml b/.travis.yml
index 210f03e6e..362465fae 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -6,7 +6,7 @@ branches:
 matrix:
   include:
     - node_js: "8"
-    - node_js: "9"
+    - node_js: "10"
       if: head_branch IS blank AND branch = master
 dist: trusty
 cache:
@@ -42,7 +42,7 @@ script:
   - yarn test-viewer
   - yarn test-lantern
   - yarn i18n:checks
-  # _JAVA_OPTIONS is breaking parsing of compiler output. See #3338.
+  # _JAVA_OPTIONS is breaking parsing of `compile-devtools` output. See #3338.
   - unset _JAVA_OPTIONS
   # FIXME(paulirish): re-enable after a roll of LH->CDT
   # - yarn compile-devtools
diff --git a/lighthouse-core/audits/image-aspect-ratio.js b/lighthouse-core/audits/image-aspect-ratio.js
index ed70e01f6..750acafff 100644
--- a/lighthouse-core/audits/image-aspect-ratio.js
+++ b/lighthouse-core/audits/image-aspect-ratio.js
@@ -77,6 +77,8 @@ class ImageAspectRatio extends Audit {
       // - filter all svgs as they have no natural dimensions to audit
       return image.networkRecord &&
         image.networkRecord.mimeType !== 'image/svg+xml' &&
+        image.naturalHeight > 5 &&
+        image.naturalWidth > 5 &&
         image.width &&
         image.height &&
         !image.usesObjectFit;
diff --git a/lighthouse-core/lib/proto-preprocessor.js b/lighthouse-core/lib/proto-preprocessor.js
index a23d1d660..721a488d1 100644
--- a/lighthouse-core/lib/proto-preprocessor.js
+++ b/lighthouse-core/lib/proto-preprocessor.js
@@ -32,6 +32,11 @@ function processForProto(result) {
     }
   }
 
+  // Remove runtimeError if it is NO_ERROR
+  if (reportJson.runtimeError && reportJson.runtimeError.code === 'NO_ERROR') {
+    delete reportJson.runtimeError;
+  }
+
   // Clean up actions that require 'audits' to exist
   if (reportJson.audits) {
     Object.keys(reportJson.audits).forEach(auditName => {
diff --git a/proto/lighthouse-result.proto b/proto/lighthouse-result.proto
index 7e8ba7332..c88d8e4ce 100644
--- a/proto/lighthouse-result.proto
+++ b/proto/lighthouse-result.proto
@@ -6,42 +6,41 @@ import "google/protobuf/wrappers.proto";
 
 // Canonical list of errors created by Lighthouse.
 enum LighthouseError {
-  UNDEFINED = 0;
   // No error in Lighthouse; the results are reliable.
-  NO_ERROR = 1;
+  NO_ERROR = 0;
   // An uncategorized error occurred, likely a JavaScript exception.
-  UNKNOWN_ERROR = 2;
+  UNKNOWN_ERROR = 1;
   // The trace did not contain any screenshot events.
-  NO_SPEEDLINE_FRAMES = 3;
+  NO_SPEEDLINE_FRAMES = 2;
   // No visual change between the beginning and end of load.
-  SPEEDINDEX_OF_ZERO = 4;
+  SPEEDINDEX_OF_ZERO = 3;
   // The trace did not contain any screenshot events.
-  NO_SCREENSHOTS = 5;
+  NO_SCREENSHOTS = 4;
   // The computed speedindex results are non-finite.
-  INVALID_SPEEDLINE = 6;
+  INVALID_SPEEDLINE = 5;
   // The trace did not contain a TracingStartedInPage event.
-  NO_TRACING_STARTED = 7;
+  NO_TRACING_STARTED = 6;
   // The trace did not contain a navigationStart event.
-  NO_NAVSTART = 8;
+  NO_NAVSTART = 7;
   // The trace did not contain a firstContentfulPaint event.
-  NO_FCP = 9;
+  NO_FCP = 8;
   // The trace did not contain a domContentLoaded event.
-  NO_DCL = 10;
+  NO_DCL = 9;
   // No network request could be identified as the primary HTML document.
-  NO_DOCUMENT_REQUEST = 11;
+  NO_DOCUMENT_REQUEST = 10;
   // The HTML document's network request failed due to Chrome-internal reasons
   // (canceled, blocked, etc).
-  FAILED_DOCUMENT_REQUEST = 12;
+  FAILED_DOCUMENT_REQUEST = 11;
   // The HTML document's network request completed, but returned an HTTP status
   // code of 4xx or 5xx.
-  ERRORED_DOCUMENT_REQUEST = 13;
+  ERRORED_DOCUMENT_REQUEST = 12;
   // Chromium's tracing controller did not manage to begin tracing across
   // processes. Typically fixed by restarting Chromium.
-  TRACING_ALREADY_STARTED = 14;
+  TRACING_ALREADY_STARTED = 13;
   // The trace data wasn't parsed correctly.
-  PARSING_PROBLEM = 15;
+  PARSING_PROBLEM = 14;
   // The trace data failed to stream over the protocol.
-  READ_FAILED = 16;
+  READ_FAILED = 15;
 }
 
 // The overarching Lighthouse Response object (LHR)
diff --git a/proto/sample_v2_round_trip.json b/proto/sample_v2_round_trip.json
index 8542cf721..37b142e56 100644
--- a/proto/sample_v2_round_trip.json
+++ b/proto/sample_v2_round_trip.json
@@ -3291,8 +3291,5 @@
     "lighthouseVersion": "3.2.0", 
     "requestedUrl": "http://localhost:10200/dobetterweb/dbw_tester.html", 
     "runWarnings": [], 
-    "runtimeError": {
-        "code": "NO_ERROR"
-    }, 
     "userAgent": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/66.0.3358.0 Safari/537.36"
 }
\ No newline at end of file


Test patch:
diff --git a/lighthouse-core/test/audits/image-aspect-ratio-test.js b/lighthouse-core/test/audits/image-aspect-ratio-test.js
index b7ae1dc1d..ac438abe9 100644
--- a/lighthouse-core/test/audits/image-aspect-ratio-test.js
+++ b/lighthouse-core/test/audits/image-aspect-ratio-test.js
@@ -137,11 +137,10 @@ describe('Images: aspect-ratio audit', () => {
     },
   });
 
-  testImage('has invalid natural sizing information', {
+  testImage('is placeholder image', {
     rawValue: true,
-    warning: 'Invalid image sizing information https://google.com/logo.png',
-    clientSize: [100, 100],
-    naturalSize: [0, 0],
+    clientSize: [300, 220],
+    naturalSize: [1, 1],
     props: {
       isCss: false,
       usesObjectFit: false,
diff --git a/lighthouse-core/test/lib/i18n/locales-test.js b/lighthouse-core/test/lib/i18n/locales-test.js
index cf66ca390..eba944fa3 100644
--- a/lighthouse-core/test/lib/i18n/locales-test.js
+++ b/lighthouse-core/test/lib/i18n/locales-test.js
@@ -11,10 +11,25 @@ const assert = require('assert');
 /* eslint-env jest */
 
 describe('locales', () => {
-  it('has only canonical language tags', () => {
+  it('has only canonical (or expected-deprecated) language tags', () => {
+    // Map of deprecated codes to their canonical version. Depending on the ICU
+    // version used to run Lighthouse/this test, these *may* come back as their
+    // substitute, not themselves.
+    const deprecatedCodes = {
+      in: 'id',
+      iw: 'he',
+    };
+
     for (const locale of Object.keys(locales)) {
       const canonicalLocale = Intl.getCanonicalLocales(locale)[0];
-      assert.strictEqual(locale, canonicalLocale);
+      const substitute = deprecatedCodes[locale];
+      assert.ok(locale === canonicalLocale || substitute === canonicalLocale,
+          `locale code '${locale}' not canonical`);
+    }
+
+    // Deprecation subsitutes should be removed from the test if no longer used.
+    for (const locale of Object.keys(deprecatedCodes)) {
+      assert.ok(locales[locale], `${locale} substitute should be removed from test`);
     }
   });
 
diff --git a/lighthouse-core/test/lib/proto-preprocessor-test.js b/lighthouse-core/test/lib/proto-preprocessor-test.js
index b35982970..cadb0a455 100644
--- a/lighthouse-core/test/lib/proto-preprocessor-test.js
+++ b/lighthouse-core/test/lib/proto-preprocessor-test.js
@@ -27,6 +27,30 @@ describe('processing for proto', () => {
     expect(JSON.parse(output)).toMatchObject(expectation);
   });
 
+  it('cleans up default runtimeErrors', () => {
+    const input = {
+      'runtimeError': {
+        'code': 'NO_ERROR',
+      },
+    };
+
+    const output = processForProto(JSON.stringify(input));
+
+    expect(JSON.parse(output)).not.toHaveProperty('runtimeError');
+  });
+
+  it('non-default runtimeErrors are untouched', () => {
+    const input = {
+      'runtimeError': {
+        'code': 'ERROR_NO_DOCUMENT_REQUEST',
+      },
+    };
+
+    const output = processForProto(JSON.stringify(input));
+
+    expect(JSON.parse(output)).toMatchObject(input);
+  });
+
   it('cleans up audits', () => {
     const input = {
       'audits': {