-
Notifications
You must be signed in to change notification settings - Fork 4
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
Implement end2end tests #11
base: main
Are you sure you want to change the base?
Conversation
JetStreamDriver.js
Outdated
if (urlParameters.has('test')) | ||
customTestList = urlParameters.getAll("test"); | ||
globalThis.startDelay = getIntParam(urlParameters, "startDelay"); |
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 seems like it will always set startDelay
to undefined
when the there's no startDelay
URL parameter. I think that's fine for testIterationCount
and testWorstCaseCount
since they'd always be undefined on the web as it is. Although maybe it's worth changing there too so folks aren't surprised?
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 hoped to make the initialization explicit by the lines above globalThis.startDelay ??= undefined;
at least for me that's a bit more consistent across all configuration params.
Medium term I'd like to adapt the Params object from Speedometer (this also allows us to easily copy over the ?developerMode UI).
Maybe I missed your point here, but when running via the shell we now also always initialize startDelay=undefined.
With the html runner we'd initialize to undefined by default (and set it again to undefined if there is no matching url param)
But generally not feeling strongly about this one :)
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 the issue is when you do index.html?report=true
startDelay
ends up as undefined
rather than 4000
. If you moved the report check below this and changed it to if (shouldReport) globalThis.startDelay ??= 4000
I think that would fix it.
Safari, at least, relies on the startDelay to make sure the browser has calmed down after launching enough to prevent noise.
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.
ok, then I understood correctly.
Keeping the old behavior with report=true
sg.
I've now merged in the latest PR from speedomter to use the local-web-server package so we don't have to write our own little webserver. |
npm test:chrome
,npm test:firefox
,npm test:safari
worstCaseCount
anditerationCount
to lower test durationiterations
toBenchmark.validate
to fix unipoker expectationsJetStreamReady
andJetStreamDone
events to theDriver
for easier testing