core(network-analyzer): more distrustful of chrome connection info (#…

…4828)
GoogleChrome · Mar 21, 2018 · 1c23205 · 1c23205
1 parent dc3c21d
commit 1c23205
Show file tree

Hide file tree

Showing 3 changed files with 75 additions and 10 deletions.
diff --git a/lighthouse-core/lib/dependency-graph/simulator/network-analyzer.js b/lighthouse-core/lib/dependency-graph/simulator/network-analyzer.js
@@ -185,6 +185,23 @@ class NetworkAnalyzer {
     });
   }
 
+  /**
+   * @param {LH.NetworkRequest[]} records
+   * @return {boolean}
+   */
+  static canTrustConnectionInformation(records) {
+    const connectionIdWasStarted = new Map();
+    for (const record of records) {
+      const started = connectionIdWasStarted.get(record.connectionId) || !record.connectionReused;
+      connectionIdWasStarted.set(record.connectionId, started);
+    }
+
+    // We probably can't trust the network information if all the connection IDs were the same
+    if (connectionIdWasStarted.size <= 1) return false;
+    // Or if there were connections that were always reused (a connection had to have started at some point)
+    return Array.from(connectionIdWasStarted.values()).every(started => started);
+  }
+
   /**
    * Returns a map of requestId -> connectionReused, estimating the information if the information
    * available in the records themselves appears untrustworthy.
@@ -196,9 +213,8 @@ class NetworkAnalyzer {
   static estimateIfConnectionWasReused(records, options) {
     options = Object.assign({forceCoarseEstimates: false}, options);
 
-    const connectionIds = new Set(records.map(record => record.connectionId));
-    // If the records actually have distinct connectionIds we can reuse these.
-    if (!options.forceCoarseEstimates && connectionIds.size > 1) {
+    // Check if we can trust the connection information coming from the protocol
+    if (!options.forceCoarseEstimates && NetworkAnalyzer.canTrustConnectionInformation(records)) {
       // @ts-ignore
       return new Map(records.map(record => [record.requestId, !!record.connectionReused]));
     }

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
@@ -67,7 +67,7 @@ describe('DependencyGraph/Simulator/NetworkAnalyzer', () => {
       assert.deepStrictEqual(result, expected);
     });
 
-    it('should estimate values when not trustworthy', () => {
+    it('should estimate values when not trustworthy (duplicate IDs)', () => {
       const records = [
         createRecord({requestId: 1, startTime: 0, endTime: 15}),
         createRecord({requestId: 2, startTime: 10, endTime: 25}),
@@ -80,6 +80,43 @@ describe('DependencyGraph/Simulator/NetworkAnalyzer', () => {
       assert.deepStrictEqual(result, expected);
     });
 
+    it('should estimate values when not trustworthy (connectionReused nonsense)', () => {
+      const records = [
+        createRecord({
+          requestId: 1,
+          connectionId: 1,
+          connectionReused: true,
+          startTime: 0,
+          endTime: 15,
+        }),
+        createRecord({
+          requestId: 2,
+          connectionId: 1,
+          connectionReused: true,
+          startTime: 10,
+          endTime: 25,
+        }),
+        createRecord({
+          requestId: 3,
+          connectionId: 1,
+          connectionReused: true,
+          startTime: 20,
+          endTime: 40,
+        }),
+        createRecord({
+          requestId: 4,
+          connectionId: 2,
+          connectionReused: false,
+          startTime: 30,
+          endTime: 40,
+        }),
+      ];
+
+      const result = NetworkAnalyzer.estimateIfConnectionWasReused(records);
+      const expected = new Map([[1, false], [2, false], [3, true], [4, true]]);
+      assert.deepStrictEqual(result, expected);
+    });
+
     it('should estimate with earliest allowed reuse', () => {
       const records = [
         createRecord({requestId: 1, startTime: 0, endTime: 40}),
@@ -134,6 +171,23 @@ describe('DependencyGraph/Simulator/NetworkAnalyzer', () => {
       assert.deepStrictEqual(result.get('https://example.com'), expected);
     });
 
+    it('should handle untrustworthy connection information', () => {
+      const timing = {sendStart: 100};
+      const recordA = createRecord({startTime: 0, endTime: 1, timing, connectionReused: true});
+      const recordB = createRecord({
+        startTime: 0,
+        endTime: 1,
+        timing,
+        connectionId: 2,
+        connectionReused: true,
+      });
+      const result = NetworkAnalyzer.estimateRTTByOrigin([recordA, recordB], {
+        coarseEstimateMultiplier: 1,
+      });
+      const expected = {min: 50, max: 50, avg: 50, median: 50};
+      assert.deepStrictEqual(result.get('https://example.com'), expected);
+    });
+
     it('should work on a real devtoolsLog', () => {
       return computedArtifacts.requestNetworkRecords(devtoolsLog).then(records => {
         records.forEach(addOriginToRecord);

diff --git a/lighthouse-extension/test/extension-test.js b/lighthouse-extension/test/extension-test.js
@@ -148,12 +148,7 @@ describe('Lighthouse chrome extension', function() {
     }
 
     const auditErrors = await extensionPage.$$eval('.lh-debug', getDebugStrings, selectors);
-    const errors = auditErrors.filter(
-      item =>
-        item.debugString.includes('Audit error:') &&
-        // FIXME(phulce): fix timing failing on travis.
-        !item.debugString.includes('No timing information available')
-    );
+    const errors = auditErrors.filter(item => item.debugString.includes('Audit error:'));
     assert.deepStrictEqual(errors, [], 'Audit errors found within the report');
   });
 });