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): always assert lhr.runtimeError #9130

Merged
merged 3 commits into from
Jun 6, 2019
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
2 changes: 2 additions & 0 deletions lighthouse-cli/test/smokehouse/error-config.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ module.exports = {
maxWaitForLoad: 5000,
onlyAudits: [
'first-contentful-paint',
// TODO: this audit collects a non-base artifact, allowing the runtimeError to be collected.
Copy link
Collaborator

Choose a reason for hiding this comment

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

heh, yikes I forgot about that!

'content-width',
],
},
};
8 changes: 8 additions & 0 deletions lighthouse-cli/test/smokehouse/error-expectations.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,15 +14,23 @@ module.exports = [
requestedUrl: 'http://localhost:10200/infinite-loop.html',
finalUrl: 'http://localhost:10200/infinite-loop.html',
audits: {},
// TODO: runtimeError only exists because of selection of audits.
runtimeError: {code: 'PAGE_HUNG'},
},
artifacts: {
ViewportDimensions: {code: 'PAGE_HUNG'},
},
},
{
lhr: {
requestedUrl: 'https://expired.badssl.com',
finalUrl: 'https://expired.badssl.com/',
audits: {},
// TODO: runtimeError only exists because of selection of audits.
runtimeError: {code: 'FAILED_DOCUMENT_REQUEST'},
},
artifacts: {
ViewportDimensions: {code: 'FAILED_DOCUMENT_REQUEST'},
},
},
];
4 changes: 4 additions & 0 deletions lighthouse-cli/test/smokehouse/seo/expectations.js
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,10 @@ module.exports = [
// Note: most scores are null (audit error) because the page 403ed.
requestedUrl: BASE_URL + 'seo-failure-cases.html?status_code=403',
finalUrl: BASE_URL + 'seo-failure-cases.html?status_code=403',
runtimeError: {
code: 'ERRORED_DOCUMENT_REQUEST',
message: /Status code: 403/,
},
Copy link
Member Author

Choose a reason for hiding this comment

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

has to be asserted now

audits: {
'http-status-code': {
score: 0,
Expand Down
9 changes: 8 additions & 1 deletion lighthouse-cli/test/smokehouse/smokehouse-report.js
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,11 @@ function makeComparison(name, actualResult, expectedResult) {
* @return {Smokehouse.Comparison[]}
*/
function collateResults(actual, expected) {
// If actual run had a runtimeError, expected *must* have a runtimeError.
// Relies on the fact that an `undefined` argument to makeComparison() can only match `undefined`.
const runtimeErrorAssertion = makeComparison('runtimeError', actual.lhr.runtimeError,
Copy link
Collaborator

Choose a reason for hiding this comment

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

doh! everything is off of lhr now we should just iterate through the keys

Copy link
Member Author

Choose a reason for hiding this comment

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

everything is off of lhr now we should just iterate through the keys

agreed, though I really want to make sure we check runtimeError, even if it's not in the expectations

expected.lhr.runtimeError);

/** @type {Smokehouse.Comparison[]} */
let artifactAssertions = [];
if (expected.artifacts) {
Expand Down Expand Up @@ -161,6 +166,7 @@ function collateResults(actual, expected) {
expected: expected.lhr.finalUrl,
equal: actual.lhr.finalUrl === expected.lhr.finalUrl,
},
runtimeErrorAssertion,
...artifactAssertions,
...auditAssertions,
];
Expand Down Expand Up @@ -194,7 +200,8 @@ function reportAssertion(assertion) {
} else {
if (assertion.diff) {
const diff = assertion.diff;
const fullActual = JSON.stringify(assertion.actual, null, 2).replace(/\n/g, '\n ');
const fullActual = String(JSON.stringify(assertion.actual, null, 2))
Copy link
Member Author

Choose a reason for hiding this comment

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

fixes the case when assertion.actual is undefined, which surprisingly I guess we've never encountered before because this throws on undefined.replace(...)

.replace(/\n/g, '\n ');
const msg = `
${log.redify(log.cross)} difference at ${log.bold}${diff.path}${log.reset}
expected: ${JSON.stringify(diff.expected)}
Expand Down
26 changes: 18 additions & 8 deletions lighthouse-cli/test/smokehouse/smokehouse.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,10 @@ const path = require('path');
const spawnSync = require('child_process').spawnSync;
const yargs = require('yargs');
const log = require('lighthouse-logger');
const rimraf = require('rimraf');

const {collateResults, report} = require('./smokehouse-report.js');
const assetSaver = require('../../../lighthouse-core/lib/asset-saver.js');

const PROTOCOL_TIMEOUT_EXIT_CODE = 67;
const RETRIES = 3;
Expand Down Expand Up @@ -88,27 +91,34 @@ function runLighthouse(url, configPath, isDebug) {
console.error(`STDERR: ${runResults.stderr}`);
}

if (runResults.status === PROTOCOL_TIMEOUT_EXIT_CODE) {
const exitCode = runResults.status;
if (exitCode === PROTOCOL_TIMEOUT_EXIT_CODE) {
console.error(`Lighthouse debugger connection timed out ${RETRIES} times. Giving up.`);
process.exit(1);
} else if (!fs.existsSync(outputPath)) {
console.error(`Lighthouse run failed to produce a report and exited with ${runResults.status}. stderr to follow:`); // eslint-disable-line max-len
console.error(`Lighthouse run failed to produce a report and exited with ${exitCode}. stderr to follow:`); // eslint-disable-line max-len
console.error(runResults.stderr);
process.exit(runResults.status);
process.exit(exitCode);
}

/** @type {LH.Result} */
const lhr = JSON.parse(fs.readFileSync(outputPath, 'utf8'));
const artifacts = assetSaver.loadArtifacts(artifactsDirectory);
Copy link
Member Author

Choose a reason for hiding this comment

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

we weren't loading the trace(s) and devtoolsLogs before, and I guess it's possible someone will want them at some point :) Regardless, switching to loadArtifacts is the better solution


if (isDebug) {
console.log('LHR output available at: ', outputPath);
console.log('Artifacts avaiable in: ', artifactsDirectory);
} else {
fs.unlinkSync(outputPath);
rimraf.sync(artifactsDirectory);
}

// Artifacts are undefined if they weren't written to disk (e.g. if there was an error).
let artifacts;
try {
artifacts = JSON.parse(fs.readFileSync(`${artifactsDirectory}/artifacts.json`, 'utf8'));
} catch (e) {}
// There should either be both an error exitCode and a lhr.runtimeError or neither.
if (!exitCode !== !lhr.runtimeError) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: there's a lot of ! to parse :)

Boolean(exitCode) !== Boolean(lhr.runtimeError)?

Copy link
Member Author

Choose a reason for hiding this comment

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

nit: there's a lot of ! to parse :)

Boolean(exitCode) !== Boolean(lhr.runtimeError)?

obviously we should go with if (!!(!!exitCode !== !!lhr.runtimeError))

(your suggestion sounds good :)

const runtimeErrorCode = lhr.runtimeError && lhr.runtimeError.code;
console.error(`Lighthouse did not exit with an error correctly, exiting with ${exitCode} but with runtimeError '${runtimeErrorCode}'`); // eslint-disable-line max-len
process.exit(1);
}

return {
lhr,
Expand Down
4 changes: 2 additions & 2 deletions lighthouse-core/lib/asset-saver.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,9 @@ const devtoolsLogSuffix = '.devtoolslog.json';
* Load artifacts object from files located within basePath
* Also save the traces to their own files
* @param {string} basePath
* @return {Promise<LH.Artifacts>}
* @return {LH.Artifacts}
*/
async function loadArtifacts(basePath) {
function loadArtifacts(basePath) {
Copy link
Member Author

Choose a reason for hiding this comment

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

fix to keep smokehouse's runLighthouse sync. Since we dropped the streaming JSON loader in #6099, this doesn't need to be async anyways

Copy link
Collaborator

Choose a reason for hiding this comment

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

runLighthouse needs to become async for #8962 (which still needs review btw :P)

Copy link
Member Author

Choose a reason for hiding this comment

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

runLighthouse needs to become async for #8962 (which still needs review btw :P)

ha, fair. Nothing is async in it anyways, though, so saves us a microtask or two :)

log.log('Reading artifacts from disk:', basePath);

if (!fs.existsSync(basePath)) {
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ class Runner {
if (settings.auditMode && !settings.gatherMode) {
// No browser required, just load the artifacts from disk.
const path = Runner._getArtifactsPath(settings);
artifacts = await assetSaver.loadArtifacts(path);
artifacts = assetSaver.loadArtifacts(path);
requestedUrl = artifacts.URL.requestedUrl;

if (!requestedUrl) {
Expand Down
4 changes: 2 additions & 2 deletions lighthouse-core/scripts/update-report-fixtures.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ async function update(artifactName) {
res(address.port);
}));

const oldArtifacts = await assetSaver.loadArtifacts(artifactPath);
const oldArtifacts = assetSaver.loadArtifacts(artifactPath);

const url = `http://localhost:${port}/dobetterweb/dbw_tester.html`;
const rawFlags = [
Expand All @@ -41,7 +41,7 @@ async function update(artifactName) {

if (artifactName) {
// Revert everything except the one artifact
const newArtifacts = await assetSaver.loadArtifacts(artifactPath);
const newArtifacts = assetSaver.loadArtifacts(artifactPath);
if (!(artifactName in newArtifacts) && !(artifactName in oldArtifacts)) {
throw Error('Unknown artifact name: ' + artifactName);
}
Expand Down
2 changes: 1 addition & 1 deletion types/smokehouse.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
diff?: Difference | null;
}

export type ExpectedLHR = Pick<LH.Result, 'audits' | 'finalUrl' | 'requestedUrl'>
export type ExpectedLHR = Pick<LH.Result, 'audits' | 'finalUrl' | 'requestedUrl' | 'runtimeError'>

export type ExpectedRunnerResult = {
lhr: ExpectedLHR,
Expand Down