-
Notifications
You must be signed in to change notification settings - Fork 780
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
feat: parallel test running in workers for node and the browser #1682
base: main
Are you sure you want to change the base?
Conversation
|
@brandonocasey This is awesome! I have to set realistic expectations unfortunately, and I'm not sure yet at this time whether I could include this in core and maintain it going forward to stability and quality that I like and have time for. I'm not saying we will never have this built-in, but there are a number of ways to do this, and I'd like to do it with as little complexity as possible (both for end-users, and for us as maintainers). I suspect that to achieve that, we'd likely have to prioritise other efforts first. For example, I'd like to move toward using the TAP standard for communicating with integrations like Karma, Grunt, and BrowserStack/SauceLabs. Some details at qunitjs/js-reporters#133, and some recent activity at TestAnything/testanything.github.io#36. Among other things, this would remove the need for custom logic for every integration, and would also make message passing much simpler as you'd then get a formatted string from QUnit, ready to go, which can then be passed from the worker upwards and aggregated accordingly. A few points on the current approach, which I found as risks (not problems per-se, but things to think about):
Some thinking of my own (though feel free to ignore, as I'm not the one writing it!):
Practical advice: I would recommend first shipping this as a plugin or wrapper in a separate repo (could live under @qunitjs for continuity if you'd like). There is also @izelnakri working on QUnitX, which has an entry on his roadmap for "implement concurrency". If there's anything you need in core to make this possible or easier, I'd be happy to merge and release smaller changes as-needed to unblock you. We can always revisit things in a little while to see how things are. As a sort of incubator. |
PS: Sorry about the CI noise about missing spaces. GitHub released a feature the day before you submitted this, which proactively analyzes people's build logs and takes out anything that looks like a lint error and tries to render it as an inline check annotation, even for projects that haven't enabled such functionality or decided that it shouldn't be so prominent (and it's doing it without de-duplication logic, so its reporting lots of duplicates from different build variants). There's a way to hide it (dot-dot menu: Show annotations). I've also re-configured our CI now to accomodate this change in behaviour. And, I added a |
Hi @brandonocasey, I've also just skimmed this PR and here are my thoughts as you might be interested: As @Krinkle pointed out, I have a parallel testing in plans for QUnitX project. My latest plan is to build a deno bridge first and implement a concurrent execution there. In the meantime deno/node.js API/DX parity might get better as it does currently on every deno release. Then I'll figure out whether to implement this with native worker_thread module of node.js or perhaps something else at that time(probably it will not be worker_thread based). Currently QUnitX uses esbuild to build it for the browser but perhaps even better approach is to not do any transpilation and build an asset map that reference npm packages. Then doing the concurrency in JS runtime rather than the CLI runtime is something I'll consider/compare. Whatever approach I take, my priority is to make the Lastly, I highly doubt iframe approach would work for deno, my suggestion for you is to separate these two approaches in two different PRs/git branches as 2 different unit of work. This might help in various ways. |
Description
This is a pull request for #947 that I have been tinkering with for a bit now. I think it still has a bit to go, but I would love to hear some feedback and see whether this is something that would be accepted in this project. I will add comments to the code and a loose list of things that I think would still need to be done. in the following section.
As for what the code does:
QUnit.config.isWorker = false
, start a worker factory with one worker.QUnit.config.workerType
, which is a literal className for the worker class to start. On the browser there are Web Workers and Iframe workers. On Node there is just a Node worker that usesworker_threads
.QUnit.config.isWorker = true
as well asQUnit.config.autostart = false
QUnit.config.maxThreads
unless there are less tests thenmaxWorkers
in which case it will only start up 1 worker for each test.Still TODO
importScripts
and iframe workers need to use script tags.moduleId
,module
,testId
,filter
) for nodejs, although maybe this is another pull request.