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

tests(smokehouse): add flag for test sharding #13047

Merged
merged 3 commits into from Sep 14, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 7 additions & 6 deletions .github/workflows/smoke.yml
Expand Up @@ -13,15 +13,16 @@ jobs:
strategy:
matrix:
chrome-channel: ['stable', 'ToT']
smoke-test-invert: [false, true]
smoke-test-shard: [1, 2]
# e.g. if set 1 fails, continue with set 2 anyway
fail-fast: false
runs-on: ubuntu-latest
env:
# The smokehouse tests run by job `test set 1`. `test set 2` will run the rest.
SMOKE_BATCH_1: a11y oopif pwa dbw redirects errors offline
# Job named e.g. "Chrome stable, test set 1".
name: Chrome ${{ matrix.chrome-channel }}, batch ${{ matrix.smoke-test-invert == false && '1' || '2' }}
# The total number of shards. Set dynamically when length of single matrix variable is
# computable. See https://github.community/t/get-length-of-strategy-matrix-or-get-all-matrix-options/18342
SHARD_TOTAL: 2
Comment on lines +21 to +23
Copy link
Member Author

Choose a reason for hiding this comment

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

this is unfortunately really annoying. You can get the length of the entire matrix with ${{ strategy.job-index }}, but that's 4 here, not 2 (since it's chrome-channel × smoke-test-shard).

There are also ways you can do fancy things with | jq '. | length', but you need a reference to the whole array first, so you need it to be the output of another job (can't do it with env because they aren't available inside matrix...), at which point it all becomes much more verbose than just having a 2 here.

So... :/

# Job named e.g. "Chrome stable, batch 1".
name: Chrome ${{ matrix.chrome-channel }}, batch ${{ matrix.smoke-test-shard }}

steps:
- name: git clone
Expand Down Expand Up @@ -52,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 --invert-match ${{ matrix.smoke-test-invert }} $SMOKE_BATCH_1
xvfb-run --auto-servernum yarn c8 yarn smoke --debug -j=1 --retries=2 --shard=${{ matrix.smoke-test-shard }}/$SHARD_TOTAL
yarn c8 report --reporter text-lcov > smoke-coverage.lcov

- name: Upload test coverage to Codecov
Expand Down
55 changes: 54 additions & 1 deletion lighthouse-cli/test/smokehouse/frontends/smokehouse-bin.js
Expand Up @@ -13,6 +13,7 @@

/* eslint-disable no-console */

import {strict as assert} from 'assert';
import path from 'path';
import fs from 'fs';
import url from 'url';
Expand Down Expand Up @@ -79,6 +80,52 @@ function getDefinitionsToRun(allTestDefns, requestedIds, {invertMatch}) {
return smokes;
}

/**
* Parses the cli `shardArg` flag into `shardNumber/shardTotal`. Splits
* `testDefns` into `shardTotal` shards and returns the `shardNumber`th shard.
* Shards will differ in size by at most 1.
* Shard params must be 1 ≤ shardNumber ≤ shardTotal.
* @param {Array<Smokehouse.TestDfn>} testDefns
* @param {string=} shardArg
* @return {Array<Smokehouse.TestDfn>}
*/
function getShardedDefinitions(testDefns, shardArg) {
if (!shardArg) return testDefns;

// eslint-disable-next-line max-len
const errorMessage = `'shard' must be of the form 'n/d' and n and d must be positive integers with 1 ≤ n ≤ d. Got '${shardArg}'`;
const match = /^(?<shardNumber>\d+)\/(?<shardTotal>\d+)$/.exec(shardArg);
assert(match && match.groups, errorMessage);
const shardNumber = Number(match.groups.shardNumber);
const shardTotal = Number(match.groups.shardTotal);
assert(shardNumber > 0 && Number.isInteger(shardNumber), errorMessage);
assert(shardTotal > 0 && Number.isInteger(shardTotal));
assert(shardNumber <= shardTotal, errorMessage);

// Array is sharded with `Math.ceil(length / shardTotal)` shards first
// and then the remaining `Math.floor(length / shardTotal) shards.
// e.g. `[0, 1, 2, 3]` split into 3 shards is `[[0, 1], [2], [3]]`.
const baseSize = Math.floor(testDefns.length / shardTotal);
const biggerSize = baseSize + 1;
const biggerShardCount = testDefns.length % shardTotal;

// Since we don't have tests for this file, construct all shards so correct
// structure can be asserted.
const shards = [];
let index = 0;
for (let i = 0; i < shardTotal; i++) {
const shardSize = i < biggerShardCount ? biggerSize : baseSize;
shards.push(testDefns.slice(index, index + shardSize));
index += shardSize;
}
brendankenny marked this conversation as resolved.
Show resolved Hide resolved
assert.equal(shards.length, shardTotal);
assert.deepEqual(shards.flat(), testDefns);

const shardDefns = shards[shardNumber - 1];
console.log(`In this shard (${shardArg}), running: ${shardDefns.map(d => d.id).join(' ')}\n`);
return shardDefns;
}

/**
* Prune the `networkRequests` from the test expectations when `takeNetworkRequestUrls`
* is not defined. Custom servers may not have this method available in-process.
Expand Down Expand Up @@ -163,6 +210,11 @@ async function begin() {
default: false,
describe: 'Run all available tests except the ones provided',
},
'shard': {
type: 'string',
// eslint-disable-next-line max-len
describe: 'A argument of the form "n/d", which divides the selected tests into d groups and runs the nth group. n and d must be positive integers with 1 ≤ n ≤ d.',
},
})
.wrap(y.terminalWidth())
.argv;
Expand All @@ -187,7 +239,8 @@ async function begin() {
const {default: rawTestDefns} = await import(url.pathToFileURL(testDefnPath).href);
const allTestDefns = updateTestDefnFormat(rawTestDefns);
const invertMatch = argv.invertMatch;
const testDefns = getDefinitionsToRun(allTestDefns, requestedTestIds, {invertMatch});
const requestedTestDefns = getDefinitionsToRun(allTestDefns, requestedTestIds, {invertMatch});
const testDefns = getShardedDefinitions(requestedTestDefns, argv.shard);

let smokehouseResult;
let server;
Expand Down