misc: specify types for some dependency-graph options objects (#7962)

* misc: specify types for some dependency-graph options objects * feedback
GoogleChrome · Apr 5, 2019 · 584bad7 · 584bad7
1 parent 7a43125
commit 584bad7
Show file tree

Hide file tree

Showing 6 changed files with 79 additions and 77 deletions.
diff --git a/lighthouse-core/computed/network-analysis.js b/lighthouse-core/computed/network-analysis.js
@@ -12,7 +12,7 @@ const NetworkRecords = require('./network-records.js');
 class NetworkAnalysis {
   /**
    * @param {Array<LH.Artifacts.NetworkRequest>} records
-   * @return {Omit<LH.Artifacts.NetworkAnalysis, 'throughput'|'records'>}
+   * @return {Omit<LH.Artifacts.NetworkAnalysis, 'throughput'>}
    */
   static computeRTTAndServerResponseTime(records) {
     // First pass compute the estimated observed RTT to each origin's servers.

diff --git a/lighthouse-core/lib/dependency-graph/simulator/connection-pool.js b/lighthouse-core/lib/dependency-graph/simulator/connection-pool.js
@@ -18,22 +18,10 @@ const CONNECTIONS_PER_ORIGIN = 6;
 module.exports = class ConnectionPool {
   /**
    * @param {LH.Artifacts.NetworkRequest[]} records
-   * @param {Object=} options
+   * @param {Required<LH.Gatherer.Simulation.Options>} options
    */
   constructor(records, options) {
-    this._options = Object.assign(
-      {
-        rtt: undefined,
-        throughput: undefined,
-        additionalRttByOrigin: new Map(),
-        serverResponseTimeByOrigin: new Map(),
-      },
-      options
-    );
-
-    if (!this._options.rtt || !this._options.throughput) {
-      throw new Error('Cannot create pool with no rtt or throughput');
-    }
+    this._options = options;
 
     this._records = records;
     /** @type {Map<string, TcpConnection[]>} */

diff --git a/lighthouse-core/lib/dependency-graph/simulator/dns-cache.js b/lighthouse-core/lib/dependency-graph/simulator/dns-cache.js
@@ -16,19 +16,9 @@ class DNSCache {
   /**
    * @param {{rtt: number}} options
    */
-  constructor(options) {
-    this._options = Object.assign(
-      {
-        rtt: undefined,
-      },
-      options
-    );
+  constructor({rtt}) {
+    this._rtt = rtt;
 
-    if (!this._options.rtt) {
-      throw new Error('Cannot create DNS cache with no rtt');
-    }
-
-    this._rtt = this._options.rtt;
     /** @type {Map<string, {resolvedAt: number}>} */
     this._resolvedDomainNames = new Map();
   }

diff --git a/lighthouse-core/lib/dependency-graph/simulator/network-analyzer.js b/lighthouse-core/lib/dependency-graph/simulator/network-analyzer.js
@@ -260,14 +260,14 @@ class NetworkAnalyzer {
    * available in the records themselves appears untrustworthy.
    *
    * @param {LH.Artifacts.NetworkRequest[]} records
-   * @param {object} [options]
+   * @param {{forceCoarseEstimates: boolean}} [options]
    * @return {Map<string, boolean>}
    */
   static estimateIfConnectionWasReused(records, options) {
-    options = Object.assign({forceCoarseEstimates: false}, options);
+    const {forceCoarseEstimates = false} = options || {};
 
     // Check if we can trust the connection information coming from the protocol
-    if (!options.forceCoarseEstimates && NetworkAnalyzer.canTrustConnectionInformation(records)) {
+    if (!forceCoarseEstimates && NetworkAnalyzer.canTrustConnectionInformation(records)) {
       // @ts-ignore
       return new Map(records.map(record => [record.requestId, !!record.connectionReused]));
     }
@@ -303,52 +303,46 @@ class NetworkAnalyzer {
    * is unavailable.
    *
    * @param {LH.Artifacts.NetworkRequest[]} records
-   * @param {object} [options]
-   * @return {Map<string, !NetworkAnalyzer.Summary>}
+   * @param {NetworkAnalyzer.RTTEstimateOptions} [options]
+   * @return {Map<string, NetworkAnalyzer.Summary>}
    */
   static estimateRTTByOrigin(records, options) {
-    options = Object.assign(
-      {
-        // TCP connection handshake information will be used when available, but for testing
-        // it's useful to see how the coarse estimates compare with higher fidelity data
-        forceCoarseEstimates: false,
-        // coarse estimates include lots of extra time and noise
-        // multiply by some factor to deflate the estimates a bit
-        coarseEstimateMultiplier: 0.3,
-        // useful for testing to isolate the different methods of estimation
-        useDownloadEstimates: true,
-        useSendStartEstimates: true,
-        useHeadersEndEstimates: true,
-      },
-      options
-    );
+    const {
+      forceCoarseEstimates = false,
+      // coarse estimates include lots of extra time and noise
+      // multiply by some factor to deflate the estimates a bit.
+      coarseEstimateMultiplier = 0.3,
+      useDownloadEstimates = true,
+      useSendStartEstimates = true,
+      useHeadersEndEstimates = true,
+    } = options || {};
 
     let estimatesByOrigin = NetworkAnalyzer._estimateRTTByOriginViaTCPTiming(records);
-    if (!estimatesByOrigin.size || options.forceCoarseEstimates) {
+    if (!estimatesByOrigin.size || forceCoarseEstimates) {
       estimatesByOrigin = new Map();
       const estimatesViaDownload = NetworkAnalyzer._estimateRTTByOriginViaDownloadTiming(records);
       const estimatesViaSendStart = NetworkAnalyzer._estimateRTTByOriginViaSendStartTiming(records);
       const estimatesViaTTFB = NetworkAnalyzer._estimateRTTByOriginViaHeadersEndTiming(records);
 
       for (const [origin, estimates] of estimatesViaDownload.entries()) {
-        if (!options.useDownloadEstimates) continue;
+        if (!useDownloadEstimates) continue;
         estimatesByOrigin.set(origin, estimates);
       }
 
       for (const [origin, estimates] of estimatesViaSendStart.entries()) {
-        if (!options.useSendStartEstimates) continue;
+        if (!useSendStartEstimates) continue;
         const existing = estimatesByOrigin.get(origin) || [];
         estimatesByOrigin.set(origin, existing.concat(estimates));
       }
 
       for (const [origin, estimates] of estimatesViaTTFB.entries()) {
-        if (!options.useHeadersEndEstimates) continue;
+        if (!useHeadersEndEstimates) continue;
         const existing = estimatesByOrigin.get(origin) || [];
         estimatesByOrigin.set(origin, existing.concat(estimates));
       }
 
       for (const estimates of estimatesByOrigin.values()) {
-        estimates.forEach((x, i) => (estimates[i] = x * options.coarseEstimateMultiplier));
+        estimates.forEach((x, i) => (estimates[i] = x * coarseEstimateMultiplier));
       }
     }
 
@@ -361,21 +355,17 @@ class NetworkAnalyzer {
    * estimated automatically if not provided.
    *
    * @param {LH.Artifacts.NetworkRequest[]} records
-   * @param {Object=} options
-   * @return {Map<string, !NetworkAnalyzer.Summary>}
+   * @param {NetworkAnalyzer.RTTEstimateOptions & {rttByOrigin?: Map<string, number>}} [options]
+   * @return {Map<string, NetworkAnalyzer.Summary>}
    */
   static estimateServerResponseTimeByOrigin(records, options) {
-    options = Object.assign(
-      {
-        rttByOrigin: null,
-      },
-      options
-    );
-
-    let rttByOrigin = options.rttByOrigin;
+    let rttByOrigin = (options || {}).rttByOrigin;
     if (!rttByOrigin) {
-      rttByOrigin = NetworkAnalyzer.estimateRTTByOrigin(records, options);
-      for (const [origin, summary] of rttByOrigin.entries()) {
+      /** @type {Map<string, number>} */
+      rttByOrigin = new Map();
+
+      const rttSummaryByOrigin = NetworkAnalyzer.estimateRTTByOrigin(records, options);
+      for (const [origin, summary] of rttSummaryByOrigin.entries()) {
         rttByOrigin.set(origin, summary.min);
       }
     }
@@ -474,3 +464,12 @@ module.exports = NetworkAnalyzer;
  * @property {number} avg
  * @property {number} median
  */
+
+/**
+ * @typedef NetworkAnalyzer.RTTEstimateOptions
+ * @property {boolean} [forceCoarseEstimates] TCP connection handshake information will be used when available, but in some circumstances this data can be unreliable. This flag exposes an option to ignore the handshake data and use the coarse download/TTFB timing data.
+ * @property {number} [coarseEstimateMultiplier] Coarse estimates include lots of extra time and noise multiply by some factor to deflate the estimates a bit.
+ * @property {boolean} [useDownloadEstimates] Useful for testing to isolate the different methods of estimation.
+ * @property {boolean} [useSendStartEstimates] Useful for testing to isolate the different methods of estimation.
+ * @property {boolean} [useHeadersEndEstimates] Useful for testing to isolate the different methods of estimation.
+ */
diff --git a/lighthouse-core/test/lib/dependency-graph/simulator/connection-pool-test.js b/lighthouse-core/test/lib/dependency-graph/simulator/connection-pool-test.js
@@ -29,13 +29,25 @@ describe('DependencyGraph/Simulator/ConnectionPool', () => {
     }, data);
   }
 
+  function simulationOptions(options) {
+    return Object.assign(
+      {
+        rtt: 150,
+        throughput: 1024,
+        additionalRttByOrigin: new Map(),
+        serverResponseTimeByOrigin: new Map(),
+      },
+      options
+    );
+  }
+
   beforeEach(() => {
     requestId = 1;
   });
 
   describe('#constructor', () => {
     it('should create the pool', () => {
-      const pool = new ConnectionPool([record()], {rtt, throughput});
+      const pool = new ConnectionPool([record()], simulationOptions({rtt, throughput}));
       // Make sure 6 connections are created for each origin
       assert.equal(pool._connectionsByOrigin.get('http://example.com').length, 6);
       // Make sure it populates connectionWasReused
@@ -49,28 +61,30 @@ describe('DependencyGraph/Simulator/ConnectionPool', () => {
 
     it('should set TLS properly', () => {
       const recordA = record({url: 'https://example.com'});
-      const pool = new ConnectionPool([recordA], {rtt, throughput});
+      const pool = new ConnectionPool([recordA], simulationOptions({rtt, throughput}));
       const connection = pool._connectionsByOrigin.get('https://example.com')[0];
       assert.ok(connection._ssl, 'should have set connection TLS');
     });
 
     it('should set H2 properly', () => {
       const recordA = record({protocol: 'h2'});
-      const pool = new ConnectionPool([recordA], {rtt, throughput});
+      const pool = new ConnectionPool([recordA], simulationOptions({rtt, throughput}));
       const connection = pool._connectionsByOrigin.get('http://example.com')[0];
       assert.ok(connection.isH2(), 'should have set HTTP/2');
     });
 
     it('should set origin-specific RTT properly', () => {
       const additionalRttByOrigin = new Map([['http://example.com', 63]]);
-      const pool = new ConnectionPool([record()], {rtt, throughput, additionalRttByOrigin});
+      const pool = new ConnectionPool([record()],
+          simulationOptions({rtt, throughput, additionalRttByOrigin}));
       const connection = pool._connectionsByOrigin.get('http://example.com')[0];
       assert.ok(connection._rtt, rtt + 63);
     });
 
     it('should set origin-specific server latency properly', () => {
       const serverResponseTimeByOrigin = new Map([['http://example.com', 63]]);
-      const pool = new ConnectionPool([record()], {rtt, throughput, serverResponseTimeByOrigin});
+      const pool = new ConnectionPool([record()],
+          simulationOptions({rtt, throughput, serverResponseTimeByOrigin}));
       const connection = pool._connectionsByOrigin.get('http://example.com')[0];
       assert.ok(connection._serverLatency, 63);
     });
@@ -80,7 +94,7 @@ describe('DependencyGraph/Simulator/ConnectionPool', () => {
     it('should remember the connection associated with each record', () => {
       const recordA = record();
       const recordB = record();
-      const pool = new ConnectionPool([recordA, recordB], {rtt, throughput});
+      const pool = new ConnectionPool([recordA, recordB], simulationOptions({rtt, throughput}));
 
       const connectionForA = pool.acquire(recordA);
       const connectionForB = pool.acquire(recordB);
@@ -93,15 +107,15 @@ describe('DependencyGraph/Simulator/ConnectionPool', () => {
     });
 
     it('should allocate at least 6 connections', () => {
-      const pool = new ConnectionPool([record()], {rtt, throughput});
+      const pool = new ConnectionPool([record()], simulationOptions({rtt, throughput}));
       for (let i = 0; i < 6; i++) {
         assert.ok(pool.acquire(record()), `did not find connection for ${i}th record`);
       }
     });
 
     it('should allocate all connections', () => {
       const records = new Array(7).fill(undefined, 0, 7).map(() => record());
-      const pool = new ConnectionPool(records, {rtt, throughput});
+      const pool = new ConnectionPool(records, simulationOptions({rtt, throughput}));
       const connections = records.map(record => pool.acquire(record));
       assert.ok(connections[0], 'did not find connection for 1st record');
       assert.ok(connections[5], 'did not find connection for 6th record');
@@ -111,7 +125,8 @@ describe('DependencyGraph/Simulator/ConnectionPool', () => {
     it('should respect observed connection reuse', () => {
       const coldRecord = record();
       const warmRecord = record();
-      const pool = new ConnectionPool([coldRecord, warmRecord], {rtt, throughput});
+      const pool = new ConnectionPool([coldRecord, warmRecord],
+          simulationOptions({rtt, throughput}));
       pool._connectionReusedByRequestId.set(warmRecord.requestId, true);
 
       assert.ok(pool.acquire(coldRecord), 'should have acquired connection');
@@ -139,7 +154,8 @@ describe('DependencyGraph/Simulator/ConnectionPool', () => {
     it('should ignore observed connection reuse when flag is present', () => {
       const coldRecord = record();
       const warmRecord = record();
-      const pool = new ConnectionPool([coldRecord, warmRecord], {rtt, throughput});
+      const pool = new ConnectionPool([coldRecord, warmRecord],
+          simulationOptions({rtt, throughput}));
       pool._connectionReusedByRequestId.set(warmRecord.requestId, true);
 
       const opts = {ignoreConnectionReused: true};
@@ -160,7 +176,8 @@ describe('DependencyGraph/Simulator/ConnectionPool', () => {
       const recordA = record();
       const recordB = record();
       const recordC = record();
-      const pool = new ConnectionPool([recordA, recordB, recordC], {rtt, throughput});
+      const pool = new ConnectionPool([recordA, recordB, recordC],
+          simulationOptions({rtt, throughput}));
       pool._connectionReusedByRequestId.set(recordA.requestId, true);
       pool._connectionReusedByRequestId.set(recordB.requestId, true);
       pool._connectionReusedByRequestId.set(recordC.requestId, true);
@@ -183,13 +200,13 @@ describe('DependencyGraph/Simulator/ConnectionPool', () => {
   describe('.release', () => {
     it('noop for record without connection', () => {
       const recordA = record();
-      const pool = new ConnectionPool([recordA], {rtt, throughput});
+      const pool = new ConnectionPool([recordA], simulationOptions({rtt, throughput}));
       assert.equal(pool.release(recordA), undefined);
     });
 
     it('frees the connection for reissue', () => {
       const records = new Array(6).fill(undefined, 0, 7).map(() => record());
-      const pool = new ConnectionPool(records, {rtt, throughput});
+      const pool = new ConnectionPool(records, simulationOptions({rtt, throughput}));
       records.push(record());
 
       records.forEach(record => pool.acquire(record));

diff --git a/lighthouse-core/test/lib/dependency-graph/simulator/dns-cache-test.js b/lighthouse-core/test/lib/dependency-graph/simulator/dns-cache-test.js
@@ -61,4 +61,12 @@ describe('DependencyGraph/Simulator/DNSCache', () => {
       expect(dns.getTimeUntilResolution(request, {requestedAt: 2000})).toBe(0);
     });
   });
+
+  describe('.setResolvedAt', () => {
+    it('should set the DNS resolution time for a record', () => {
+      dns.setResolvedAt(request.parsedURL.host, 123);
+      const resolutionTime = dns.getTimeUntilResolution(request);
+      expect(resolutionTime).toEqual(123);
+    });
+  });
 });