misc: update stack packs, remove duplicated stack pack files #11396

connorjclark · 2020-09-08T19:24:51Z

fixes the regression b/c of underscores (see #11370 (comment))

also forgot to delete some old files in #10138.

had to do another i18n import

patrickhulce · 2020-09-08T19:45:26Z

@connorjclark should we do the revert of #11370 and reland with a test once the upstream fix happens before master drifts too far and we build on top of it? Or am I misunderstanding the impact of the bug over there? (It's my understanding that currently no stack packs will render at all)

connorjclark · 2020-09-08T19:52:29Z

@patrickhulce, I'm about to update the repo. I had to wait to get admin access. It's a quick fix.

patrickhulce · 2020-09-08T20:54:58Z

could we get a test case using inline snapshots that the stack packs match audit ids? :D

connorjclark · 2020-09-08T20:57:21Z

could we get a test case using inline snapshots that the stack packs match audit ids? :D

No, because there are plugins inside these stack packs.

I probably misunderstood, because how would inline snapshots do this? can you say more?

patrickhulce · 2020-09-08T20:57:52Z

No, because there are plugins inside these stack packs.

That's why I said "using inline snapshots" :)

Update: Ah yeah sure here's an example of what I'm saying

Example:

const nonMatchingStackPackKeys = // magic
expect(nonMatchingStackPackKeys).toMatchInlineSnapshot(`
// this list better be small and not change or we know somethings wrong :)
`)

connorjclark · 2020-09-08T21:02:24Z

ya?

patrickhulce · 2020-09-08T21:31:06Z

that feels a bit too much like a duplicate of sample JSON we won't notice 😆

I was thinkin something more like the psuedocode below

auditIds = defaultConfig.audits
unrecognizedIds = []
for stackPack in stackPacks
  for message in stackPack
    if ! auditIds.includes(message.auditId)
      unrecognizedIds.push(message.auditId)

expect(unrecognizedIds).toMatchInlineSnapshot()

connorjclark · 2020-09-08T21:33:29Z

ah ok cool. let's do both 🎉

patrickhulce · 2020-09-08T22:25:35Z

lighthouse-core/test/lib/stack-packs-test.js

+
+    expect([...unrecognizedKeys]).toMatchInlineSnapshot(`
+      Array [
+        "time-to-first-byte",


huzzah catching bugs already! I'm guessing it should be server-response-time since it was renamed from time-to-first-byte :)

cool, but punt GoogleChrome/lighthouse-stack-packs#50

edit: or maybe dont punt. idk

sure punt is fine, wasn't trying to flag this as a blocker, just celebrating the success of its implementation 🎉 :)

brendankenny · 2020-09-08T22:51:45Z

lighthouse-core/test/lib/stack-packs-test.js

+describe('lighthouse-stack-packs', () => {
+  it('snapshot all strings', () => {
+    /* eslint-disable max-len */
+    expect(lighthouseStackPacks).toMatchInlineSnapshot(`


Snapshot testing the entirety of the stack packs seems a little excessive? We don't have a snapshot test for all of axe.min.js, for instance :)

Is there an underlying purpose that can be served with something more succinct?

The test that @patrickhulce suggested does a more narrow check for a problem we've seen already.

This one is larger, a catch all for whatever strings that end up in Lighthouse. A chance for us to stop something bad from being published.

Maybe what we're looking for here then @connorjclark is a comment expressing what a reviewer should look for? Right now I'm not sure what changes to this snapshot I should expect and what changes to this would be bad (which was the same core problem in missing the - -> _ change in the sample_v2.json)

I think what @brendankenny was saying if we're trying to prevent something like "all of these strings disappeared" then we can test that with a narrower snapshot assertion than having to inspect all of these strings manually.

[rather ...] than having to inspect all of these strings manually.

But we should check these manually. The release process for this package, while still handled by Googlers, is not 100% in our direct control. It's maybe not a realistic attack vector, but a quick 5-10s glance every quarter when we update this package is not an expensive thing.

But we should check these manually. The release process for this package, while still handled by Googlers, is not 100% in our direct control. It's maybe not a realistic attack vector, but a quick 5-10s glance every quarter when we update this package is not an expensive thing.

That's not really a test (unit or otherwise), though, it's a "hey human, check all of this for...something" (which we're also doing for the same strings in en-US.json and x50 for all the other languages when the translations come in).

If this is the chief need for this check, we should be explicit about it and do a periodic static copy of lighthouse-stack-packs into third-party/. Then it's crystal clear that what really needs to happen is a code review.

That seems like overkill, though. Really we care about things like, are joomla strings returned when getStackPacks is called with joomla detected, are strings displayed by the renderer when they're in sample_v2 like they are, etc. Pushing the basic responsibilities to the separate repo seems like part of the point of having it in a separate repo.

is not 100% in our direct control. It's maybe not a realistic attack vector

and if we should be worried about this, the renderer is ok because no string gets privileged treatment, but we should really bulk up collect-strings and do some unknown checking because each string in there costs us $$ for translations :)

updated the tests

lighthouse-core/test/lib/stack-packs-test.js

adamraine

Overall LGTM

adamraine · 2020-09-11T23:35:50Z

lighthouse-core/test/lib/stack-packs-test.js

+    `);
+  });
+
+  // Keys should only be added, not removed.


Is this instructions for someone updating the snapshot or a description of the test?

for updating the snapshot. moved it down a bit.

misc: remove duplicated stack pack files

88d811c

connorjclark requested a review from a team as a code owner September 8, 2020 19:24

connorjclark requested review from adamraine and removed request for a team September 8, 2020 19:24

googlebot added the cla: yes label Sep 8, 2020

devtools-bot assigned adamraine Sep 8, 2020

devtools-bot added the waiting4reviewer label Sep 8, 2020

update

34a16b5

vercel bot temporarily deployed to Preview September 8, 2020 20:32 Inactive

update

69b1c5e

vercel bot deployed to Preview September 8, 2020 20:33 View deployment

connorjclark changed the title ~~misc: remove duplicated stack pack files~~ misc: update stack packs, remove duplicated stack pack files Sep 8, 2020

import

22e9dae

vercel bot deployed to Preview September 8, 2020 20:43 View deployment

test

8adcbb8

vercel bot deployed to Preview September 8, 2020 21:02 View deployment

lint

2b45cec

vercel bot deployed to Preview September 8, 2020 21:03 View deployment

test

62f956c

vercel bot deployed to Preview September 8, 2020 21:44 View deployment

patrickhulce reviewed Sep 8, 2020

View reviewed changes

connorjclark mentioned this pull request Sep 8, 2020

time-to-first-byte was renamed GoogleChrome/lighthouse-stack-packs#50

Closed

brendankenny reviewed Sep 8, 2020

View reviewed changes

better tests

4a36ab5

vercel bot deployed to Preview September 10, 2020 20:03 View deployment

adamraine approved these changes Sep 11, 2020

View reviewed changes

connorjclark mentioned this pull request Sep 12, 2020

core(stack-packs): add Drupal pack #10522

Merged

connorjclark added 3 commits September 14, 2020 13:32

Merge remote-tracking branch 'origin/master' into delete-packs

42bbf4c

update

4173aa7

comment

85277af

vercel bot deployed to Preview September 14, 2020 18:33 View deployment

connorjclark added the land-when-ci-is-green label Sep 14, 2020

devtools-bot merged commit 5f2a8e2 into master Sep 14, 2020

devtools-bot deleted the delete-packs branch September 14, 2020 18:42

devtools-bot removed the land-when-ci-is-green label Sep 14, 2020

brendankenny mentioned this pull request Sep 29, 2020

i18n: import #11494

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

misc: update stack packs, remove duplicated stack pack files #11396

misc: update stack packs, remove duplicated stack pack files #11396

connorjclark commented Sep 8, 2020 •

edited

Loading

patrickhulce commented Sep 8, 2020

connorjclark commented Sep 8, 2020

patrickhulce commented Sep 8, 2020

connorjclark commented Sep 8, 2020 •

edited

Loading

patrickhulce commented Sep 8, 2020 •

edited

Loading

connorjclark commented Sep 8, 2020

patrickhulce commented Sep 8, 2020

connorjclark commented Sep 8, 2020

patrickhulce Sep 8, 2020

connorjclark Sep 8, 2020 •

edited

Loading

patrickhulce Sep 9, 2020

brendankenny Sep 8, 2020 •

edited

Loading

connorjclark Sep 9, 2020

patrickhulce Sep 9, 2020 •

edited

Loading

connorjclark Sep 9, 2020 •

edited

Loading

brendankenny Sep 9, 2020

brendankenny Sep 9, 2020

connorjclark Sep 10, 2020

adamraine left a comment

adamraine Sep 11, 2020

connorjclark Sep 14, 2020

misc: update stack packs, remove duplicated stack pack files #11396

misc: update stack packs, remove duplicated stack pack files #11396

Conversation

connorjclark commented Sep 8, 2020 • edited Loading

patrickhulce commented Sep 8, 2020

connorjclark commented Sep 8, 2020

patrickhulce commented Sep 8, 2020

connorjclark commented Sep 8, 2020 • edited Loading

patrickhulce commented Sep 8, 2020 • edited Loading

connorjclark commented Sep 8, 2020

patrickhulce commented Sep 8, 2020

connorjclark commented Sep 8, 2020

Choose a reason for hiding this comment

connorjclark Sep 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny Sep 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce Sep 9, 2020 • edited Loading

Choose a reason for hiding this comment

connorjclark Sep 9, 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

adamraine left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark commented Sep 8, 2020 •

edited

Loading

connorjclark commented Sep 8, 2020 •

edited

Loading

patrickhulce commented Sep 8, 2020 •

edited

Loading

connorjclark Sep 8, 2020 •

edited

Loading

brendankenny Sep 8, 2020 •

edited

Loading

patrickhulce Sep 9, 2020 •

edited

Loading

connorjclark Sep 9, 2020 •

edited

Loading