From 072ff7970f2be7ee2f57d1568502b6c827be71bc Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Mon, 1 May 2017 14:15:18 -0700 Subject: [PATCH 1/4] fix: only record a trace if needed by an audit --- lighthouse-core/config/config.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lighthouse-core/config/config.js b/lighthouse-core/config/config.js index 41159dd46025..1d4cfc731681 100644 --- a/lighthouse-core/config/config.js +++ b/lighthouse-core/config/config.js @@ -482,6 +482,7 @@ class Config { * @return {!Object} fresh passes object */ static generatePassesNeededByGatherers(oldPasses, requiredGatherers) { + const needsTraces = requiredGatherers.has('traces') || requiredGatherers.has('devtoolsLog'); const passes = JSON.parse(JSON.stringify(oldPasses)); const filteredPasses = passes.map(pass => { // remove any unncessary gatherers from within the passes @@ -489,6 +490,10 @@ class Config { gathererName = GatherRunner.getGathererClass(gathererName).name; return requiredGatherers.has(gathererName); }); + + // disable the trace if no audit requires a trace + pass.recordTrace = pass.recordTrace && needsTraces; + return pass; }).filter(pass => { // remove any passes lacking concrete gatherers, unless they are dependent on the trace From 56c4a00485c10c58510f7b5cdef5e568779ee5dc Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Mon, 1 May 2017 15:13:47 -0700 Subject: [PATCH 2/4] add tests --- lighthouse-core/test/config/config-test.js | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/lighthouse-core/test/config/config-test.js b/lighthouse-core/test/config/config-test.js index 6c5e6571d123..f5e642bba99b 100644 --- a/lighthouse-core/test/config/config-test.js +++ b/lighthouse-core/test/config/config-test.js @@ -330,12 +330,26 @@ describe('Config', () => { assert.ok(config.audits.length, 3); assert.equal(config.passes.length, 2); + assert.ok(config.passes[0].recordTrace, 'preserves recordTrace pass'); assert.ok(!config.categories['unused-category'], 'removes unused categories'); assert.equal(config.categories['needed-category'].audits.length, 2); assert.equal(config.categories['other-category'].audits.length, 1); }); - it('filtering works with extension', () => { + it('filtering filters out traces when not needed', () => { + const config = new Config({ + extends: true, + settings: { + onlyCategories: ['accessibility'], + }, + }); + + assert.ok(config.audits.length, 'inherited audits by extension'); + assert.equal(config.passes.length, 1, 'filtered out passes'); + assert.equal(config.passes[0].recordTrace, false, 'turns off tracing if not needed'); + }); + + it('filters works with extension', () => { const config = new Config({ extends: true, settings: { From 4e97e431df229d79436e6c6c0e088563da044f2b Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Mon, 1 May 2017 17:20:47 -0700 Subject: [PATCH 3/4] added warning --- lighthouse-core/config/config.js | 7 +++++-- lighthouse-core/test/config/config-test.js | 4 ++++ 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/lighthouse-core/config/config.js b/lighthouse-core/config/config.js index 1d4cfc731681..6826ec961c3e 100644 --- a/lighthouse-core/config/config.js +++ b/lighthouse-core/config/config.js @@ -482,7 +482,7 @@ class Config { * @return {!Object} fresh passes object */ static generatePassesNeededByGatherers(oldPasses, requiredGatherers) { - const needsTraces = requiredGatherers.has('traces') || requiredGatherers.has('devtoolsLog'); + const auditsNeedTrace = requiredGatherers.has('traces'); const passes = JSON.parse(JSON.stringify(oldPasses)); const filteredPasses = passes.map(pass => { // remove any unncessary gatherers from within the passes @@ -492,7 +492,10 @@ class Config { }); // disable the trace if no audit requires a trace - pass.recordTrace = pass.recordTrace && needsTraces; + if (pass.recordTrace && !auditsNeedTrace) { + log.warn('config', `Trace not requested by an audit, dropping trace in ${pass.passName}`); + pass.recordTrace = false; + } return pass; }).filter(pass => { diff --git a/lighthouse-core/test/config/config-test.js b/lighthouse-core/test/config/config-test.js index f5e642bba99b..07522434780f 100644 --- a/lighthouse-core/test/config/config-test.js +++ b/lighthouse-core/test/config/config-test.js @@ -337,6 +337,9 @@ describe('Config', () => { }); it('filtering filters out traces when not needed', () => { + const warnings = []; + const saveWarning = evt => warnings.push(evt); + log.events.addListener('warning', saveWarning); const config = new Config({ extends: true, settings: { @@ -346,6 +349,7 @@ describe('Config', () => { assert.ok(config.audits.length, 'inherited audits by extension'); assert.equal(config.passes.length, 1, 'filtered out passes'); + assert.equal(warnings.length, 1, 'warned about dropping trace'); assert.equal(config.passes[0].recordTrace, false, 'turns off tracing if not needed'); }); From 282873e695d5916df485edc9ddafac769668ce50 Mon Sep 17 00:00:00 2001 From: Patrick Hulce Date: Tue, 2 May 2017 08:40:47 -0700 Subject: [PATCH 4/4] nits --- lighthouse-core/config/config.js | 3 ++- lighthouse-core/test/config/config-test.js | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/lighthouse-core/config/config.js b/lighthouse-core/config/config.js index 6826ec961c3e..a25c51bedd1b 100644 --- a/lighthouse-core/config/config.js +++ b/lighthouse-core/config/config.js @@ -493,7 +493,8 @@ class Config { // disable the trace if no audit requires a trace if (pass.recordTrace && !auditsNeedTrace) { - log.warn('config', `Trace not requested by an audit, dropping trace in ${pass.passName}`); + const passName = pass.passName || 'unknown pass'; + log.warn('config', `Trace not requested by an audit, dropping trace in ${passName}`); pass.recordTrace = false; } diff --git a/lighthouse-core/test/config/config-test.js b/lighthouse-core/test/config/config-test.js index 07522434780f..2a93d5dc82b8 100644 --- a/lighthouse-core/test/config/config-test.js +++ b/lighthouse-core/test/config/config-test.js @@ -347,6 +347,7 @@ describe('Config', () => { }, }); + log.events.removeListener('warning', saveWarning); assert.ok(config.audits.length, 'inherited audits by extension'); assert.equal(config.passes.length, 1, 'filtered out passes'); assert.equal(warnings.length, 1, 'warned about dropping trace');