Address comments by brendankenny

GoogleChrome · brendankenny · Jun 5, 2019 · May 16, 2019 · May 16, 2019 · May 16, 2019
commit ebbc7062e2f2467a077be827a3fb1ac3279f0b65
diff --git a/lighthouse-core/computed/metrics/cumulative-long-queuing-delay.js b/lighthouse-core/computed/metrics/cumulative-long-queuing-delay.js
@@ -69,8 +69,7 @@ class CumulativeLongQueuingDelay extends ComputedMetric {
       // optimizing to reach FCP as fast as possible without having to worry about task lengths.
       //
       // TTI is picked as the upper bound because we want a well defined end point so that the
-      // metric does not rely on how long we trace. There is very low probability of encountering a
-      // Long Queuing Delay region past TTI.
+      // metric does not rely on how long we trace.
       if (region.end < fcpTimeInMs) continue;
       if (region.start > interactiveTimeMs) continue;
 

diff --git a/lighthouse-core/computed/metrics/lantern-cumulative-long-queuing-delay.js b/lighthouse-core/computed/metrics/lantern-cumulative-long-queuing-delay.js
@@ -47,15 +47,18 @@ class LanternCumulativeLongQueuingDelay extends LanternMetric {
    * @return {LH.Gatherer.Simulation.Result}
    */
   static getEstimateFromSimulation(simulation, extras) {
-    // Intentionally use the opposite FCP estimate, a more pessimistic FCP means that more tasks are
-    // excluded from the CumulativeLongQueuingDelay computation, so a higher FCP means lower value
-    // for the same work.
+    // Intentionally use the opposite FCP estimate. A pessimistic FCP is higher than equal to an
+    // optimistic FCP, which means potentially more tasks are excluded from the
+    // CumulativeLongQueuingDelay computation. So a more pessimistic FCP gives a more optimistic
+    // CumulativeLongQueuingDelay for the same work.
     const fcpTimeInMs = extras.optimistic
       ? extras.fcpResult.pessimisticEstimate.timeInMs
       : extras.fcpResult.optimisticEstimate.timeInMs;
 
-    // Optimistic Interactive Time means fewer tasks were considered while counting
-    // CumulativeLongQueuingDelay, which should result in a lower (better) value.
+    // Similarly, we always have pessimistic TTI >= optimistic TTI. Therefore, picking optimistic
+    // TTI means our window of interest is smaller and thus potentially more tasks are excluded from
+    // CumulativeLongQueuingDelay computation, yielding a lower (more optimistic)
+    // CumulativeLongQueuingDelay value for the same work.
     const interactiveTimeMs = extras.optimistic
       ? extras.interactiveResult.optimisticEstimate.timeInMs
       : extras.interactiveResult.pessimisticEstimate.timeInMs;
@@ -101,7 +104,7 @@ class LanternCumulativeLongQueuingDelay extends LanternMetric {
 
     for (const [node, timing] of nodeTimings.entries()) {
       if (node.type !== BaseNode.TYPES.CPU) continue;
-      // Filtering out events below minimum duration to avoid unnecessary sorting work later.
+      // Filtering out events below minimum duration.
       if (timing.duration < minDurationMs) continue;
 
       events.push({
@@ -111,7 +114,7 @@ class LanternCumulativeLongQueuingDelay extends LanternMetric {
       });
     }
 
-    return events.sort((a, b) => a.start - b.start);
+    return events;
   }
 }
 

diff --git a/lighthouse-core/test/audits/metrics/cumulative-long-queuing-delay-test.js b/lighthouse-core/test/audits/metrics/cumulative-long-queuing-delay-test.js
@@ -5,15 +5,15 @@
  */
 'use strict';
 
-const Audit = require('../../../audits/metrics/cumulative-long-queuing-delay.js');
-const options = Audit.defaultOptions;
+const cLQDAudit = require('../../../audits/metrics/cumulative-long-queuing-delay.js');
+const options = cLQDAudit.defaultOptions;
 
 const pwaTrace = require('../../fixtures/traces/progressive-app-m60.json');
 
 function generateArtifactsWithTrace(trace) {
   return {
-    traces: {[Audit.DEFAULT_PASS]: trace},
-    devtoolsLogs: {[Audit.DEFAULT_PASS]: []},
+    traces: {[cLQDAudit.DEFAULT_PASS]: trace},
+    devtoolsLogs: {[cLQDAudit.DEFAULT_PASS]: []},
   };
 }
 /* eslint-env jest */
@@ -23,10 +23,10 @@ describe('Performance: cumulative-long-queuing-delay audit', () => {
     const artifacts = generateArtifactsWithTrace(pwaTrace);
     const settings = {throttlingMethod: 'provided'};
     const context = {options, settings, computedCache: new Map()};
-    const output = await Audit.audit(artifacts, context);
+    const output = await cLQDAudit.audit(artifacts, context);
 
     expect(output.numericValue).toBeCloseTo(48.3, 1);
-    expect(output.score).toBeCloseTo(1, 2);
+    expect(output.score).toBe(1);
     expect(output.displayValue).toBeDisplayString('50\xa0ms');
   });
 });