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

ci: fix browser-tests ubuntu 24.04 compat #2587

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

legobeat
Copy link

@legobeat legobeat commented Oct 16, 2024

Description

Fix recent failing CI browser tests

  • ci: use npm-run instead of npx to run playwright
  • ci: manually install playwright system dependencies

Context

Security Considerations

N/A

Scaling Considerations

N/A

Documentation Considerations

Give our docs folks some hints about what needs to be described to downstream users. Backwards compatibility: what happens to existing data or deployments when this code is shipped? Do we need to instruct users to do something to upgrade their saved data? If there is no upgrade path possible, how bad will that be for users?

Testing Considerations

Every PR should of course come with tests of its own functionality. What additional tests are still needed beyond those unit tests? How does this affect CI, other test automation, or the testnet?

Compatibility Considerations

N/A

Upgrade Considerations

N/A

@legobeat legobeat force-pushed the ci-fix-browser-test branch 7 times, most recently from 8e536e0 to a710787 Compare October 16, 2024 23:42
@legobeat legobeat marked this pull request as ready for review October 16, 2024 23:42
Comment on lines +32 to +39
- name: Install browser dependencies
run: |
sudo apt-get update
sudo apt-get install -y --no-install-recommends \
ca-certificates \
libatomic1 libflite1 libgles2 libharfbuzz-icu0 libhyphen0 libmanette-0.2-0 libicu74 libsecret-1-0 libxslt1.1 woff2 \
ca-certificates fonts-liberation libatk-bridge2.0-0 libatk1.0-0 libc6 libcairo2 libcups2 libdbus-1-3 libexpat1 libfontconfig1 libgbm1 libgcc1 libglib2.0-0 libgtk-3-0 libnspr4 libnss3 libpango-1.0-0 libpangocairo-1.0-0 libstdc++6 libx11-6 libx11-xcb1 libxcb1 libxcomposite1 libxcursor1 libxdamage1 libxext6 libxfixes3 libxi6 libxrandr2 libxrender1 libxss1 libxtst6 lsb-release wget xdg-utils \
libvpx9 libevent-2.1-7t64 libopus0 libgstreamer-plugins-base1.0-0 libgstreamer-gl1.0-0 libgstreamer-plugins-bad1.0-0 gstreamer1.0-libav
Copy link
Contributor

Choose a reason for hiding this comment

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

are all of these extra deps required? or can any be left out to be more minimal

Copy link
Author

Choose a reason for hiding this comment

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

They may not all be required. Most if not all were previously installed under the hood by the playwright util.

Copy link
Contributor

Choose a reason for hiding this comment

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

  • We should definitely install only the minimum needed
  • This seems like a future maintenance burden
  • Linebreaks seem inconsistent

Copy link
Contributor

@boneskull boneskull left a comment

Choose a reason for hiding this comment

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

I don't understand the motivation behind this change. Can you update the PR description?

Comment on lines +32 to +39
- name: Install browser dependencies
run: |
sudo apt-get update
sudo apt-get install -y --no-install-recommends \
ca-certificates \
libatomic1 libflite1 libgles2 libharfbuzz-icu0 libhyphen0 libmanette-0.2-0 libicu74 libsecret-1-0 libxslt1.1 woff2 \
ca-certificates fonts-liberation libatk-bridge2.0-0 libatk1.0-0 libc6 libcairo2 libcups2 libdbus-1-3 libexpat1 libfontconfig1 libgbm1 libgcc1 libglib2.0-0 libgtk-3-0 libnspr4 libnss3 libpango-1.0-0 libpangocairo-1.0-0 libstdc++6 libx11-6 libx11-xcb1 libxcb1 libxcomposite1 libxcursor1 libxdamage1 libxext6 libxfixes3 libxi6 libxrandr2 libxrender1 libxss1 libxtst6 lsb-release wget xdg-utils \
libvpx9 libevent-2.1-7t64 libopus0 libgstreamer-plugins-base1.0-0 libgstreamer-gl1.0-0 libgstreamer-plugins-bad1.0-0 gstreamer1.0-libav
Copy link
Contributor

Choose a reason for hiding this comment

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

  • We should definitely install only the minimum needed
  • This seems like a future maintenance burden
  • Linebreaks seem inconsistent

@@ -41,10 +53,10 @@ jobs:
run: npm ci --ignore-scripts
- name: Install Playwright Browsers
working-directory: browser-test
run: npx playwright install --with-deps
run: npm exec playwright install --with-deps
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't npx do the same thing if playwright is local?

@leotm
Copy link
Contributor

leotm commented Oct 29, 2024

I don't understand the motivation behind this change. Can you update the PR description?

updated ^ it's to fix the recent failing Browser Tests happening in a few PRs

@boneskull
Copy link
Contributor

Is this still a problem?

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.

3 participants