tests(smokehouse): always assert lhr.runtimeError #9130
Conversation
| runtimeError: { | ||
| code: 'ERRORED_DOCUMENT_REQUEST', | ||
| message: /Status code: 403/, | ||
| }, |
There was a problem hiding this comment.
has to be asserted now
| @@ -194,7 +199,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)) | |||
There was a problem hiding this comment.
fixes the case when assertion.actual is undefined, which surprisingly I guess we've never encountered before because this throws on undefined.replace(...)
| const lhr = JSON.parse(fs.readFileSync(outputPath, 'utf8')); | ||
| const artifacts = assetSaver.loadArtifacts(artifactsDirectory); |
There was a problem hiding this comment.
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
| */ | ||
| async function loadArtifacts(basePath) { | ||
| function loadArtifacts(basePath) { |
There was a problem hiding this comment.
fix to keep smokehouse's runLighthouse sync. Since we dropped the streaming JSON loader in #6099, this doesn't need to be async anyways
There was a problem hiding this comment.
runLighthouse needs to become async for #8962 (which still needs review btw :P)
There was a problem hiding this comment.
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 :)
|
added an audit that needs more than the base artifacts to the errors config so that the gathering |
| @@ -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. | |||
There was a problem hiding this comment.
heh, yikes I forgot about that!
| @@ -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, | |||
There was a problem hiding this comment.
doh! everything is off of lhr now we should just iterate through the keys
There was a problem hiding this comment.
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
| 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) { |
There was a problem hiding this comment.
nit: there's a lot of ! to parse :)
Boolean(exitCode) !== Boolean(lhr.runtimeError)?
There was a problem hiding this comment.
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 :)
| */ | ||
| async function loadArtifacts(basePath) { | ||
| function loadArtifacts(basePath) { |
There was a problem hiding this comment.
runLighthouse needs to become async for #8962 (which still needs review btw :P)
follow up to #9121 and part of the discussion from #8865. Adds two additional checks to smokehouse:
runtimeError(or if there's aruntimeErrorit must have had a non-zero exit code). This just double checks the change in ff4f486#diff-8035764042a5861297234f1e1b922ca7R221 (and should always be 👍)runtimeErrorin the run's LHR there must be aruntimeErrorin the expectations (slightly more strict than usual where something left out of the expectations isn't tested). We're asking everyone to keep an eye onruntimeErrorto spot problems, so our tests should be expecting them if one is in there :)Since we weren't checking
runtimeErrorbefore, it meanserror-expectations.jsneeds to be updated because those runs don't actually include aruntimeError(because the audit being run is a metric and doesn't need anything but base artifacts -- mentioned in the last bullet point of #8865 (review)).Also does a drive-by fix to delete the smokehouse test artifact files after use. I was up to several hundred MBs of them in
.tmp/:)