-
Notifications
You must be signed in to change notification settings - Fork 89
Cleanup/shared #513
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Cleanup/shared #513
Conversation
✅ Deploy Preview for webkit-speedometer-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't tried to run it but this looks reasonable
@@ -13,7 +13,7 @@ export class BenchmarkStep { | |||
this.run = run; | |||
} | |||
|
|||
async runAndRecord(params, suite, test, callback) { | |||
async runAndRecordStep(params, suite, test, callback) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this one be "Test" instead of "Step"??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the new comments, that's super useful!
Cleanup as a result from comments in this pr: #509
resources/shared/helpers.mjs:
getParent
torecursivelyQuerySelector
resources/shared/test-invoker.mjs:
start
method intoTestInvoker
and removeBaseRAFTestInvoker
classresources/shared/benchmark.mjs
Open discussion:
resources/shared/benchmark.mjs - forward url params (current implementation), or grab from top.location.search?
pros: nicer to debug, since you can see the full url of the workload (including params)
cons: we have to propagate and forward the query string from the main document
Follow up:
run build scripts of postmessage enabled workloads