-
Notifications
You must be signed in to change notification settings - Fork 89
Enable Speedometer to use an external config.json file #515
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?
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 didn't test fully locally and I think I don't get the full picture yet, but here is a first set of comments.
cb77ab0
to
0f511fe
Compare
resources/shared/params.mjs
Outdated
@@ -193,6 +205,14 @@ export class Params { | |||
} | |||
} | |||
|
|||
function isValidJsonUrl(url) { | |||
try { | |||
return new URL(url) && url.toLowerCase().endsWith(".json"); |
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.
This will return true for foo.html?query=a.json
or foo.html#bar.json
.
I don't think that's what we want? Can we just use URL
's pathname
instead?
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.
Added a better validation. Let me know if that works for you.
resources/data-provider.mjs
Outdated
try { | ||
const benchmarkUrl = new URL(window.location); | ||
// Don't fetch if the URL is from DISALLOWED_DOMAINS | ||
if (DISALLOWED_DOMAINS.includes(benchmarkUrl.hostname)) { |
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.
Please disallow all subdomains of browserbench.org
at minimum.
Something like benchmarkUrl.hostname.endsWith('browserbench.org')
.
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.
done
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, this looks better.
I still see a small moment at load time where the button is inactive, but this was present before the patch too. Probably because we wait for DOMContentLoaded before calling init/prepareUI. We miught not need this waiting for DOMContentLoaded actually, because we're in a script loaded with type="module"
that is always defered. But that's work for another patch IMO.
@rniwa - following up, if we're good with the pr. |
resources/tests.mjs
Outdated
async prepare(page) { | ||
(await page.waitForElement(".new-todo")).focus(); | ||
page.getLocalStorage().getItem("javascript-es5"); | ||
export const defaultSuites = [ |
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.
Can we undo this change? It's difficult to review all these line changing the format.
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'd love to not revert. I'm aware that this file looks like there are many changed lines, but basically it's a simple cleanup. Happy to walk through the changes in this file, if a sync is helpful.
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.
the gist is:
default-tests file now only contains the defaultSuites array, but it doesn't extend the array anymore (Suites.enable) and it doesn't automatically freeze the Suites / Tags.
The BenchmarkConfigurator is now responsible for assigning suites and calling enableSuites / freezeSuites / freezeTags.
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.
Maybe if wouldn't rename the file the diff is easier.
|
||
try { | ||
const urlObject = new URL(url, "http://www.example.com"); | ||
return urlObject.pathname.toLowerCase().endsWith(".json"); |
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.
This isn't necessarily required, right? You can have other file extensions or no file extension at all with Content-Type: application/json
.
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.
correct, is this a blocker?
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'd rather not have arbitrary rule enforced on a filename.
{ | ||
"name": "NewsSite-PostMessage", | ||
"url": "resources/newssite/news-next/dist/index.html", | ||
"tags": ["default", "newssite", "language"], |
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.
Why is this tag including "default"? Isn't it supposed to be "experimental" instead?
} | ||
if (this.#suites.some((suite) => suite.enabled)) | ||
return; | ||
let message, debugInfo; |
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.
Nit: please don't declare multiple variables in a single statement.
validTags: Array.from(this.#tags), | ||
}; | ||
} | ||
alert(message); |
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 think we should extra a helper function like _reportError which takes message & debugInfo instead of declaring local variables then if-else branching it like this. So that we can do instead:
if (names?.length)
this._reportError(`Suites "${names}" does not match any Suite. No tests to run.`, { providedNames: names, validNames: this.#suites.map((each) => each.name) });
else
this._reportError(`Tags "${tags}" does not match any Suite. No tests to run.`, { providedTags: tags, validTags: Array.from(this.#tags) });
It could be a local lambda too.
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.
r=me assuming my comments will be addressed.
This pr sets up Speedometer to use an optional
config
url param, to assign different workloads / tests to use with the benchmark.The initial config.json enables the news site next workload, which is currently the only workload that opted into the postMessage api for it's experimental version. The test name in the config is optional, if the default tests are targeted.
The new `data-provider' checks an allowed list of external urls that the workloads can be pulled in from. Currently this just houses the netlify preview folder.
Other changes:
the
suites
andtags
are not global anymore and thedata-provider
is the source of truth. This is to ensure that thedata-provider
can process the suites / tests, prior to running speedometer in any mode.news-site dist folder changed, since I had to run the
build
again, after updating shared files from the benchmark.Preview with config assigned:
https://deploy-preview-515--webkit-speedometer-preview.netlify.app/?config=https://deploy-preview-515--webkit-speedometer-preview.netlify.app/resources/config.json