tests: re-organize driver tests by method #7212
Conversation
| registrationId: id, | ||
| scopeURL: url, | ||
| }; | ||
| function findSendCommandInvocation(command) { |
There was a problem hiding this comment.
some docs here would be good
| scriptURL: url, | ||
| controlledClients, | ||
| status, | ||
| function createSendCommandMockFn() { |
There was a problem hiding this comment.
docs would be good here too (e.g. the returned function has a mockSendCommandResponse on it, use it to push responses, can be used multiple times, where each call will get the next one, etc
| return Promise.resolve(mockResponse); | ||
| } | ||
|
|
||
| function sendCommandOldStub(command, params) { |
There was a problem hiding this comment.
keeping this because it'd be too much churn to remove it and break up across tests?
There was a problem hiding this comment.
yeah I had to draw the line somewhere for the first pass. half the tests still need sendCommandOldStub but absolutely the idea is to migrate the rest of them too
| name: 'novalue', | ||
| }], | ||
| result: | ||
| params.objectId === 'invalid' |
There was a problem hiding this comment.
prefer the old formatting :)
| scopeURL: url, | ||
| }; | ||
| function findSendCommandInvocation(command) { | ||
| expect(connectionStub.sendCommand).toHaveBeenCalledWith(command, expect.anything()); |
There was a problem hiding this comment.
it's a little weird to me that it's using the assumed global connectionStub here (while createSendCommandMockFn is all encapsulated), could we put this as another method on the return value of createSendCommandMockFn()? Or have the particular sendCommand method passed in
There was a problem hiding this comment.
yeah sounds good to add to the mock function! 👍 done
There was a problem hiding this comment.
yeah, ❤️ this.
Main feedback is as before, on the ergonomics two sendCommand helper functions and if we can get rid of sendCommandOldStub. It seems like it's only still used in those top tests for querySelector and getObjectProperty? Would be great to get rid of it and the long list of commands it's not clear which one goes to which test :)
You cool with following up with this and the rest of the driver cleanup in separate PR? |
|
^^ @brendankenny that one was for you :) |
tl;dr - for EASIER REVIEWING just look at 8772643
Summary
It's no secret that driver tests have been somewhat of a mess for a long-time. Driver is one of the most critical pieces of Lighthouse logic and yet adding tests has always been intimidating and somewhat unconvincing that things are working. This PR is a first step in an attempt to change that.
The Changes
sendCommandmethods that it actually uses. This should hopefully make it much easier and less magical for contributors to understand what each method on driver requires, how to add tests that use new protocol methods, etc. A nice side effect is decent documentation about what each protocol method will actually do.Related Issues/PRs
#7174, didn't want to proceed with more major gotoURL without the promised cleanup and testing first