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(unit): retry failures and upload failure artifacts #15378

Merged
merged 5 commits into from
Aug 15, 2023
Merged

tests(unit): retry failures and upload failure artifacts #15378

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b48c205
WIP: upload unit test failure artifacts
adamraine Aug 14, 2023
120dd29
fix
adamraine Aug 14, 2023
a98ba71
retries again
adamraine Aug 15, 2023
f54b888
log tries
adamraine Aug 15, 2023
6577d34
fix
adamraine Aug 15, 2023
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
7 changes: 7 additions & 0 deletions .github/workflows/unit.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,13 @@ jobs:
- name: yarn test-viewer
run: yarn build-viewer && xvfb-run --auto-servernum bash $GITHUB_WORKSPACE/.github/scripts/test-retry.sh yarn test-viewer

- name: Upload failures
if: failure()
uses: actions/upload-artifact@v1
with:
name: Unit (ubuntu; node ${{ matrix.node }})
path: .tmp/unit-failures/

# For windows, just test the potentially platform-specific code.
unit-windows:
runs-on: windows-latest
Expand Down
7 changes: 7 additions & 0 deletions core/test/scenarios/api-test-pptr.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ describe('Individual modes API', function() {
if (!result) throw new Error('Lighthouse failed to produce a result');

const {lhr, artifacts} = result;
state.saveTrace(artifacts.Trace);
expect(artifacts.URL).toEqual({
finalDisplayedUrl: `${state.serverBaseUrl}/onclick.html#done`,
});
Expand Down Expand Up @@ -133,6 +134,8 @@ describe('Individual modes API', function() {

if (!result) throw new Error('Lighthouse failed to produce a result');

state.saveTrace(result.artifacts.Trace);

expect(result.artifacts.URL).toEqual({
finalDisplayedUrl: `${serverBaseUrl}/onclick.html#done`,
});
Expand Down Expand Up @@ -165,6 +168,8 @@ describe('Individual modes API', function() {

if (!result) throw new Error('Lighthouse failed to produce a result');

state.saveTrace(result.artifacts.Trace);

const networkRequestsDetails = /** @type {LH.Audit.Details.Table} */ (
result.lhr.audits['network-requests'].details);
const networkRequests = networkRequestsDetails?.items
Expand Down Expand Up @@ -222,6 +227,7 @@ Array [
if (!result) throw new Error('Lighthouse failed to produce a result');

const {lhr, artifacts} = result;
state.saveTrace(artifacts.Trace);
expect(artifacts.URL).toEqual({
requestedUrl: url,
mainDocumentUrl: url,
Expand Down Expand Up @@ -263,6 +269,7 @@ Array [
expect(requestor).toHaveBeenCalled();

const {lhr, artifacts} = result;
state.saveTrace(artifacts.Trace);
expect(lhr.requestedUrl).toEqual(requestedUrl);
expect(lhr.finalDisplayedUrl).toEqual(mainDocumentUrl);
expect(artifacts.URL).toEqual({
Expand Down
28 changes: 28 additions & 0 deletions core/test/scenarios/pptr-test-utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,14 @@
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
*/

import fs from 'fs';

import {before, beforeEach, after, afterEach} from 'mocha';
import * as puppeteer from 'puppeteer-core';
import {getChromePath} from 'chrome-launcher';

import {Server} from '../../../cli/test/fixtures/static-server.js';
import {LH_ROOT} from '../../../root.js';

/** @typedef {InstanceType<typeof import('../../../cli/test/fixtures/static-server.js').Server>} StaticServer */

Expand All @@ -22,12 +25,17 @@ const FLAKY_AUDIT_IDS_APPLICABILITY = new Set([
'layout-shift-elements', // Depends on if the JS takes too long after input to be ignored for layout shift.
]);

const UNIT_OUTPUT_DIR = `${LH_ROOT}/.tmp/unit-failures`;

function createTestState() {
/** @param {string} name @return {any} */
const any = name => new Proxy({}, {get: () => {
throw new Error(`${name} used without invoking \`state.before\``);
}});

/** @type {LH.Trace|undefined} */
let trace;

return {
browser: /** @type {puppeteer.Browser} */ (any('browser')),
page: /** @type {puppeteer.Page} */ (any('page')),
Expand Down Expand Up @@ -68,17 +76,37 @@ function createTestState() {
});

beforeEach(async () => {
trace = undefined;
console.log('########');
this.page = await this.browser.newPage();
});

afterEach(async () => {
await this.page.close();
});

afterEach(function() {
// eslint-disable-next-line no-invalid-this
const currentTest = this.currentTest;
if (currentTest?.state === 'failed' && trace) {
const dirname = currentTest.fullTitle().replace(/[^a-z0-9]/gi, '_').toLowerCase();
const testOutputDir = `${UNIT_OUTPUT_DIR}/${dirname}`;
fs.mkdirSync(testOutputDir, {recursive: true});
fs.writeFileSync(`${testOutputDir}/trace.json`, JSON.stringify(trace, null, 2));
}
});

after(async () => {
await this.browser.close();
});
},

/**
* @param {LH.Trace} testTrace
*/
saveTrace(testTrace) {
trace = testTrace;
},
};
}

Expand Down
2 changes: 2 additions & 0 deletions core/test/scenarios/start-end-navigation-test-pptr.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ describe('Start/End navigation', function() {
const lhr = flowResult.steps[0].lhr;
const artifacts = flowArtifacts.gatherSteps[0].artifacts;

state.saveTrace(artifacts.Trace);

expect(artifacts.URL).toEqual({
requestedUrl: `${state.serverBaseUrl}/?redirect=/index.html`,
mainDocumentUrl: `${state.serverBaseUrl}/index.html`,
Expand Down
14 changes: 14 additions & 0 deletions core/test/scripts/run-mocha-tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,14 @@ const rawArgv = y
'require': {
type: 'string',
},
'retries': {
type: 'number',
default: process.env.CI ? 5 : undefined,
},
'forbidOnly': {
type: 'boolean',
default: Boolean(process.env.CI),
},
})
.wrap(y.terminalWidth())
.argv;
Expand Down Expand Up @@ -257,8 +265,10 @@ function exit({numberFailures, numberMochaInvocations}) {
* @typedef OurMochaArgs
* @property {RegExp | string | undefined} grep
* @property {boolean} bail
* @property {boolean} forbidOnly
* @property {boolean} parallel
* @property {string | undefined} require
* @property {number | undefined} retries
*/

/**
Expand All @@ -278,9 +288,11 @@ async function runMocha(tests, mochaArgs, invocationNumber) {
timeout: 20_000,
bail: mochaArgs.bail,
grep: mochaArgs.grep,
forbidOnly: mochaArgs.forbidOnly,
// TODO: not working
// parallel: tests.length > 1 && mochaArgs.parallel,
parallel: false,
retries: mochaArgs.retries,
});

// @ts-expect-error - not in types.
Expand Down Expand Up @@ -325,6 +337,8 @@ async function main() {
bail: argv.bail,
parallel: argv.parallel,
require: argv.require,
retries: argv.retries,
forbidOnly: argv.forbidOnly,
};

mochaGlobalSetup();
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@
"unit-viewer": "yarn mocha --testMatch viewer/**/*-test.js",
"unit-flow": "bash flow-report/test/run-flow-report-tests.sh",
"unit": "yarn unit-flow && yarn mocha",
"unit:ci": "NODE_OPTIONS=--max-old-space-size=8192 npm run unit --forbid-only",
Copy link
Collaborator

Choose a reason for hiding this comment

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

oh. we need to support this in the script. can you not remove ?

Copy link
Member Author

@adamraine adamraine Aug 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The --forbid-only flag doesn't work when we use it like this. This PR adds it to run-mocha-tests.js and gives it a default value of true in CI instead.

Copy link
Collaborator

@connorjclark connorjclark Aug 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

What do you mean it doesn't work when used like this?

I know it doesn't work at all w/o this PR or #15383 , but I don't understand your meaning here

Copy link
Member Author

@adamraine adamraine Aug 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Two reasons:

  • Before this PR we didn't pipe the --forbid-only option into mocha
  • We would need to do npm run unit -- --forbid-ony to pass the flag all the way down

Instead of messing with the flags in the package scripts, I would prefer to just look at process.env.CI. We already do this for --parallel in run-mocha-tests.js:

lighthouse/core/test/scripts/run-mocha-tests.js

Lines 127 to 132 in 06ebc36

// Although much faster, mocha's parallel test runner defers printing errors until
// all tests have finished. This may be undesired for local development, so enable
// parallel mode by default only in CI.
// Also, good to default to false locally because that avoids missing cross-file
// test contamination by chance of mocha splitting up the work in a way that hides it.
default: Boolean(process.env.CI),

"unit:ci": "NODE_OPTIONS=--max-old-space-size=8192 npm run unit",
"core-unit": "yarn unit-core",
"cli-unit": "yarn unit-cli",
"viewer-unit": "yarn unit-viewer",
Expand Down
Loading