tests: cron to check for relevant chromium changes #11763

paulirish · 2020-12-04T08:15:36Z

We have two efforts where we have some risk if we fall behind Chromium.

if devtools protocol adds new Issues types to the issues panel. aka Handle the unknown inspector issues #11530
if the PWA installable_manager adds new error codes that we don't have installable-manifest strings for. (ref core(installable-manifest): use devtools InstallabilityErrors #11745)

i've been ruminating on some kinda test for a while and got these in a decent state.

to reviewers: the installability errors one is fairly grody, but it holds up fine and I don't think its worth it to parse the C++ into an AST or anything...

Setting on draft until #11745 lands and things quiet down a bit. no rush on this.

connorjclark

connorjclark · 2020-12-04T14:08:59Z

.github/workflows/cron-weekly.yml

@@ -24,3 +24,18 @@ jobs:
          use-verbose-mode: 'yes'


tying all these workflows to the same exact schedule seems brittle to me. Can we keep these as separate files?

isn't this the only workflow on a schedule? consolidating to our "weekly" checks seems like a positive IMO

not anymore https://github.com/GoogleChrome/lighthouse/blob/master/.github/workflows/publish.yml#L4

connorjclark · 2020-12-04T14:17:34Z

.github/workflows/cron-weekly.yml

+        node-version: 12.x
+    - run: yarn --frozen-lockfile
+
+    - name: yarn jest .github/test/*-uptodate-test.js


I think run: is just fine here.

connorjclark · 2020-12-04T14:19:10Z

.github/test/inspector-issues-details-uptodate-test.js

+
+  it('are each handled explicitly in the gatherer', () => {
+    for (const inspectorIssueDetailsType of inspectorIssueDetailsTypes) {
+      expect(inspectorIssuesGathererSource.includes(inspectorIssueDetailsType)).toBeTruthy();


I'd suggest refactoring this gatherer to use (and export) a list of supported detail types (and then create the artifacts based on that list, instead of copying the same code for each case), but that approach was decided against in code review. this is kinda awk but no more than C++ regexes so WFM...

Suggested change

expect(inspectorIssuesGathererSource.includes(inspectorIssueDetailsType)).toBeTruthy();

expect(inspectorIssuesGathererSource).toContain(inspectorIssueDetailsType);

will print out the contents if it can't find it :)

Considering it's the entire source file, maybe not what we want :)

why not....

expect(false).toBeTruthy() doesn't seem better 😆

Because the info you want from this test is "some value is missing". some useful info might be what the value is. but def dont want 100+ lines of code printed.

wb if (!inspectorIssuesGathererSource.includes(inspectorIssueDetailsType)) assert.fail('missing issue type ' + inspectorIssueDetailsType)

Because the info you want from this test is "some value is missing". some useful info might be what the value is.

you also get this information front and center with toContain, fwiw. for some logs that we don't really expect to fail that often ~80 lines of extra output doesn't seem that bad 🤷 though I must say I'm surprised coming from the same advocate of permanent verbose output in our smokes ;)

but ya I like your new suggestion too if your original proposal doesn't fly 👍

patrickhulce

I like it 💯

.github/test/inspector-issues-details-uptodate-test.js

+ */
+'use strict';
+
+const assert = require('assert').strict;


patrickhulce · 2020-12-04T16:56:06Z

.github/test/inspector-issues-details-uptodate-test.js

+
+  it('are each handled explicitly in the gatherer', () => {
+    for (const inspectorIssueDetailsType of inspectorIssueDetailsTypes) {
+      expect(inspectorIssuesGathererSource.includes(inspectorIssueDetailsType)).toBeTruthy();


Suggested change

expect(inspectorIssuesGathererSource.includes(inspectorIssueDetailsType)).toBeTruthy();

expect(inspectorIssuesGathererSource).toContain(inspectorIssueDetailsType);

will print out the contents if it can't find it :)

patrickhulce · 2020-12-04T16:59:44Z

.github/test/installability-errors-uptodate-test.js

+  });
+
+  it('should notify us if something changed', () => {
+    expect(chromiumErrorIds.sort()).toMatchInlineSnapshot(`


sort it before? feels weird to mutate in a single it :)

patrickhulce · 2020-12-04T17:00:57Z

.github/test/installability-errors-uptodate-test.js

+  });
+
+  it('are each handled explicitly in the gatherer', () => {
+    // expect.arrayContaining is semantically great, but the output sucks


or...

Suggested change

// expect.arrayContaining is semantically great, but the output sucks

const errorStrings = Object.keys(InstallableManifestAudit.UIStrings)

.filter(key => chromiumErrorIds.includes(key))

.sort();

expect(errorStrings).toEqual(chromiumErrorIds);

for a nice one-shot output that lists all the missing ones? :)

patrickhulce · 2020-12-04T17:02:27Z

.github/workflows/cron-weekly.yml

@@ -24,3 +24,18 @@ jobs:
          use-verbose-mode: 'yes'


isn't this the only workflow on a schedule? consolidating to our "weekly" checks seems like a positive IMO

patrickhulce · 2020-12-04T17:03:36Z

.github/test/inspector-issues-details-uptodate-test.js

@@ -0,0 +1,47 @@
+/**


feels weird that these live in github though, aren't they effectively checks against our implementations and the github part is just an artifact of how frequently we want to run them?

connorjclark · 2020-12-17T19:46:36Z

Still draft for a reason?

patrickhulce

LGTM, I could go either way on the weekly thing so up to you and @connorjclark 👍

paulirish · 2021-01-12T23:35:28Z

LGTM, I could go either way on the weekly thing so up to you and @connorjclark 👍

@connorjclark pr over to you

third-party/chromium-synchronization/inspector-issueAdded-types-test.js

connorjclark · 2021-01-12T23:46:08Z

update with master + can you make sure it is still passing when run?

…s-test.js Co-authored-by: Connor Clark <cjamcl@google.com>

google-cla · 2021-03-23T19:57:17Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

paulirish requested a review from a team as a code owner December 4, 2020 08:15

paulirish requested review from patrickhulce and removed request for a team December 4, 2020 08:15

google-cla bot added the cla: yes label Dec 4, 2020

paulirish marked this pull request as draft December 4, 2020 08:15

devtools-bot assigned patrickhulce Dec 4, 2020

devtools-bot added the waiting4reviewer label Dec 4, 2020

vercel bot deployed to Preview December 4, 2020 08:21 View deployment

connorjclark reviewed Dec 4, 2020

View reviewed changes

patrickhulce reviewed Dec 4, 2020

View reviewed changes

patrickhulce added waiting4committer and removed waiting4reviewer labels Dec 4, 2020

Base automatically changed from dt-installable to master December 15, 2020 16:00

paulirish added 5 commits December 16, 2020 20:42

introducing tests that make sure we're up to date with chromium

955fb37

rename the cron'd gh workflow

b477234

add ci script to run these occasional tests

80da673

feedback

2a7a192

move files. update snapshots

00e3b28

paulirish force-pushed the chromium-tests-cron branch from 4311e98 to 00e3b28 Compare December 17, 2020 05:59

vercel bot deployed to Preview December 17, 2020 05:59 View deployment

move files location

9bb7251

vercel bot deployed to Preview December 17, 2020 07:43 View deployment

paulirish added waiting4reviewer and removed waiting4committer labels Dec 17, 2020

connorjclark added waiting4committer and removed waiting4reviewer labels Dec 17, 2020

paulirish added waiting4reviewer and removed waiting4committer labels Dec 22, 2020

patrickhulce approved these changes Jan 4, 2021

View reviewed changes

patrickhulce added waiting4committer and removed waiting4reviewer labels Jan 4, 2021

paulirish assigned connorjclark and unassigned patrickhulce Jan 12, 2021

paulirish added waiting4reviewer and removed waiting4committer labels Jan 12, 2021

connorjclark reviewed Jan 12, 2021

View reviewed changes

third-party/chromium-synchronization/inspector-issueAdded-types-test.js Outdated Show resolved Hide resolved

connorjclark approved these changes Jan 12, 2021

View reviewed changes

connorjclark added waiting4committer and removed waiting4reviewer labels Jan 12, 2021

Update third-party/chromium-synchronization/inspector-issueAdded-type…

9e3ab4b

…s-test.js Co-authored-by: Connor Clark <cjamcl@google.com>

vercel bot deployed to Preview March 23, 2021 19:54 View deployment

vercel bot deployed to Preview March 23, 2021 19:56 View deployment

google-cla bot added cla: no and removed cla: yes labels Mar 23, 2021

Merge remote-tracking branch 'origin' into chromium-tests-cron

524257c

paulirish force-pushed the chromium-tests-cron branch from 5eecc0a to 524257c Compare March 23, 2021 19:58

vercel bot deployed to Preview March 23, 2021 19:58 View deployment

google-cla bot added cla: yes and removed cla: no labels Mar 23, 2021

connorjclark changed the title ~~tests: add weekly cron tests to confirm we're sync'd with chromium changes~~ tests: cron to check for relevant chromium changes Apr 13, 2021

connorjclark merged commit f3bfb3c into master Apr 13, 2021

connorjclark deleted the chromium-tests-cron branch April 13, 2021 17:51

paulirish mentioned this pull request Apr 14, 2021

tests: update chromium installable source path #12364

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests: cron to check for relevant chromium changes #11763

tests: cron to check for relevant chromium changes #11763

paulirish commented Dec 4, 2020

connorjclark left a comment •

edited

Loading

connorjclark Dec 4, 2020

patrickhulce Dec 4, 2020

connorjclark Dec 17, 2020

connorjclark Dec 4, 2020

connorjclark Dec 4, 2020 •

edited

Loading

patrickhulce Dec 4, 2020

connorjclark Dec 4, 2020

patrickhulce Dec 4, 2020

connorjclark Dec 5, 2020

patrickhulce Dec 5, 2020

patrickhulce left a comment

This comment was marked as outdated.

patrickhulce Dec 4, 2020

patrickhulce Dec 4, 2020

patrickhulce Dec 4, 2020

patrickhulce Dec 4, 2020

patrickhulce Dec 4, 2020

connorjclark commented Dec 17, 2020

patrickhulce left a comment

paulirish commented Jan 12, 2021

connorjclark commented Jan 12, 2021

google-cla bot commented Mar 23, 2021

	expect(inspectorIssuesGathererSource.includes(inspectorIssueDetailsType)).toBeTruthy();
	expect(inspectorIssuesGathererSource).toContain(inspectorIssueDetailsType);

-    // expect.arrayContaining is semantically great, but the output sucks
+    const errorStrings = Object.keys(InstallableManifestAudit.UIStrings)
+      .filter(key => chromiumErrorIds.includes(key))
+      .sort();
+    expect(errorStrings).toEqual(chromiumErrorIds);

tests: cron to check for relevant chromium changes #11763

tests: cron to check for relevant chromium changes #11763

Conversation

paulirish commented Dec 4, 2020

connorjclark left a comment • edited Loading

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

connorjclark Dec 4, 2020 • edited Loading

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

patrickhulce left a comment

Choose a reason for hiding this comment

This comment was marked as outdated.

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

connorjclark commented Dec 17, 2020

patrickhulce left a comment

Choose a reason for hiding this comment

paulirish commented Jan 12, 2021

connorjclark commented Jan 12, 2021

google-cla bot commented Mar 23, 2021

connorjclark left a comment •

edited

Loading

connorjclark Dec 4, 2020 •

edited

Loading