tests: add heading key tests #10746

patrickhulce · 2020-05-11T15:42:27Z

Summary
Adds a basic test to ensure all the keys that the headings are referring to a key that actually exists in items. I think this is safe, but there might be legit cases where we conditionally define something so I've left it enabled only in test-like scenarios for now.

Related Issues/PRs
would have caught #10743

patrickhulce · 2020-05-11T15:42:55Z

lighthouse-core/audits/audit.js

+    // 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;


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 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.

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.

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)

patrickhulce · 2020-05-11T15:43:25Z

lighthouse-core/audits/byte-efficiency/duplicated-javascript.js

@@ -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)},


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

brendankenny · 2020-05-11T16:14:51Z

lighthouse-core/audits/audit.js

@@ -81,6 +81,26 @@ class Audit {
    return clampTo2Decimals(percentile);
  }

+  /**
+   * @param {LH.Audit.Details.Table['headings']|LH.Audit.Details.Opportunity['headings']} headings


:/

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

brendankenny · 2020-06-15T22:21:08Z

After discussion today:

still bummed about test-only assertions in main code, typescript is really close to being able to catch this, but let's move on :)
if we're going to have these, it would be nice to have it as a single inTest (or whatever) function rather than a proliferation of variations on typeof describe === 'undefined' && !process.env.CI. Provides a simple string for searching (and "Find all references") as well.
process.env.CI means that it won't be caught in local testing, but even setting a LIGHTHOUSE_UNDER_TEST variable for yarn unit:core in package.json means using yarn jest my-audit won't run it either. Probably not much to do there.

paulirish · 2020-06-15T22:27:28Z

process.env.CI means that it won't be caught in local testing, but even setting a LIGHTHOUSE_UNDER_TEST variable for yarn unit:core in package.json means using yarn jest my-audit won't run it either. Probably not much to do there.

wow did not expect this but...

Jest will set process.env.NODE_ENV to 'test' if it's not set to something else.

(from https://jestjs.io/docs/en/getting-started#using-babel under Making your Babel config jest-aware)

tested it out and indeed that's the case.

handy

patrickhulce · 2020-06-16T03:14:42Z

lighthouse-core/lib/lh-env.js

+ */
+'use strict';
+
+module.exports = {


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

@brendankenny does this work for you?

naming this file anything you'd like

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

brendankenny

LGTM, just the jsdoc and CI question

brendankenny · 2020-06-19T20:45:50Z

lighthouse-core/lib/lh-env.js

+ */
+'use strict';
+
+module.exports = {


naming this file anything you'd like

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

brendankenny · 2020-06-19T20:46:16Z

lighthouse-core/lib/lh-env.js

+ */
+'use strict';
+
+module.exports = {


brendankenny · 2020-06-19T20:46:40Z

lighthouse-core/lib/lh-env.js

+module.exports = {
+  // 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',


!!process.env.CI

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

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?

We do and this does.

oh, lol, ||. Ignore me :)

brendankenny · 2020-06-19T20:47:15Z

lighthouse-core/audits/audit.js

@@ -81,6 +81,26 @@ class Audit {
    return clampTo2Decimals(percentile);
  }

+  /**
+   * @param {LH.Audit.Details.Table['headings']|LH.Audit.Details.Opportunity['headings']} headings


:/

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

lighthouse-core/audits/audit.js

tests: add heading key tests

1c9a0a1

patrickhulce requested a review from a team as a code owner May 11, 2020 15:42

patrickhulce requested review from Beytoven and removed request for a team May 11, 2020 15:42

googlebot added the cla: yes label May 11, 2020

patrickhulce commented May 11, 2020

View reviewed changes

brendankenny reviewed May 11, 2020

View reviewed changes

patrickhulce requested a review from connorjclark May 11, 2020 16:20

connorjclark removed their request for review May 26, 2020 23:01

devtools-bot assigned Beytoven May 26, 2020

devtools-bot added the waiting4reviewer label May 26, 2020

connorjclark mentioned this pull request Jun 12, 2020

misc: use null key instead of underscore #10955

Closed

patrickhulce added 2 commits June 15, 2020 22:08

Merge branch 'master' into test_audit_key

ce63cf0

use a util function

7cd794c

vercel bot deployed to Preview June 16, 2020 03:13 View deployment

patrickhulce commented Jun 16, 2020

View reviewed changes

brendankenny approved these changes Jun 19, 2020

View reviewed changes

patrickhulce added 2 commits June 19, 2020 17:12

add jsdoc

8d78b10

Merge branch 'master' into test_audit_key

39f4dc8

vercel bot deployed to Preview June 19, 2020 22:13 View deployment

patrickhulce added land-when-ci-is-green and removed waiting4reviewer labels Jun 19, 2020

devtools-bot merged commit 553ca03 into master Jun 19, 2020

devtools-bot deleted the test_audit_key branch June 19, 2020 22:42

devtools-bot removed the land-when-ci-is-green label Jun 19, 2020

gMakunde pushed a commit to gMakunde/lighthouse that referenced this pull request Jul 6, 2020

tests: add heading key tests (GoogleChrome#10746)

706a7a9

gMakunde pushed a commit to gMakunde/lighthouse that referenced this pull request Aug 28, 2020

tests: add heading key tests (GoogleChrome#10746)

45ed5f6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests: add heading key tests #10746

tests: add heading key tests #10746

patrickhulce commented May 11, 2020

patrickhulce May 11, 2020

brendankenny May 11, 2020

patrickhulce May 11, 2020

brendankenny May 11, 2020

patrickhulce May 11, 2020

connorjclark May 11, 2020

brendankenny May 11, 2020

brendankenny Jun 19, 2020 •

edited

Loading

brendankenny commented Jun 15, 2020 •

edited

Loading

paulirish commented Jun 15, 2020

patrickhulce Jun 16, 2020

patrickhulce Jun 17, 2020

brendankenny Jun 19, 2020

brendankenny Jun 19, 2020

brendankenny left a comment

brendankenny Jun 19, 2020

brendankenny Jun 19, 2020

brendankenny Jun 19, 2020

patrickhulce Jun 19, 2020

brendankenny Jun 19, 2020 •

edited

Loading

brendankenny Jun 19, 2020 •

edited

Loading

tests: add heading key tests #10746

tests: add heading key tests #10746

Conversation

patrickhulce commented May 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny Jun 19, 2020 • edited Loading

Choose a reason for hiding this comment

brendankenny commented Jun 15, 2020 • edited Loading

paulirish commented Jun 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny Jun 19, 2020 • edited Loading

Choose a reason for hiding this comment

brendankenny Jun 19, 2020 • edited Loading

Choose a reason for hiding this comment

brendankenny Jun 19, 2020 •

edited

Loading

brendankenny commented Jun 15, 2020 •

edited

Loading

brendankenny Jun 19, 2020 •

edited

Loading

brendankenny Jun 19, 2020 •

edited

Loading