Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tests: add heading key tests #10746

Merged
merged 5 commits into from
Jun 19, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion lighthouse-cli/test/smokehouse/lighthouse-runners/cli.js
Original file line number Diff line number Diff line change
Expand Up @@ -79,12 +79,13 @@ async function internalRun(url, tmpPath, configJson, isDebug) {
}

const command = 'node';
const env = {...process.env, NODE_ENV: 'test'};
localConsole.log(`${log.dim}$ ${command} ${args.join(' ')} ${log.reset}`);

/** @type {{stdout: string, stderr: string, code?: number}} */
let execResult;
try {
execResult = await execFileAsync(command, args);
execResult = await execFileAsync(command, args, {env});
} catch (e) {
// exec-thrown errors have stdout, stderr, and exit code from child process.
execResult = e;
Expand Down
28 changes: 28 additions & 0 deletions lighthouse-core/audits/audit.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
*/
'use strict';

const {isUnderTest} = require('../lib/lh-env.js');
const statistics = require('../lib/statistics.js');
const Util = require('../report/html/renderer/util.js');

Expand Down Expand Up @@ -81,6 +82,29 @@ class Audit {
return clampTo2Decimals(percentile);
}

/**
patrickhulce marked this conversation as resolved.
Show resolved Hide resolved
* This catches typos in the `key` property of a heading definition of table/opportunity details.
* Throws an error if any of keys referenced by headings don't exist in at least one of the items.
*
* @param {LH.Audit.Details.Table['headings']|LH.Audit.Details.Opportunity['headings']} headings
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:/

Copy link
Member

@brendankenny brendankenny Jun 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:/

face was to the param type we're stuck with forever, not this function, btw :)

* @param {LH.Audit.Details.Opportunity['items']|LH.Audit.Details.Table['items']} items
*/
static assertHeadingKeysExist(headings, items) {
// If there are no items, there's nothing to check.
if (!items.length) return;
// Only do this in tests for now.
if (!isUnderTest) return;

for (const heading of headings) {
// `null` heading key means it's a column for subrows only
if (heading.key === null) continue;

const key = heading.key;
if (items.some(item => key in item)) continue;
throw new Error(`"${heading.key}" is missing from items`);
}
}

/**
* @param {LH.Audit.Details.Table['headings']} headings
* @param {LH.Audit.Details.Table['items']} results
Expand All @@ -97,6 +121,8 @@ class Audit {
};
}

Audit.assertHeadingKeysExist(headings, results);

return {
type: 'table',
headings: headings,
Expand Down Expand Up @@ -179,6 +205,8 @@ class Audit {
* @return {LH.Audit.Details.Opportunity}
*/
static makeOpportunityDetails(headings, items, overallSavingsMs, overallSavingsBytes) {
Audit.assertHeadingKeysExist(headings, items);

return {
type: 'opportunity',
headings: items.length === 0 ? [] : headings,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,7 @@ class DuplicatedJavascript extends ByteEfficiencyAudit {
const headings = [
/* eslint-disable max-len */
{key: 'source', valueType: 'code', subItemsHeading: {key: 'url', valueType: 'url'}, label: str_(i18n.UIStrings.columnSource)},
{key: '_', valueType: 'bytes', subItemsHeading: {key: 'sourceTransferBytes'}, granularity: 0.05, label: str_(i18n.UIStrings.columnTransferSize)},
{key: null, valueType: 'bytes', subItemsHeading: {key: 'sourceTransferBytes'}, granularity: 0.05, label: str_(i18n.UIStrings.columnTransferSize)},
{key: 'wastedBytes', valueType: 'bytes', granularity: 0.05, label: str_(i18n.UIStrings.columnWastedBytes)},
/* eslint-enable max-len */
];
Expand Down
12 changes: 12 additions & 0 deletions lighthouse-core/lib/lh-env.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
/**
* @license Copyright 2020 The Lighthouse Authors. All Rights Reserved.
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
*/
'use strict';

module.exports = {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brendankenny you have the great honor of naming this file anything you'd like :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brendankenny does this work for you?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

naming this file anything you'd like

lighthouse-core/utilities/utils/utils.util.js pls

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(LGTM)

// NODE_ENV is set to test by jest and by smokehouse CLI runner
// CI as a catchall for everything we do in Travis/GitHub Actions
isUnderTest: !!process.env.CI || process.env.NODE_ENV === 'test',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

!!process.env.CI

I thought we wanted it to work in local testing too?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do and this does.

NODE_ENV is set to test by jest and by smokehouse CLI runner

Do you not like the idea that there's possibly something in CI that we test we don't catch locally without NODE_ENV?

Copy link
Member

@brendankenny brendankenny Jun 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do and this does.

oh, lol, ||. Ignore me :)

};
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,7 @@ describe('Byte efficiency base audit', () => {
});

const baseHeadings = [
{key: 'totalBytes', itemType: 'bytes', displayUnit: 'kb', granularity: 1, text: ''},
{key: 'wastedBytes', itemType: 'bytes', displayUnit: 'kb', granularity: 1, text: ''},
{key: 'wastedMs', itemType: 'text', text: ''},
];

describe('#estimateTransferSize', () => {
Expand Down Expand Up @@ -197,7 +195,7 @@ describe('Byte efficiency base audit', () => {
const simulator = await LoadSimulator.request({devtoolsLog, settings}, {computedCache});
const result = ByteEfficiencyAudit.createAuditProduct(
{
headings: [{key: 'value', text: 'Label'}],
headings: [{key: 'wastedBytes', text: 'Label'}],
items: [
{url: 'https://www.googletagmanager.com/gtm.js?id=GTM-Q5SW', wastedBytes: 30 * 1024},
],
Expand Down