Remote Runner / PostMessge api #456
Conversation
flashdesignory
requested review from
bgrins,
camillobruni,
rniwa,
julienw and
smaug----
| window.version = "1.0.0"; | ||
|
|
||
| /** ********************************************************************** | ||
| * Params |
There was a problem hiding this comment.
@flashdesignory I don't see the following logic used in the main runner. Since it also reads params from the URL, would it be possible to share some code here instead?
There was a problem hiding this comment.
I've shared params.mjs in the other example 728aeab
There was a problem hiding this comment.
we could - the params.mjs is slightly different, but I can take a look at both and see if I can clean up and reuse
There was a problem hiding this comment.
I haven't tried locally but this looks pretty good to me. We can always fix things later if there's anything broken anyway.
r+ with my comments fixed.
Note: it would be a lot easier to review if you'd reworked the PR to have consistent commits, for example:
Commit 1: move files around without changing them, just update the references in other files
Commit 2: add the remote runner + the workload tools
Commit 3: change the workload itself
If you do this for future complex PR this will make them a joy to review (and hence faster to review as well). Thanks!
|
|
||
| export default function App() { | ||
| useLayoutEffect(() => { |
There was a problem hiding this comment.
Can you please add a comment explaining why you chose useLayoutEffect over useEffect?
|
|
||
| export default function App() { | ||
| useLayoutEffect(() => { | ||
| connectToBenchmark(getBenchmarkSuitesManager(), "news-next", 1); |
There was a problem hiding this comment.
I admit I haven't followed closely, but do we need to disconnect when it's unmounted?
| @@ -0,0 +1,41 @@ | |||
| import { BenchmarkTestStep, BenchmarkTestSuite, BenchmarkSuitesManager, forceLayout, getElement } from "speedometer-utils/workload-testing-utils.mjs"; | |||
There was a problem hiding this comment.
oh I so want #371 to be merged finally
| // FIXME: use this._suite in all SuiteRunner methods directly. | ||
| await this._prepareSuite(); | ||
| await this._runSuite(); | ||
|
|
||
| window.removeEventListener("message", handler); |
There was a problem hiding this comment.
nit: put that in a try/finally so that we remove the event listener even if there's an exception
| get frame() { | ||
| return this.#frame; | ||
| } | ||
|
|
||
| get page() { | ||
| return this.#page; | ||
| } | ||
|
|
||
| get params() { | ||
| return this.#params; | ||
| } | ||
|
|
||
| get suite() { | ||
| return this.#suite; | ||
| } | ||
|
|
||
| get client() { | ||
| return this.#client; | ||
| } | ||
|
|
||
| get suiteResults() { | ||
| return this.#suiteResults; | ||
| } | ||
|
|
There was a problem hiding this comment.
frankly all these private variables look superfluous to me, I'd rather put everything public, but well I'm not opposed either.
| * Various methods that are extracted from the Page class. | ||
| */ |
There was a problem hiding this comment.
could the Page class use them directly (maybe as a follow-up, I don't know)
I don't feel comfortable with the duplication.
| performance.mark(suitePrepareStartLabel); | ||
|
|
||
| // Wait for the app-ready message from the workload. | ||
| const promise = this._subscribeOnce("app-ready"); |
There was a problem hiding this comment.
| const promise = this._subscribeOnce("app-ready"); | |
| const appReadyPromise = this._subscribeOnce("app-ready"); |
| this.suiteResults.tests = response.result.tests; | ||
| this.suiteResults.total += response.result.total; |
There was a problem hiding this comment.
why are we overriding the tests property but incrementing the total value?
Should we append the tests instead?
(I haven't researched closely but it would be good to comment about that)
| @@ -146,21 +146,25 @@ class Params { | |||
| return shuffleSeed; | |||
| } | |||
|
|
|||
| toSearchParams() { | |||
| toSearchParams(forRemote = false) { | |||
There was a problem hiding this comment.
can we have an option object parameter instead of a boolean? it's really not clear what this does from the caller site.
(and actually from here neither, a comment would be nice)
| return new URLSearchParams(rawParams).toString(); | ||
| } | ||
| } | ||
|
|
||
| export const defaultParams = new Params(); | ||
|
|
||
| const searchParams = new URLSearchParams(window.location.search); | ||
| const searchParams = new URLSearchParams(typeof window !== "undefined" ? window.location.search : undefined); |
There was a problem hiding this comment.
when is it happening that window isn't present?
optional nit: use ?. operator
This pr introduces Remote Runner workloads.
Workloads that are local OR that point to a remote url (when running Speedometer locally), can communicate and run their own tests via PostMessage.
Basic flow:
Notes