Skip to content
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

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

camillobruni
Copy link
Contributor

@camillobruni camillobruni commented Nov 26, 2024

  • Copy over package.json from Speedometer3
  • Implement npm test:chrome, npm test:firefox, npm test:safari
  • Add better support for custom worstCaseCount and iterationCount to lower test duration
  • Pass along iterations to Benchmark.validate to fix unipoker expectations
    • Add JetStreamReady and JetStreamDone events to the Driver for easier testing

tests/server.mjs Outdated Show resolved Hide resolved
JetStreamDriver.js Outdated Show resolved Hide resolved
if (urlParameters.has('test'))
customTestList = urlParameters.getAll("test");
globalThis.startDelay = getIntParam(urlParameters, "startDelay");
Copy link
Contributor

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?

Copy link
Contributor Author

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 :)

Copy link
Contributor

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.

Copy link
Contributor Author

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.

@camillobruni
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants