tests: add computed/metrics/interactive-test.js to tsconfig #13071
Conversation
connorjclark
requested review from
adamraine
and removed request for
a team
| 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.
Not every trace follows this naming scheme. Maybe make loadTraceFixture and loadDevtoolsLogFixture and use the entire filename?
There was a problem hiding this comment.
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.
the idea here was to get automatic type coercion to LH.Trace, LH.DevtoolsLog
|
@adamraine updated |
brendankenny
left a comment
brendankenny
left a comment
There was a problem hiding this comment.
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)
| 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.
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?
|
|
||
| it('should compute a simulated value', async () => { | ||
| const settings = {throttlingMethod: 'simulate'}; | ||
| const settings = /** @type {LH.Config.Settings} */ ( |
There was a problem hiding this comment.
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.
we have create-test-trace to have real traces for tests, as well
4 participants
Started by seeing if the
tests/computed/metricsfolder could be make type checked, but saw it was a lot of red. Decided to try just one file. Initially I was using themakeParamsOptionalmethod, but then I saw that it may be more straightforward to coerce types as needed.Adds
loadTraceFixturefunction:pretty nice. should be useful.