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

Bump Circle CI docker image #18914

Merged
merged 17 commits into from
May 2, 2023
Merged

Bump Circle CI docker image #18914

merged 17 commits into from
May 2, 2023

Conversation

FrederikBolding
Copy link
Member

@FrederikBolding FrederikBolding commented May 2, 2023

Explanation

Stops using deprecated circleci/node:16-browsers image and uses cimg/node:16.20-browsers instead.
https://circleci.com/developer/images/image/cimg/node

This updates the Node version we run in CI and fixes the following SES bug: nodejs/node#43496

A few other changes were made to accommodate the new Docker image:

  • Stopped using xset to check for X-server, this would cause tests to timeout
  • Added sudo apt-get update to the Chrome installation script because of missing packages required for Chrome installation
  • Adjusted Firefox binary location to opt/firefox/firefox (we might not have been running the FF version we thought before)

Don't mind the commit log 😄
image

@FrederikBolding FrederikBolding marked this pull request as ready for review May 2, 2023 14:02
@metamaskbot
Copy link
Collaborator

Builds ready [12335a6]
Page Load Metrics (1580 ± 57 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint941901162110
domContentLoaded13801842155211455
load13981842158012057
domInteractive13801842155211455
Bundle size diffs
  • background: 0 bytes
  • ui: 0 bytes
  • common: 0 bytes

process.env.SELENIUM_BROWSER === 'chrome' &&
process.env.CI === 'true'
) {
await ensureXServerIsRunning();
Copy link
Member

Choose a reason for hiding this comment

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

We had found that the e2e tests would intermittently fail unless this was running. This took a long time for us to debug. Eventually we settled on this solution after finding that the Chromium project uses a similar strategy for their own internal test suite.

I wonder why it's not required anymore... hopefully this won't bring the intermittent failures back.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure.. None of the Chrome E2E tests would start when this was being run with it. It seemed like it would never resolve 🤔

Perhaps xset is not available on the new image? It's curious that it didn't throw though.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry I'm getting mixed up; this wasn't related to the e2e test failures I was thinking of. I went back to check the logs. I think we hypothesized that some recurring failures were due to this, but that was never found to be the case.

I used X server to get the e2e tests running on GitHub Actions in an experimental PR I was working on on-and-off for months. Not an intermittent failure issue, just basic compatibility. That's what I remember this being critical for. That PR was closed, no part of that landed in develop.

It looks like this script was added to make the tests run correctly locally on M1 macbooks. Same solution but a different problem than I was thinking of. I'll test this PR locally on a macbook.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is only enabled in CI though 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Ah, interesting. That condition was not present in the original PR where this was introduced.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would say we remove it for now and we can investigate further if it turns out to introduce intermittent test failures?

Copy link
Member

Choose a reason for hiding this comment

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

Disabled locally here: #12043
Weird 🤷 I guess we can assume it's fine to omit it then.

Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

@FrederikBolding FrederikBolding merged commit 55d974d into develop May 2, 2023
@FrederikBolding FrederikBolding deleted the fb/bump-ci-image branch May 2, 2023 15:10
@github-actions github-actions bot locked and limited conversation to collaborators May 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants