Runner Cleanup: SuiteRunner & TestRunner classes #452
Conversation
flashdesignory
commented
Nov 14, 2024
flashdesignory
commented
- move SuiteRunner class into a separate file
- create a new TestRunner class, which a remote workload could consume in the future
- rename _runTestAndRecordResults to TestRunner.runTest()
- create class methods for functions used in the runSync & measureAsync functions
flashdesignory
requested review from
camillobruni,
julienw,
rniwa and
bgrins
|
I like the direction here - would like @julienw to review the change if possible. @flashdesignory to confirm - the big picture idea here is that remote tests will (eventually) consume resources/test-runner.mjs themselves, and it will grow functionality to manage communication back and forth with the suite (postMessage etc)? |
remote tests would use the test-runner class, that's correct 😄. |
resources/test-runner.mjs
Outdated
| let asyncTime; | ||
| const runSync = () => { | ||
| this._runWarmupSuite(); | ||
| syncTime = this._measureSyncTime(syncStartLabel, syncEndLabel); |
There was a problem hiding this comment.
This code is harder to follow with nested function calls. We should go back to having all the code here instead.
There was a problem hiding this comment.
I was trying to find a good way to be able to reuse as much as possible for remote workloads.
If I keep everything in one function, I'll end up overriding the whole function, when I might just need to change one line in one nested function call.
With this, I should be able to create a new class that extends TestRunner, but only overrides one method, which is more targeted and introduces less CLs.
There was a problem hiding this comment.
Which is the line that needs to be changed for remote workloads?
There was a problem hiding this comment.
Some examples, that are slightly different for remote workloads:
this._test.run(this._page);
The remote workloads don't use a page object to wrap elements.
Also, if we opt into async workloads, we might want to await the run function.
_forceLayout()
It works, although I won't use or pass in a frame reference.
These are just some examples
There was a problem hiding this comment.
Both examples can work as is (even if page and frame are not passed in), but I still feel that smaller methods might be easier to follow and reuse.
This shouldn't be a blocker though and if I'm the only one feeling this way, I can certainly change it.
There was a problem hiding this comment.
Perhaps we should make it more of a copy/paste for this PR and then follow up with refactors? I think it'll be easier to say one way or the other with (a) the change unbundled from the file move and (b) when we have a remote implementation to consider.
There was a problem hiding this comment.
Yeah, doing the split as a follow up seems like a good idea here.
There was a problem hiding this comment.
sounds good - i'll fix it up!
|
FTR looks good to me! |
