Enable Speedometer to use an external config.json file #515
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| { | ||
| "suites": [ | ||
| { | ||
| "name": "NewsSite-PostMessage", | ||
| "url": "resources/newssite/news-next/dist/index.html", | ||
| "tags": ["default", "newssite", "language"], | ||
| "type": "remote", | ||
| "config": { | ||
| "name": "default" | ||
| } | ||
| } | ||
| ] | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,151 @@ | ||
| // example url for local testing: | ||
| // http://localhost:8080/?config=http://localhost:8080/resources/config.json | ||
| import { defaultSuites } from "./tests.mjs"; | ||
| import { params } from "./shared/params.mjs"; | ||
|
|
||
| const DEFAULT_TAGS = ["all", "default", "experimental"]; | ||
| const DISALLOWED_DOMAINS = ["browserbench.org", "www.browserbench.org"]; | ||
| export class DataProvider { | ||
|
There was a problem hiding this comment.
There was a problem hiding this comment. It does a little more than just fetching a config, how about There was a problem hiding this comment. Sure, either name is better than There was a problem hiding this comment. renamed to BenchmarkConfigurator |
||
| _tags = new Set(DEFAULT_TAGS); | ||
|
There was a problem hiding this comment. Can we just use private variables? |
||
| _suites = []; | ||
|
|
||
| get tags() { | ||
| return this._tags; | ||
| } | ||
|
|
||
| get suites() { | ||
| return this._suites; | ||
| } | ||
|
|
||
| /** | ||
| * Checks if a given string is a valid URL, supporting both absolute and relative paths. | ||
| * | ||
| * This function attempts to construct a URL object. For relative paths, it uses | ||
| * a dummy base URL to allow the URL constructor to parse them successfully. | ||
| * | ||
| * @param {string} url The URL string to validate. | ||
| * @returns {boolean} True if the URL is valid (absolute or relative), false otherwise. | ||
| */ | ||
| _isValidUrl(url) { | ||
| if (typeof url !== "string" || url.length === 0) | ||
| return false; | ||
|
|
||
| try { | ||
| new URL(url, "http://www.example.com"); | ||
| return true; | ||
| } catch (error) { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| _freezeSuites() { | ||
| Object.freeze(this._suites); | ||
| this._suites.forEach((suite) => { | ||
| if (!suite.tags) | ||
| suite.tags = []; | ||
| if (suite.url.startsWith("experimental/")) | ||
| suite.tags.unshift("all", "experimental"); | ||
| else | ||
| suite.tags.unshift("all"); | ||
| suite.enabled = suite.tags.includes("default"); | ||
| Object.freeze(suite.tags); | ||
| Object.freeze(suite.steps); | ||
| }); | ||
| } | ||
|
|
||
| _freezeTags() { | ||
| Object.freeze(this._tags); | ||
| } | ||
|
|
||
| async init() { | ||
| if (params.config) { | ||
| try { | ||
| const benchmarkUrl = new URL(window.location); | ||
| // Don't fetch if the URL is from DISALLOWED_DOMAINS | ||
flashdesignory marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| if (DISALLOWED_DOMAINS.includes(benchmarkUrl.hostname)) { | ||
flashdesignory marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| console.warn("Configuration fetch not allowed. Loading default suites."); | ||
| this._loadDefaultSuites(); | ||
| return; | ||
| } | ||
|
|
||
| const response = await fetch(params.config); | ||
| // Validate that the network request was successful | ||
| if (!response.ok) | ||
| throw new Error(`Could not fetch config: ${response.status}`); | ||
|
|
||
| const config = await response.json(); | ||
| // Validate the structure of the fetched config object | ||
| if (!config || !Array.isArray(config.suites)) | ||
| throw new Error("Could not find a valid config structure!"); | ||
|
|
||
| config.suites.flatMap((suite) => suite.tags || []).forEach((tag) => this._tags.add(tag)); | ||
| config.suites.forEach((suite) => { | ||
| // Validate each suite object before processing | ||
| if (suite && suite.url && this._isValidUrl(suite.url)) | ||
flashdesignory marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| this._suites.push(suite); | ||
| else | ||
| throw new Error("Invalid suite data"); | ||
| }); | ||
| } catch (error) { | ||
| console.warn(`Error loading custom configuration: ${error.message}. Loading default suites.`); | ||
| this._loadDefaultSuites(); | ||
| } | ||
| } else { | ||
| this._loadDefaultSuites(); | ||
| } | ||
|
|
||
| this._freezeTags(); | ||
| this._freezeSuites(); | ||
| } | ||
|
|
||
| _loadDefaultSuites() { | ||
| defaultSuites.flatMap((suite) => suite.tags).forEach((tag) => this._tags.add(tag)); | ||
| defaultSuites.forEach((suite) => this._suites.push(suite)); | ||
| } | ||
|
|
||
| enableSuites(names, tags) { | ||
|
There was a problem hiding this comment. a comment to explain what this function does regarding the 2 parameters would be good. There was a problem hiding this comment. Frankly I'd split the function in 2 functions: enableSuitesByNames and enableSuitesByTags... There was a problem hiding this comment. Some of the logic that we currently use, I just reformatted, but tried to copy / paste as much of the existing logic as possible. This function comes from the current Suites.enable (https://github.com/WebKit/Speedometer/blob/main/resources/tests.mjs#L15). I'm trying to avoid to introduce more changes and rather would tackle that in a follow up. I do agree though that this is an area that can get cleaned up. There was a problem hiding this comment. okay; this can be made simpler to read for sure... |
||
| if (names?.length) { | ||
| const lowerCaseNames = names.map((each) => each.toLowerCase()); | ||
| this._suites.forEach((suite) => { | ||
| suite.enabled = lowerCaseNames.includes(suite.name.toLowerCase()); | ||
| }); | ||
| } else if (tags?.length) { | ||
| tags.forEach((tag) => { | ||
| if (!this._tags.has(tag)) | ||
| console.error(`Unknown Suites tag: "${tag}"`); | ||
| }); | ||
| const tagsSet = new Set(tags); | ||
| this._suites.forEach((suite) => { | ||
| suite.enabled = suite.tags.some((tag) => tagsSet.has(tag)); | ||
| }); | ||
| } else { | ||
| console.warn("Neither names nor tags provided. Enabling all default suites."); | ||
| this._suites.forEach((suite) => { | ||
| suite.enabled = suite.tags.includes("default"); | ||
| }); | ||
| } | ||
| if (this._suites.some((suite) => suite.enabled)) | ||
| return; | ||
| let message, debugInfo; | ||
| if (names?.length) { | ||
| message = `Suites "${names}" does not match any Suite. No tests to run.`; | ||
| debugInfo = { | ||
| providedNames: names, | ||
| validNames: this._suites.map((each) => each.name), | ||
| }; | ||
| } else if (tags?.length) { | ||
| message = `Tags "${tags}" does not match any Suite. No tests to run.`; | ||
| debugInfo = { | ||
| providedTags: tags, | ||
| validTags: Array.from(this._tags), | ||
| }; | ||
| } | ||
| alert(message); | ||
| console.error(message, debugInfo); | ||
| } | ||
| } | ||
|
|
||
| const dataProvider = new DataProvider(); | ||
| await dataProvider.init(); | ||
|
There was a problem hiding this comment. Currently there's a UX problem, because we don't call What do you think of calling this But that's not all !
Therefore, the promise you get from calling the init function or from the async import could be used. That could look like something like that: // Note: maybe the asynchronous stuff can be moved to an async init function?
class MainBenchmarkClient {
constructor() {
this._dataProviderPromise = import("./data-provider.mjs");
this.prepareUI();
this._showSection(window.location.hash);
this._dataProviderPromise.then(() => { window.dispatchEvent(new Event("SpeedometerReady")); });
}
async _startBenchmark() {
const dataProvider = await this._dataProviderPromise;
... Do everything needed with dataProvider
}
async prepareUI() {
// part 1 that doesn't depend on data provider
const dataProvider = await this._dataProviderPromise;
// part 2 that depends on dataProvider
}There was a problem hiding this comment. that makes sense - thanks for the suggestion. There was a problem hiding this comment. @julienw - implemented. Hope you still have time to review 😄 |
||
|
|
||
| export { dataProvider }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1 @@ | ||
| <!DOCTYPE html><html lang="en"><head><meta charSet="utf-8"/><meta name="viewport" content="width=device-width"/><meta name="next-head-count" content="2"/><link rel="preload" href="./_next/static/css/a0dca1379a01e5cf.css" as="style"/><link rel="stylesheet" href="./_next/static/css/a0dca1379a01e5cf.css" data-n-g=""/><noscript data-n-css=""></noscript><script defer="" nomodule="" src="./_next/static/chunks/polyfills-c67a75d1b6f99dc8.js"></script><script src="./_next/static/chunks/webpack-e50e9853db18b759.js" defer=""></script><script src="./_next/static/chunks/framework-2c79e2a64abdb08b.js" defer=""></script><script src="./_next/static/chunks/main-2ba37e62325cc71b.js" defer=""></script><script src="./_next/static/chunks/pages/_app-77983e68be50f72a.js" defer=""></script><script src="./_next/static/chunks/pages/_error-54de1933a164a1ff.js" defer=""></script><script src="./_next/static/tuwdCnX7HYK_fwpI0QvDm/_buildManifest.js" defer=""></script><script src="./_next/static/tuwdCnX7HYK_fwpI0QvDm/_ssgManifest.js" defer=""></script></head><body><div id="__next"></div><div id="settings-container"></div><div id="notifications-container"></div><script id="__NEXT_DATA__" type="application/json">{"props":{"pageProps":{"statusCode":404}},"page":"/_error","query":{},"buildId":"tuwdCnX7HYK_fwpI0QvDm","assetPrefix":".","nextExport":true,"isFallback":false,"gip":true,"scriptLoader":[]}</script></body></html> |
Uh oh!
There was an error while loading. Please reload this page.