-
Notifications
You must be signed in to change notification settings - Fork 9.5k
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: add type checking to cli/tests #6874
Changes from 11 commits
306c541
71e6d19
e84c5a6
f12c9e9
77649f9
9243ff7
4380698
79b8b28
9a153ec
859a00c
0ebfccd
e8c4a31
db39a91
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,14 +28,21 @@ describe('CLI run', function() { | |
const timeoutFlag = `--max-wait-for-load=${9000}`; | ||
const flags = getFlags(`--output=json --output-path=${filename} ${timeoutFlag} ${url}`); | ||
return run.runLighthouse(url, flags, fastConfig).then(passedResults => { | ||
if (!passedResults) { | ||
assert.fail('no results'); | ||
return; | ||
} | ||
|
||
const {lhr} = passedResults; | ||
assert.ok(fs.existsSync(filename)); | ||
/** @type {LH.Result} */ | ||
const results = JSON.parse(fs.readFileSync(filename, 'utf-8')); | ||
assert.equal(results.audits.viewport.rawValue, false); | ||
|
||
// passed results match saved results | ||
assert.strictEqual(results.fetchTime, lhr.fetchTime); | ||
assert.strictEqual(results.url, lhr.url); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 😆 nice find! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yay types! |
||
assert.strictEqual(results.requestedUrl, lhr.requestedUrl); | ||
assert.strictEqual(results.finalUrl, lhr.finalUrl); | ||
assert.strictEqual(results.audits.viewport.rawValue, lhr.audits.viewport.rawValue); | ||
assert.strictEqual( | ||
Object.keys(results.audits).length, | ||
|
@@ -57,6 +64,7 @@ describe('flag coercing', () => { | |
|
||
describe('saveResults', () => { | ||
it('will quit early if we\'re in gather mode', async () => { | ||
// @ts-ignore - testing a thin part of the interface | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we switch to casting There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Because of these errors, closest I could get is
Which isn't much better than the current code. wdyt? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Mocking types is going to come up as an issue like this a whole lot when/if we convert The ideal solution is the mocks become good enough to properly match the types, but failing that for this case, we can also do: const result = await run.saveResults(
/** @type {LH.RunnerResult} */ ({}),
/** @type {LH.CliFlags} */ ({gatherMode: true})); (I think the errors you saw were if you don't include the It looks a little sloppy, but it's also an expression of how narrowly the test is targeting the internals of the current There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh yes, that code works. I must have mistyped the jsdoc comment. |
||
const result = await run.saveResults(undefined, {gatherMode: true}); | ||
assert.equal(result, undefined); | ||
}); | ||
|
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,7 @@ const execAsync = promisify(require('child_process').exec); | |
const {server, serverForOffline} = require('../fixtures/static-server'); | ||
const log = require('lighthouse-logger'); | ||
|
||
/** @param {string} str */ | ||
const purpleify = str => `${log.purple}${str}${log.reset}`; | ||
const smokehouseDir = 'lighthouse-cli/test/smokehouse/'; | ||
|
||
|
@@ -88,18 +89,15 @@ const SMOKETESTS = [{ | |
|
||
/** | ||
* Display smokehouse output from child process | ||
* @param {{id: string, process: NodeJS.Process} || {id: string, error: Error & {stdout : NodeJS.WriteStream, stderr: NodeJS.WriteStream}}} result | ||
* @param {{id: string, stdout: string, stderr: string, error?: Error}} result | ||
*/ | ||
function displaySmokehouseOutput(result) { | ||
console.log(`\n${purpleify(result.id)} smoketest results:`); | ||
if (result.error) { | ||
console.log(result.error.message); | ||
process.stdout.write(result.error.stdout); | ||
process.stderr.write(result.error.stderr); | ||
} else { | ||
process.stdout.write(result.process.stdout); | ||
process.stderr.write(result.process.stderr); | ||
} | ||
process.stdout.write(result.stdout); | ||
process.stderr.write(result.stderr); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lol nice. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sometimes you just gotta go to big G unfortch 🧹 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. lol sorry I was replying to
big G being Google :) i.e. I got the broom emoji by googling "broom emoji" and copy/pasting There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
console.timeEnd(`smoketest-${result.id}`); | ||
console.log(`${purpleify(result.id)} smoketest complete. \n`); | ||
return result; | ||
|
@@ -124,8 +122,8 @@ async function runSmokehouse(smokes) { | |
|
||
// The promise ensures we output immediately, even if the process errors | ||
const p = execAsync(cmd, {timeout: 6 * 60 * 1000, encoding: 'utf8'}) | ||
.then(cp => ({id: id, process: cp})) | ||
.catch(err => ({id: id, error: err})) | ||
.then(cp => ({id, ...cp})) | ||
.catch(err => ({id, stdout: err.stdout, stderr: err.stderr, error: err})) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
.then(result => displaySmokehouseOutput(result)); | ||
|
||
// If the machine is terribly slow, we'll run all smoketests in succession, not parallel | ||
|
@@ -200,6 +198,7 @@ async function cli() { | |
if (failingTests.length && (process.env.RETRY_SMOKES || process.env.CI)) { | ||
console.log('Retrying failed tests...'); | ||
for (const failedResult of failingTests) { | ||
/** @type {number} */ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was weird. Without the explicit type annotation, this error is emitted:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that is weird... @brendankenny? :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Some more clues for @brendankenny. I think it's likely a TypeScript bug. Error is also avoided with this explicit cast:
the troublesome line is this index setter:
Without that line, there's no error. When I hover over The type inferencing is all out of whack here.
but only after the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, this is a typescript limitation.
If the type checker is being less smart, it's probably just running into a control flow limitation, where the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, good minimal example, you should file it (though it may be another variant of microsoft/TypeScript#19955). Not sure if it just wasn't the point of what was being demonstrated, but There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh yeah, end of all this is to work around it, the index needs an explicit type (as you've done here), the array needs an explicit type, or don't save an intermediate variable so there's no need for a circular definition ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great, I've filed an issue: microsoft/TypeScript#29290 |
||
const resultIndex = smokeResults.indexOf(failedResult); | ||
const smokeDefn = smokeDefns.get(failedResult.id); | ||
smokeResults[resultIndex] = (await runSmokehouse([smokeDefn]))[0]; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,22 @@ | |
*/ | ||
'use strict'; | ||
|
||
/** | ||
* @typedef {{path: string, actual: *, expected: *}} Difference | ||
*/ | ||
|
||
/** | ||
* @typedef {{category: string, actual: *, expected: *, equal: boolean, diff?: Difference | null}} Comparison | ||
*/ | ||
|
||
/** | ||
* @typedef {Pick<LH.Result, 'audits' | 'finalUrl' | 'requestedUrl'> & {errorCode?: string}} ExpectedLHR | ||
*/ | ||
|
||
/** | ||
* @typedef {{audits: Comparison[], errorCode: Comparison, finalUrl: Comparison}} LHRComparison | ||
*/ | ||
|
||
/* eslint-disable no-console */ | ||
|
||
const fs = require('fs'); | ||
|
@@ -19,7 +35,6 @@ const PAGE_HUNG_EXIT_CODE = 68; | |
const RETRIES = 3; | ||
const NUMERICAL_EXPECTATION_REGEXP = /^(<=?|>=?)((\d|\.)+)$/; | ||
|
||
|
||
/** | ||
* Attempt to resolve a path locally. If this fails, attempts to locate the path | ||
* relative to the current working directory. | ||
|
@@ -43,10 +58,10 @@ function resolveLocalOrCwd(payloadPath) { | |
* @param {string} url | ||
* @param {string} configPath | ||
* @param {boolean=} isDebug | ||
* @return {!LighthouseResults} | ||
* @return {ExpectedLHR} | ||
*/ | ||
function runLighthouse(url, configPath, isDebug) { | ||
isDebug = isDebug || process.env.SMOKEHOUSE_DEBUG; | ||
isDebug = isDebug || Boolean(process.env.SMOKEHOUSE_DEBUG); | ||
|
||
const command = 'node'; | ||
const outputPath = `smokehouse-${Math.round(Math.random() * 100000)}.report.json`; | ||
|
@@ -137,6 +152,8 @@ function matchesExpectation(actual, expected) { | |
return actual < number; | ||
case '<=': | ||
return actual <= number; | ||
default: | ||
throw new Error(`unexpected operator ${operator}`); | ||
} | ||
} else if (typeof actual === 'string' && expected instanceof RegExp && expected.test(actual)) { | ||
return true; | ||
|
@@ -157,7 +174,7 @@ function matchesExpectation(actual, expected) { | |
* @param {string} path | ||
* @param {*} actual | ||
* @param {*} expected | ||
* @return {({path: string, actual: *, expected: *}|null)} | ||
* @return {(Difference|null)} | ||
*/ | ||
function findDifference(path, actual, expected) { | ||
if (matchesExpectation(actual, expected)) { | ||
|
@@ -199,9 +216,9 @@ function findDifference(path, actual, expected) { | |
|
||
/** | ||
* Collate results into comparisons of actual and expected scores on each audit. | ||
* @param {{finalUrl: string, audits: !Array, errorCode: string}} actual | ||
* @param {{finalUrl: string, audits: !Array, errorCode: string}} expected | ||
* @return {{finalUrl: !Object, audits: !Array<!Object>}} | ||
* @param {ExpectedLHR} actual | ||
* @param {ExpectedLHR} expected | ||
* @return {LHRComparison} | ||
*/ | ||
function collateResults(actual, expected) { | ||
const auditNames = Object.keys(expected.audits); | ||
|
@@ -224,28 +241,30 @@ function collateResults(actual, expected) { | |
}); | ||
|
||
return { | ||
finalUrl: { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what does this move do? :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nothing, just alphabetizing stuff |
||
category: 'final url', | ||
actual: actual.finalUrl, | ||
expected: expected.finalUrl, | ||
equal: actual.finalUrl === expected.finalUrl, | ||
}, | ||
audits: collatedAudits, | ||
errorCode: { | ||
category: 'error code', | ||
actual: actual.errorCode, | ||
expected: expected.errorCode, | ||
equal: actual.errorCode === expected.errorCode, | ||
}, | ||
finalUrl: { | ||
category: 'final url', | ||
actual: actual.finalUrl, | ||
expected: expected.finalUrl, | ||
equal: actual.finalUrl === expected.finalUrl, | ||
}, | ||
}; | ||
} | ||
|
||
/** | ||
* Log the result of an assertion of actual and expected results. | ||
* @param {{category: string, equal: boolean, diff: ?Object, actual: boolean, expected: boolean}} assertion | ||
* @param {Comparison} assertion | ||
*/ | ||
function reportAssertion(assertion) { | ||
// @ts-ignore - this doesn't exist now but could one day, so try not to break the future | ||
const _toJSON = RegExp.prototype.toJSON; | ||
// @ts-ignore | ||
// eslint-disable-next-line no-extend-native | ||
RegExp.prototype.toJSON = RegExp.prototype.toString; | ||
|
||
|
@@ -273,14 +292,15 @@ function reportAssertion(assertion) { | |
} | ||
} | ||
|
||
// @ts-ignore | ||
// eslint-disable-next-line no-extend-native | ||
RegExp.prototype.toJSON = _toJSON; | ||
} | ||
|
||
/** | ||
* Log all the comparisons between actual and expected test results, then print | ||
* summary. Returns count of passed and failed tests. | ||
* @param {{finalUrl: !Object, audits: !Array<!Object>, errorCode: !Object}} results | ||
* @param {LHRComparison} results | ||
* @return {{passed: number, failed: number}} | ||
*/ | ||
function report(results) { | ||
|
@@ -316,11 +336,12 @@ const cli = yargs | |
'expectations-path': 'The path to the expected audit results file', | ||
'debug': 'Save the artifacts along with the output', | ||
}) | ||
.require('config-path') | ||
.require('expectations-path') | ||
.require('config-path', true) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. are our typedefs for yargs off or was this really required before and we were missing it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah they are off. we use a DefinitelyTyped which doesn't define a few things. This There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
yeah, we use an ancient |
||
.require('expectations-path', true) | ||
.argv; | ||
|
||
const configPath = resolveLocalOrCwd(cli['config-path']); | ||
/** @type {ExpectedLHR[]} */ | ||
const expectations = require(resolveLocalOrCwd(cli['expectations-path'])); | ||
|
||
// Loop sequentially over expectations, comparing against Lighthouse run, and | ||
|
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't need this anymore