-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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 computed/metrics/interactive-test.js to tsconfig #13071
Conversation
lighthouse-core/test/test-utils.js
Outdated
const dir = `${LH_ROOT}/lighthouse-core/test/fixtures/traces`; | ||
return { | ||
devtoolsLog: JSON.parse(fs.readFileSync(`${dir}/${name}.devtools.log.json`, 'utf-8')), | ||
trace: JSON.parse(fs.readFileSync(`${dir}/${name}.json`, 'utf-8')), |
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.
Not every trace follows this naming scheme. Maybe make loadTraceFixture
and loadDevtoolsLogFixture
and use the entire filename?
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.
Deferring on this, but I think we can just rename the other files. Splitting makes sense if only one of these two exists, but will wait for when other tests use this file (which isn't necessary unless they are in the tsconfig) and it comes up.
segfault in unit tests, re-running ... |
const Interactive = require('../../../computed/metrics/interactive.js'); | ||
const {getURLArtifactFromDevtoolsLog, loadTraceFixture} = require('../../test-utils.js'); | ||
|
||
const {trace, devtoolsLog} = loadTraceFixture('progressive-app-m60'); |
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.
the idea here was to get automatic type coercion to LH.Trace, LH.DevtoolsLog
@adamraine updated |
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.
A lot of this coercing could be avoided by replacing the old minimal test inputs with "real" (at least according to tsc) test inputs. Otherwise type checking isn't bringing many benefits (still opaque to many refactors, hides type errors, etc)
@@ -26,16 +40,20 @@ function generateNetworkRecords(records, timeOrigin) { | |||
startTime: (item.start + timeOriginInMs) / 1000, | |||
endTime: item.end === -1 ? -1 : (item.end + timeOriginInMs) / 1000, | |||
}; | |||
return /** @type {LH.Artifacts.NetworkRequest} */ (record); |
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.
seems like it would be better to move to network-records-to-devtools-log to get a real network request rather than coercing a fake?
const gatherContext = {gatherMode: 'navigation'}; | ||
|
||
it('should compute a simulated value', async () => { | ||
const settings = {throttlingMethod: 'simulate'}; | ||
const settings = /** @type {LH.Config.Settings} */ ( |
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.
a test refactor/util I'd love to see is moving tests like this to real config settings. If we don't want to use the full initializeConfig
(e.g. this var would be initializeConfig('navigation', undefined, {throttlingMethod: 'simulate'})
, which is kind of awkward), we could switch to an options object
const processedTrace = {timestamps: {timeOrigin, firstContentfulPaint, traceEnd}}; | ||
|
||
const cpu = []; | ||
const processedTrace = /** @type {LH.Artifacts.ProcessedNavigation} */ ( |
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.
we have create-test-trace
to have real traces for tests, as well
Started by seeing if the
tests/computed/metrics
folder could be make type checked, but saw it was a lot of red. Decided to try just one file. Initially I was using themakeParamsOptional
method, but then I saw that it may be more straightforward to coerce types as needed.Adds
loadTraceFixture
function:pretty nice. should be useful.