Remove recordNetwork (#2102)

* always record network and save in artifacts
GoogleChrome · May 1, 2017 · 07e0aab · 07e0aab
1 parent 283af87
commit 07e0aab
Show file tree

Hide file tree

Showing 9 changed files with 22 additions and 62 deletions.
diff --git a/lighthouse-cli/test/smokehouse/pwa-config.js b/lighthouse-cli/test/smokehouse/pwa-config.js
@@ -21,7 +21,6 @@
  */
 module.exports = {
   passes: [{
-    recordNetwork: true,
     recordTrace: true,
     gatherers: [
       'url',
@@ -33,13 +32,13 @@ module.exports = {
   },
   {
     passName: 'offlinePass',
-    recordNetwork: true,
     gatherers: [
       'service-worker',
       'offline'
     ]
   },
   {
+    passName: 'domstats',
     gatherers: [
       'dobetterweb/domstats',
       'http-redirect'

diff --git a/lighthouse-core/closure/typedefs/Config.js b/lighthouse-core/closure/typedefs/Config.js
@@ -26,9 +26,6 @@
  */
 var PassConfig = function() {};
 
-/** type {boolean} */
-PassConfig.prototype.recordNetwork;
-
 /** type {boolean} */
 PassConfig.prototype.recordTrace;
 

diff --git a/lighthouse-core/config/config.js b/lighthouse-core/config/config.js
@@ -129,17 +129,22 @@ function validatePasses(passes, audits, rootPath) {
     });
   });
 
-  // Log if multiple passes require trace or network recording and could overwrite one another.
+  // Passes must have unique `passName`s. Throw otherwise.
   const usedNames = new Set();
+  let defaultUsed = false;
   passes.forEach((pass, index) => {
-    if (!pass.recordNetwork && !pass.recordTrace) {
-      return;
+    let passName = pass.passName;
+    if (!passName) {
+      if (defaultUsed) {
+        throw new Error(`passes[${index}] requires a passName`);
+      }
+
+      passName = Audit.DEFAULT_PASS;
+      defaultUsed = true;
     }
 
-    const passName = pass.passName || Audit.DEFAULT_PASS;
     if (usedNames.has(passName)) {
-      log.warn('config', `passes[${index}] may overwrite trace or network ` +
-          `data of earlier pass without a unique passName (repeated name: ${passName}.`);
+      throw new Error(`Passes must have unique names (repeated passName: ${passName}.`);
     }
     usedNames.add(passName);
   });

diff --git a/lighthouse-core/config/default.js b/lighthouse-core/config/default.js
@@ -3,7 +3,6 @@ module.exports = {
   "settings": {},
   "passes": [{
     "passName": "defaultPass",
-    "recordNetwork": true,
     "recordTrace": true,
     "pauseBeforeTraceEndMs": 5000,
     "useThrottling": true,
@@ -19,7 +18,6 @@ module.exports = {
   },
   {
     "passName": "offlinePass",
-    "recordNetwork": true,
     "useThrottling": false,
     "gatherers": [
       "service-worker",
@@ -35,7 +33,6 @@ module.exports = {
     ]
   }, {
     "passName": "dbw",
-    "recordNetwork": true,
     "useThrottling": false,
     "gatherers": [
       "chrome-console-messages",

diff --git a/lighthouse-core/config/plots.json b/lighthouse-core/config/plots.json
@@ -1,6 +1,5 @@
 {
   "passes": [{
-    "recordNetwork": true,
     "recordTrace": true,
     "pauseBeforeTraceEndMs": 5000,
     "useThrottling": true,

diff --git a/lighthouse-core/gather/gather-runner.js b/lighthouse-core/gather/gather-runner.js
@@ -85,7 +85,7 @@ class GatherRunner {
     return Promise.resolve()
       // Begin tracing only if requested by config.
       .then(_ => options.config.recordTrace && driver.beginTrace(options.flags))
-      // Network is always recorded for internal use, even if not saved as artifact.
+      // Network is always recorded for gatherer use
       .then(_ => driver.beginNetworkCollect(options))
       // Navigate.
       .then(_ => driver.gotoURL(options.url, {
@@ -250,9 +250,8 @@ class GatherRunner {
       return driver.endNetworkCollect();
     }).then(networkRecords => {
       GatherRunner.assertPageLoaded(options.url, driver, networkRecords);
-
-      // Network records only given to gatherers if requested by config.
-      config.recordNetwork && (passData.networkRecords = networkRecords);
+      // Expose devtoolsLog & networkRecords to gatherers
+      passData.networkRecords = networkRecords;
       log.verbose('statusEnd', status);
     });
 
@@ -352,15 +351,14 @@ class GatherRunner {
             .then(_ => GatherRunner.pass(runOptions, gathererResults))
             .then(_ => GatherRunner.afterPass(runOptions, gathererResults))
             .then(passData => {
-              // If requested by config, merge trace and network data for this
-              // pass into tracingData.
+              // If requested by config, merge trace -> tracingData
               const passName = config.passName || Audit.DEFAULT_PASS;
               if (config.recordTrace) {
                 tracingData.traces[passName] = passData.trace;
                 tracingData.devtoolsLogs[passName] = passData.devtoolsLog;
               }
-              config.recordNetwork &&
-                  (tracingData.networkRecords[passName] = passData.networkRecords);
+
+              tracingData.networkRecords[passName] = passData.networkRecords;
 
               if (passIndex === 0) {
                 urlAfterRedirects = runOptions.url;

diff --git a/lighthouse-core/test/config/config-test.js b/lighthouse-core/test/config/config-test.js
@@ -73,55 +73,29 @@ describe('Config', () => {
     const unlikelyPassName = 'unlikelyPassName';
     const configJson = {
       passes: [{
-        recordNetwork: true,
         passName: unlikelyPassName,
         gatherers: []
       }, {
-        recordNetwork: true,
         passName: unlikelyPassName,
         gatherers: []
       }],
       audits: []
     };
 
-    return new Promise((resolve, reject) => {
-      const warningListener = function(args) {
-        const warningMsg = args[1];
-        if (new RegExp(`overwrite.+${unlikelyPassName}`).test(warningMsg)) {
-          log.events.removeListener('warning', warningListener);
-          resolve();
-        }
-      };
-      log.events.addListener('warning', warningListener);
-
-      const _ = new Config(configJson);
-    });
+    assert.throws(_ => new Config(configJson), /unique/);
   });
 
   it('warns when traced twice with no passNames specified', () => {
     const configJson = {
       passes: [{
-        recordNetwork: true,
         gatherers: []
       }, {
-        recordNetwork: true,
         gatherers: []
       }],
       audits: []
     };
 
-    return new Promise((resolve, reject) => {
-      const warningListener = function(args) {
-        const warningMsg = args[1];
-        if (new RegExp(`overwrite.+${Audit.DEFAULT_PASS}`).test(warningMsg)) {
-          log.events.removeListener('warning', warningListener);
-          resolve();
-        }
-      };
-      log.events.addListener('warning', warningListener);
-
-      const _ = new Config(configJson);
-    });
+    assert.throws(_ => new Config(configJson), /requires a passName/);
   });
 
   it('throws for unknown gatherers', () => {
@@ -299,7 +273,7 @@ describe('Config', () => {
       },
       passes: [
         {recordTrace: true, gatherers: []},
-        {recordNetwork: true, gatherers: ['accessibility']},
+        {passName: 'a11y', gatherers: ['accessibility']},
       ],
       audits: [
         'accessibility/color-contrast',
@@ -421,7 +395,7 @@ describe('Config', () => {
     it('should merge passes', () => {
       const configA = {
         passes: [
-          {passName: 'passA', recordNetwork: true, gatherers: ['a']},
+          {passName: 'passA', gatherers: ['a']},
           {passName: 'passB', gatherers: ['b']},
           {gatherers: ['c']}
         ]

diff --git a/lighthouse-core/test/gather/gather-runner-test.js b/lighthouse-core/test/gather/gather-runner-test.js
@@ -386,7 +386,6 @@ describe('GatherRunner', function() {
     };
 
     const config = {
-      recordNetwork: true,
       gatherers: [{}]
     };
 
@@ -411,7 +410,6 @@ describe('GatherRunner', function() {
     });
 
     const config = {
-      recordNetwork: true,
       gatherers: [
         new TestGatherer()
       ]
@@ -446,7 +444,6 @@ describe('GatherRunner', function() {
 
     const passes = [{
       blankDuration: 0,
-      recordNetwork: true,
       recordTrace: true,
       passName: 'firstPass',
       gatherers: [
@@ -474,13 +471,11 @@ describe('GatherRunner', function() {
   it('respects trace names', () => {
     const passes = [{
       blankDuration: 0,
-      recordNetwork: true,
       recordTrace: true,
       passName: 'firstPass',
       gatherers: [new TestGatherer()]
     }, {
       blankDuration: 0,
-      recordNetwork: true,
       recordTrace: true,
       passName: 'secondPass',
       gatherers: [new TestGatherer()]
@@ -785,7 +780,6 @@ describe('GatherRunner', function() {
     it('rejects if a gatherer does not provide an artifact', () => {
       const passes = [{
         blankDuration: 0,
-        recordNetwork: true,
         recordTrace: true,
         passName: 'firstPass',
         gatherers: [
@@ -804,7 +798,6 @@ describe('GatherRunner', function() {
     it('rejects when domain name can\'t be resolved', () => {
       const passes = [{
         blankDuration: 0,
-        recordNetwork: true,
         recordTrace: true,
         passName: 'firstPass',
         gatherers: []
@@ -835,7 +828,6 @@ describe('GatherRunner', function() {
     it('resolves when domain name can\'t be resolved but is offline', () => {
       const passes = [{
         blankDuration: 0,
-        recordNetwork: true,
         recordTrace: true,
         passName: 'firstPass',
         gatherers: []

diff --git a/lighthouse-core/test/runner-test.js b/lighthouse-core/test/runner-test.js
@@ -443,7 +443,6 @@ describe('Runner', () => {
     const config = new Config({
       passes: [{
         passName: 'firstPass',
-        recordNetwork: true,
         gatherers: ['viewport-dimensions']
       }],