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.

Sign up for GitHub

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

tests: add heading key tests #10746

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1c9a0a1
tests: add heading key tests
patrickhulce May 11, 2020
ce63cf0
Merge branch 'master' into test_audit_key
patrickhulce Jun 16, 2020
7cd794c
use a util function
patrickhulce Jun 16, 2020
8d78b10
add jsdoc
patrickhulce Jun 19, 2020
39f4dc8
Merge branch 'master' into test_audit_key
patrickhulce Jun 19, 2020
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
24 changes: 24 additions & 0 deletions lighthouse-core/audits/audit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,26 @@ class Audit {
return clampTo2Decimals(percentile);
}

/**
patrickhulce marked this conversation as resolved.
Show resolved Hide resolved
* @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 •
edited
Loading

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 (typeof describe === 'undefined' && !process.env.CI) return;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't really like this. I'd prefer to do something like process.env.LIGHTHOUSE_UNDER_TEST like I've done in other projects if others are open to it?

Copy link
Member

Choose a reason for hiding this comment

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

I don't really like this. I'd prefer to do something like process.env.LIGHTHOUSE_UNDER_TEST like I've done in other projects if others are open to it?

I'm not wild about setting a precedent and seeing that spread, though :) And it's kind of a bummer to only catch it in CI and not through local testing (I guess yarn unit:core could set the flag but yarn jest my-audit wouldn't).

I think it's probably good without the CI check, actually. It might be different if we were constructing table columns dynamically, but AFAIK all headers are hardcoded in their audits (and our general design approach and i18n means we'll likely continue doing that), so if a LH dev runs their audit and gets any kind of table output from it (unit, smoke, or sample_v2) it should hit this and get corrected before it could ever throw for an end user.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it's probably good without the CI check, actually

Are you suggesting removing the && !process.env.CI bit or the entire if?

I'm a little hesitant for removing the entire if if we currently have cases where rows are conditionally one type or another and a key might be validly missing in some situations. I don't think that's the case, but with 100+ audits I'm not sure and the failure mode is tanking the entire audit's results.

Copy link
Member

Choose a reason for hiding this comment

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

I'm a little hesitant for removing the entire if if we currently have cases where rows are conditionally one type or another and a key might be validly missing in some situations. I don't think that's the case, but with 100+ audits I'm not sure and the failure mode is tanking the entire audit's results.

Yeah, I didn't think about the fact that a row value can be set to undefined to have an empty cell, and it's perfectly possible for a table to end up with rows that all happen to have an empty cell there. That would happen with performance-budgets, for instance, if either the number of requests or size went over budget but not both, but it would pass this test because they're explicitly set to undefined.

Since in theory this assertion will force audit authors to set explicit undefined to avoid false negatives anyways (if it's possible to have a table like that you should probably have a test for it, at which point you have to set an explicit undefined so this check passes in CI) it would be better to just be a requirement, rather than a requirement for any code paths that happen to run in tests.

If the blocker is going through the existing audits with tables, that's not insurmountable. We could divide them up pretty easily (I see 40 makeTableDetails and 5 makeOpportunityDetails calls)


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 +117,8 @@ class Audit {
};
}

Audit.assertHeadingKeysExist(headings, results);

return {
type: 'table',
headings: headings,
Expand Down Expand Up @@ -179,6 +201,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
2 changes: 1 addition & 1 deletion lighthouse-core/audits/byte-efficiency/duplicated-javascript.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ class DuplicatedJavascript extends ByteEfficiencyAudit {
const headings = [
/* eslint-disable max-len */
{key: 'source', valueType: 'code', subRows: {key: 'urls', valueType: 'url'}, label: str_(i18n.UIStrings.columnSource)},
{key: '_', valueType: 'bytes', subRows: {key: 'sourceBytes'}, granularity: 0.05, label: str_(i18n.UIStrings.columnSize)},
{key: null, valueType: 'bytes', subRows: {key: 'sourceBytes'}, granularity: 0.05, label: str_(i18n.UIStrings.columnSize)},
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is the only other fix it found once #10743 landed

Copy link
Collaborator

Choose a reason for hiding this comment

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

yikes

{key: 'wastedBytes', valueType: 'bytes', granularity: 0.05, label: str_(i18n.UIStrings.columnWastedBytes)},
/* eslint-enable max-len */
];
Expand Down
4 changes: 1 addition & 3 deletions lighthouse-core/test/audits/byte-efficiency/byte-efficiency-audit-test.js
View file
Open in desktop
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