From 82c50514f70809f59d7257fa3afb504c45274ffb Mon Sep 17 00:00:00 2001 From: Paul Lewis Date: Mon, 15 Aug 2016 15:18:41 +0100 Subject: [PATCH] Fixes TTI not being counted in overall score Better checking of aggregation config to prevent happening again. Changes TTI audit rawValue to be a float not string. --- lighthouse-core/aggregator/aggregate.js | 17 +++--- lighthouse-core/audits/time-to-interactive.js | 4 +- lighthouse-core/config/default.json | 2 +- lighthouse-core/test/aggregator/aggregate.js | 53 ++++++++++++++++--- 4 files changed, 60 insertions(+), 16 deletions(-) diff --git a/lighthouse-core/aggregator/aggregate.js b/lighthouse-core/aggregator/aggregate.js index 90d0e55fc726..8c988516d85b 100644 --- a/lighthouse-core/aggregator/aggregate.js +++ b/lighthouse-core/aggregator/aggregate.js @@ -67,24 +67,28 @@ class Aggregate { * @private * @param {!AuditResult} result The audit's output value. * @param {!AggregationCriterion} expected The aggregation's expected value and weighting for this result. + * @param {!string} name The name of the audit. * @return {number} The weighted result. */ - static _convertToWeight(result, expected) { + static _convertToWeight(result, expected, name) { let weight = 0; if (typeof expected === 'undefined' || typeof expected.rawValue === 'undefined' || typeof expected.weight === 'undefined') { - return weight; + const msg = + `Config for ${name} is undefined or does not contain rawValue and weight properties`; + throw new Error(msg); } if (typeof result === 'undefined' || - typeof result.rawValue === 'undefined' || typeof result.score === 'undefined') { - return weight; + const msg = + `Audit result for ${name} is undefined or does not contain score property`; + throw new Error(msg); } - if (typeof result.rawValue !== typeof expected.rawValue) { + if (typeof result.score !== typeof expected.rawValue) { const msg = `Expected rawValue of type ${typeof expected.rawValue}, got ${typeof result.rawValue}`; throw new Error(msg); @@ -181,7 +185,8 @@ class Aggregate { overallScore += Aggregate._convertToWeight( filteredAndRemappedResults[e], - item.criteria[e]); + item.criteria[e], + e); }); return { diff --git a/lighthouse-core/audits/time-to-interactive.js b/lighthouse-core/audits/time-to-interactive.js index 5df3d64f32e5..abeecbd684d1 100644 --- a/lighthouse-core/audits/time-to-interactive.js +++ b/lighthouse-core/audits/time-to-interactive.js @@ -121,7 +121,7 @@ class TTIMetric extends Audit { // Grab this latency and try the threshold again currentLatency = estLatency; } - const timeToInteractive = startTime.toFixed(1); + const timeToInteractive = parseFloat(startTime.toFixed(1)); // Use the CDF of a log-normal distribution for scoring. // < 1200ms: score≈100 @@ -129,7 +129,7 @@ class TTIMetric extends Audit { // >= 15000ms: score≈0 const distribution = TracingProcessor.getLogNormalDistribution(SCORING_MEDIAN, SCORING_POINT_OF_DIMINISHING_RETURNS); - let score = 100 * distribution.computeComplementaryPercentile(timeToInteractive); + let score = 100 * distribution.computeComplementaryPercentile(startTime); // Clamp the score to 0 <= x <= 100. score = Math.min(100, score); diff --git a/lighthouse-core/config/default.json b/lighthouse-core/config/default.json index 637d07faef27..a1cf4e374b77 100644 --- a/lighthouse-core/config/default.json +++ b/lighthouse-core/config/default.json @@ -109,7 +109,7 @@ "weight": 1 }, "time-to-interactive": { - "value": 100, + "rawValue": 100, "weight": 1 }, "scrolling-60fps": { diff --git a/lighthouse-core/test/aggregator/aggregate.js b/lighthouse-core/test/aggregator/aggregate.js index 81b4483fece7..40fa7aa080e7 100644 --- a/lighthouse-core/test/aggregator/aggregate.js +++ b/lighthouse-core/test/aggregator/aggregate.js @@ -125,16 +125,16 @@ describe('Aggregate', () => { 'Cannot remap: test already exists'); }); - it('returns a weight of zero for undefined inputs', () => { - return assert.equal(Aggregate._convertToWeight(), 0); + it('throws for undefined inputs', () => { + return assert.throws(_ => Aggregate._convertToWeight(), 0); }); - it('returns a weight of zero for undefined results', () => { + it('throws for undefined results', () => { const expected = { rawValue: true, weight: 10 }; - return assert.equal(Aggregate._convertToWeight(undefined, expected), 0); + return assert.throws(_ => Aggregate._convertToWeight(undefined, expected)); }); it('returns a weight of zero for undefined expectations', () => { @@ -143,7 +143,7 @@ describe('Aggregate', () => { score: true, displayValue: '' }; - return assert.equal(Aggregate._convertToWeight(result, undefined), 0); + return assert.throws(_ => Aggregate._convertToWeight(result, undefined)); }); it('returns the correct weight for a boolean result', () => { @@ -176,7 +176,7 @@ describe('Aggregate', () => { return assert.equal(Aggregate._convertToWeight(result, expected), 5); }); - it('returns the a weight of zero if weight is missing from the expected', () => { + it('throws if weight is missing from the expected', () => { const expected = { rawValue: 100 }; @@ -187,7 +187,7 @@ describe('Aggregate', () => { displayValue: '50' }; - return assert.equal(Aggregate._convertToWeight(result, expected), 0); + return assert.throws(_ => Aggregate._convertToWeight(result, expected), 0); }); it('returns a weight of zero for other inputs', () => { @@ -296,6 +296,45 @@ describe('Aggregate', () => { }); }); + it('throws when given a result containing no score property', () => { + const items = [{ + criteria: { + test: { + rawValue: true, + weight: 1 + } + } + }]; + + const results = [{ + name: 'test', + value: 'should be rawValue', + displayValue: '' + }]; + const scored = true; + + return assert.throws(_ => Aggregate.compare(results, items, scored)); + }); + + it('throws when given a criterion containing no rawValue property', () => { + const items = [{ + criteria: { + test: { + weight: 1 + } + } + }]; + + const results = [{ + name: 'test', + score: false, + displayValue: '' + }]; + const scored = true; + + return assert.throws(_ => Aggregate.compare(results, items, scored)); + }); + it('filters a set correctly', () => { const items = [{ criteria: {