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

Jump to bottom

tests: run smoke tests in parallel jobs #10993

Closed
wants to merge 2 commits into from
Closed

tests: run smoke tests in parallel jobs #10993

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6c8bf7e
tests: run smoke tests in parallel jobs
brendankenny Jun 18, 2020
c69e235
feedback and split out basics
brendankenny Jun 18, 2020
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
88 changes: 60 additions & 28 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,26 @@ name: 💡🏠
on: [pull_request]

jobs:
ci:
basics:
# basic checks that depend only on the cloned and yarned repo.
runs-on: ubuntu-latest

steps:
- name: git clone
uses: actions/checkout@v2

- name: Use Node.js 10.x
uses: actions/setup-node@v1
with:
node-version: 10.x

- run: yarn --frozen-lockfile
- run: yarn type-check
- run: yarn lint
- run: yarn i18n:checks

ci:
runs-on: ubuntu-latest
strategy:
# e.g. if lint fails, continue to the unit tests anyway
fail-fast: false

steps:
- name: git clone
Expand All @@ -23,7 +37,7 @@ jobs:
with:
node-version: 10.x

- name: Setup protoc
- name: Set up protoc
uses: arduino/setup-protoc@7ad700d
with:
version: '3.7.1'
Expand All @@ -38,39 +52,35 @@ jobs:
python -m pip install --upgrade pip
pip install protobuf==3.7.1

# Cache yarn deps. thx https://github.com/actions/cache/blob/master/examples.md#node---yarn
Copy link
Member Author

Choose a reason for hiding this comment

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

looking at the runs for https://github.com/GoogleChrome/lighthouse/pull/10988/checks?check_run_id=783384854 and running a bunch of them myself, it seems like caching only saves like 5 seconds out of ~30. Doesn't seem worth it.

- name: Get yarn cache directory path
id: yarn-cache-dir-path
run: echo "::set-output name=dir::$(yarn cache dir)"
- name: Set up node_modules cache
uses: actions/cache@v1
id: yarn-cache
with:
path: ${{ steps.yarn-cache-dir-path.outputs.dir }}
key: ${{ runner.os }}-yarn-${{ hashFiles('**/yarn.lock') }}
restore-keys: |
${{ runner.os }}-yarn-

- run: yarn --frozen-lockfile
- run: yarn build-all
- run: yarn diff:sample-json
- run: yarn type-check
- run: yarn lint
- run: yarn test-proto # Run before unit-core because the roundtrip json is needed for proto tests.

# Run tests that require headfull Chrome.
- run: sudo apt-get install xvfb
- run: xvfb-run --auto-servernum yarn unit
- run: xvfb-run --auto-servernum yarn test-clients
- run: xvfb-run --auto-servernum yarn smoke --debug -j=1 --retries=2
- run: xvfb-run --auto-servernum yarn test-bundle
- run: xvfb-run --auto-servernum yarn test-docs
- name: yarn unit
run: xvfb-run --auto-servernum yarn unit
- name: yarn test-clients
run: xvfb-run --auto-servernum yarn test-clients
- name: yarn test-bundle
run: xvfb-run --auto-servernum yarn test-bundle
- name: yarn test-docs
run: xvfb-run --auto-servernum yarn test-docs

- run: yarn test-lantern
- run: yarn test-legacy-javascript
Copy link
Collaborator

@connorjclark connorjclark Jun 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Some of these tests can take a long time. That's why I split up the "special" tests (legacy js, lantern, etc.) from the core unit test and the build job–with this PR all of these are still in the same job.

- run: yarn i18n:checks
- run: yarn dogfood-lhci

# Fail if any changes were written to source files (ex, from: build/build-cdt-lib.js).
- run: git diff --exit-code

- name: Upload dist/
Copy link
Member Author

Choose a reason for hiding this comment

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

kept from #10988

uses: actions/upload-artifact@v2
with:
name: dist
path: dist/

# buildtracker runs `git merge-base HEAD origin/master` which needs more history than depth=1. https://github.com/paularmstrong/build-tracker/issues/106
- name: Deepen git fetch (for buildtracker)
run: git fetch --deepen=100
Expand All @@ -81,5 +91,27 @@ jobs:
# https://buildtracker.dev/docs/guides/github-actions#configuration
BT_API_AUTH_TOKEN: ${{ secrets.BT_API_AUTH_TOKEN }}

# Fail if any changes were written to source files (ex, from: build/build-cdt-lib.js).
- run: git diff --exit-code
smoke:
runs-on: ubuntu-latest
strategy:
matrix:
smokeTestInvert: [false, true]
Copy link
Collaborator

Choose a reason for hiding this comment

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

yaml keys are normally skewer-case.

# e.g. if smoke 0 fails, continue with smoke 1 anyway
fail-fast: false
env:
# The smokehouse tests run by job smoke 0. smoke-1 will run the rest.
Copy link
Member

Choose a reason for hiding this comment

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

whatcha thinking with ToTChrome in the mix too?

Copy link
Member Author

@brendankenny brendankenny Jun 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

whatcha thinking with ToTChrome in the mix too?

matrix:
  smokeTestInvert: [false, true]
  chrome: [Stable, ToT]
name: smoke ${{ strategy.job-index }} (Chrome ${{ matrix.chrome }})

and then conditionally set CHROME_PATH? conditionals are awkward but that doesn't seem too bad

Copy link
Collaborator

@connorjclark connorjclark Jun 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I broke up the ToT and parallel job PRs for a reason. Things are getting complex :)

SMOKE_GROUP_1: a11y oopif pwa pwa2 pwa3 dbw redirects errors offline
name: smoke ${{ strategy.job-index }}

steps:
- name: git clone
uses: actions/checkout@v2

- name: Use Node.js 10.x
uses: actions/setup-node@v1
with:
node-version: 10.x

- run: yarn --frozen-lockfile
- name: Run smoke tests
run: xvfb-run --auto-servernum yarn smoke --debug -j=1 --retries=2 --invert-match ${{ matrix.smokeTestInvert }} $SMOKE_GROUP_1
18 changes: 13 additions & 5 deletions lighthouse-cli/test/smokehouse/frontends/smokehouse-bin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,18 +34,19 @@ const runnerPaths = {
* Determine batches of smoketests to run, based on the `requestedIds`.
* @param {Array<Smokehouse.TestDfn>} allTestDefns
* @param {Array<string>} requestedIds
* @param {{invertMatch: boolean}} options
* @return {Array<Smokehouse.TestDfn>}
*/
function getDefinitionsToRun(allTestDefns, requestedIds) {
function getDefinitionsToRun(allTestDefns, requestedIds, {invertMatch}) {
let smokes = [];
const usage = ` ${log.dim}yarn smoke ${allTestDefns.map(t => t.id).join(' ')}${log.reset}\n`;

if (requestedIds.length === 0) {
if (requestedIds.length === 0 && !invertMatch) {
smokes = [...allTestDefns];
console.log('Running ALL smoketests. Equivalent to:');
console.log(usage);
} else {
smokes = allTestDefns.filter(test => requestedIds.includes(test.id));
smokes = allTestDefns.filter(test => invertMatch !== requestedIds.includes(test.id));
console.log(`Running ONLY smoketests for: ${smokes.map(t => t.id).join(' ')}\n`);
}

Expand All @@ -57,6 +58,10 @@ function getDefinitionsToRun(allTestDefns, requestedIds) {
console.log(usage);
}

if (!smokes.length) {
throw new Error('no smoketest found to run');
}

return smokes;
}

Expand All @@ -68,14 +73,16 @@ async function begin() {
.help('help')
.usage('node $0 [<options>] <test-ids>')
.example('node $0 -j=1 pwa seo', 'run pwa and seo tests serially')
.example('node $0 --invert-match byte', 'run all smoke tests but `byte`')
.describe({
'debug': 'Save test artifacts and output verbose logs',
'jobs': 'Manually set the number of jobs to run at once. `1` runs all tests serially',
'retries': 'The number of times to retry failing tests before accepting. Defaults to 0',
'runner': 'The method of running Lighthouse',
'tests-path': 'The path to a set of test definitions to run. Defaults to core smoke tests.',
'invert-match': 'Run all available tests except the ones provided',
})
.boolean(['debug'])
.boolean(['debug', 'invert-match'])
.alias({
'jobs': 'j',
})
Expand All @@ -99,7 +106,8 @@ async function begin() {
testDefnPath = path.resolve(process.cwd(), testDefnPath);
const requestedTestIds = argv._;
const allTestDefns = require(testDefnPath);
const testDefns = getDefinitionsToRun(allTestDefns, requestedTestIds);
const invertMatch = argv.invertMatch;
const testDefns = getDefinitionsToRun(allTestDefns, requestedTestIds, {invertMatch});

const options = {jobs, retries, isDebug: argv.debug, lighthouseRunner};

Expand Down
3 changes: 2 additions & 1 deletion lighthouse-cli/test/smokehouse/lighthouse-runners/bundle.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@ const ChromeLauncher = require('chrome-launcher');
const ChromeProtocol = require('../../../../lighthouse-core/gather/connections/cri.js');

// Load bundle, which creates a `global.runBundledLighthouse`.
require('../../../../dist/lighthouse-dt-bundle.js');
// @ts-ignore - file won't exist until `yarn build-all`, but not used for types anyways.
require('../../../../dist/lighthouse-dt-bundle.js'); // eslint-disable-line

/** @type {import('../../../../lighthouse-core/index.js')} */
// @ts-ignore - not worth giving test global an actual type.
Expand Down