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

chore(dep): upgrade to puppeteer 13.1.2 #740

Merged
merged 26 commits into from
Jan 25, 2022
Merged

chore(dep): upgrade to puppeteer 13.1.2 #740

merged 26 commits into from
Jan 25, 2022

Conversation

connorjclark
Copy link
Collaborator

No description provided.

@connorjclark
Copy link
Collaborator Author

connorjclark commented Jan 15, 2022

Screenshots used during testing can be 1+ MB, which trips up jest-image-snapshot running the diffing in a child process. The buffer size it uses is node's default of ~200KB. runDiffImageToSnapshot is what throws in node_modules/@storybook/addon-storyshots-puppeteer/node_modules/jest-image-snapshot/src/diff-snapshot.js.

Workaround is to use runInProcess: true. Except... although we use that package directly and can configure runInProcess: true, storybook also uses it... luckily it provides a way to configure the options to jest-image-snapshot (getMatchOptions)

ref americanexpress/jest-image-snapshot#210

@@ -60,7 +60,7 @@ function cleanStdOutput(output) {
.replace(/(\s+at \S+) .*/g, '$1')
.replace(/\s+at (async|processTicksAndRejections|process._tickCallback)(?=\n|$)/g, '')
.replace(
/appspot.com\/reports\/[0-9-]+.report.html/,
/appspot.com\/reports\/[0-9-]+.report.html/g,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

FAIL packages/cli/test/autorun-github.test.js (51.372s)
  ● Lighthouse CI autorun CLI with GitHub status check › should submit status check to GitHub

    expect(received).toMatchInlineSnapshot(snapshot)

    Snapshot name: `Lighthouse CI autorun CLI with GitHub status check should submit status check to GitHub 1`

    - Snapshot  - 1
    + Received  + 1

    @@ -16,11 +16,11 @@


      Uploading median LHR of http://localhost:XXXX/bad.html...success!
      Open the report at storage.googleapis.com/lighthouse-infrastructure.appspot.com/reports/XXXX-XXXX.report.html
      Uploading median LHR of http://localhost:XXXX/good.html...success!
    - Open the report at storage.googleapis.com/lighthouse-infrastructure.appspot.com/reports/XXXX-XXXX.report.html
    + Open the report at storage.googleapis.com/lighthouse-infrastructure.appspot.com/reports/XXXX-113.report.html
      GitHub token found, attempting to set status...
      GitHub accepted "failure" status for "lhci/url/bad.html".
      GitHub accepted "success" status for "lhci/url/good.html".

      ↵

      70 |     );
      71 | 
    > 72 |     expect(stdout).toMatchInlineSnapshot(`
         |                    ^
      73 |       "✅  .lighthouseci/ directory writable
      74 |       ✅  Configuration file found
      75 |       ✅  Chrome installation found

      at Object.<anonymous> (packages/cli/test/autorun-github.test.js:72:20)

 › 1 snapshot failed.

package.json Outdated Show resolved Hide resolved
@connorjclark
Copy link
Collaborator Author

connorjclark commented Jan 18, 2022

seems storybook (but only on mac?? these tests only run on mac, filtered in packages/server/test/ui/storybook.test.jsx) is not including packages/server/src/ui/routes/build-view/build-hash-selector.css / other css files...

@connorjclark
Copy link
Collaborator Author

newest chrome resulted in tons of minute text rendering diffs. can't even distinguish them myself but apparently the pixels are different. hence, updating tons of snapshots.

await page.waitFor(parameters.waitFor);
}
// wait for the webfont request to avoid flakiness with webfont display
await page.waitForNetworkIdle();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

seems we should just always wait.

@connorjclark connorjclark changed the title chore(dep): upgrade to puppeteer 13 chore(dep): upgrade to puppeteer 13.1.1 Jan 19, 2022
@connorjclark
Copy link
Collaborator Author

RE: #740 (comment)

Not sure why, but with the newest storybook this isn't an issue anymore.

Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

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

Workaround is to use runInProcess: true

looks you managed to drop this after the storybook upgrades? that's good news.

body {
min-height: 100vh;
} */

@import 'https://fonts.googleapis.com/css?family=Roboto:400,500&display=block';
Copy link
Member

Choose a reason for hiding this comment

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

i think these lines can also be removed now (but not 100% sure)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, this is the only way that storybook can have these fonts. The only other place they are added is to the app entry point, which storybook does not use.

Copy link
Member

Choose a reason for hiding this comment

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

Oh gotcha

@connorjclark connorjclark changed the title chore(dep): upgrade to puppeteer 13.1.1 chore(dep): upgrade to puppeteer 13.1.2 Jan 25, 2022
@connorjclark connorjclark merged commit 2ca0b86 into main Jan 25, 2022
@connorjclark connorjclark deleted the puppeteer-13 branch January 25, 2022 23:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants