tests: add networkRecord-to-devtoolsLog mocking utility #6171
Conversation
|
|
||
| const URL = 'https://webtide.com/http2-push-demo/'; | ||
| const networkRecords = require('../../fixtures/networkRecords-mix.json'); | ||
|
|
||
| /* eslint-env jest */ | ||
|
|
||
| describe('Resources are fetched over http/2', () => { | ||
| function getArtifacts(networkRecords, finalUrl) { | ||
| // networkRecords-mix.json is an old network request format, so don't verify round-trip. | ||
| const devtoolsLog = networkRecordsToDevtoolsLog(networkRecords, {skipVerification: true}); |
There was a problem hiding this comment.
using a real networkRecord as input has a few problems with verification of the output, namely that we've already snipped a bunch of circular references in the fixture and replaced them with strings and we've changed a few NetworkRequest properties since that old version of NetworkRecorder. The subset of properties needed for this test generates correctly, so it seemed better to just not verify rather than edit the json file.
|
Could this be repurposed to generate hars and other network analysis under simulation mode @patrickhulce? |
|
it could maybe be a start, but there's probably easier/more robust ways to go about it. This is definitely aimed at when you need to test the accessing of just a few properties, like the main resource by URL in a set of records and you don't care that that request started at timestamp 0 and was completed 1 µs later :) |
There was a problem hiding this comment.
this is really neat!
@benschwarz I suppose you could go this route with generation but the huge win here is coming up with all the devtools messages when you don't have any of them. In the simulation case, we have all of them we just need to mutate the timings correctly so this didn't exactly reduce the work necessary on that front.
| // If in a test, assert that the log will turn into an equivalent networkRecords. | ||
| if (global.expect && !options.skipVerification) { | ||
| const roundTrippedNetworkRecords = NetworkRecorder.recordsFromLogs(devtoolsLog); | ||
| expect(roundTrippedNetworkRecords).toMatchObject(networkRecords); |
There was a problem hiding this comment.
🎉 does this mean it's growing on you? :D
There was a problem hiding this comment.
does this mean it's growing on you?
lol, it is very helpful. It would be nice if they had used a regular assert interface like our macro-wielding forebearers intended :)
| @@ -25,16 +26,15 @@ describe('Performance: uses-rel-preload audit', () => { | |||
| let mockSimulator; | |||
|
|
|||
| const mockArtifacts = (networkRecords, mainResource = defaultMainResource) => { | |||
| return { | |||
| return Object.assign(Runner.instantiateComputedArtifacts(), { | |||
There was a problem hiding this comment.
this is a bummer :( more incentive to refactor all the compute artifacts I suppose :)
There was a problem hiding this comment.
yes, the kind of silly thing here is that the (hopefully) next PR just removes all of these. But it keeps the tests passing in the meantime
This came up while refactoring computed artifacts and I've separated it to keep the PRs simpler. Since we'll be requiring files normally, we can't just inject
requestNetworkRecordsinto tests anymore (at least not as easily).We have a lot of places that we want to pass in example/simplified
networkRecords, however our core artifact for network activity is thedevtoolsLog. This PR adds a simple test utility that will create adevtoolsLogthat, when passed back intoNetworkRecorder, will createnetworkRecordswith all the same properties.Very simple records can be passed in (good for tests! e.g. just
[{url: 'https://example.com}]) and the rest of the needed properties will be filled in with reasonable-ish defaults.I've gone through all the existing tests that were mocking
requestNetworkRecords: () => Promise.resolve(records)to use a generated devtoolsLog instead.