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

feat: parallel test running in workers for node and the browser #1682

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

brandonocasey
Copy link
Contributor

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:

  1. If this is the main thread, indicated by QUnit.config.isWorker = false, start a worker factory with one worker.
  2. The type of worker is determined by 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 uses worker_threads.
  3. That worker spins up with code that handles passing messages back and forth and QUnit, it sets QUnit.config.isWorker = true as well as QUnit.config.autostart = false
  4. The main thread then sends all of the other files that need to be imported/requested for testing.
  5. That worker sends back all of the modules and testIds that QUnit is configured with
  6. The main thread sets some of that information on the local QUnit instance so that the html reporter ui works
  7. It then starts workers. It will start up the configured maximum number of workers QUnit.config.maxThreads unless there are less tests then maxWorkers in which case it will only start up 1 worker for each test.
  8. After all the tests are divided up, the Workers start their run.
  9. As the tests run each worker sends all of the js-reporting and legacy qunit reporting details up to the main thread.
  10. The main thread merges everything in real time and re-emits events when it gets them.
  11. Testing finishes with a pass or fail via runEnd/done and everything is cleaned up.

Still TODO

  1. I think that there will likely have to be a web-worker and iframe-worker javascript bundle built via rollup to keep things browser compatible. I Also think that we would need to make sure that these files get served during testing. Alternatively we could bundle the web worker in the code, and potentially do the same for iframe, but I think iframes are better running cross origin for more performance as they get their own threads in most modern browsers. I have been running a server on two ports. and in my testing cross-orign works better. https://github.com/brandonocasey/qunit-parallel-usage-test is what I used for testing.
  2. I think that places in the browser where I use import might need to use less modern syntax. This goes along with point 1. Web workers need to use importScripts and iframe workers need to use script tags.
  3. Unit testing
  4. Potentially support (moduleId, module, testId, filter) for nodejs, although maybe this is another pull request.
  5. Potentially a flag that defaults to on that disables workers and runs on the main thread. This would prevent breaking anything as it is right now, but offer performance for those who want to use it. This would also mean that QUnit needs an environment agnostic way to load code, such as an import polyfill?

@linux-foundation-easycla
Copy link

CLA Not Signed

@Krinkle
Copy link
Member

Krinkle commented Mar 28, 2022

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

  • The callback API (begin/done, testStart/testDone etc.) are used by plugins and integrations with the expectation that they can inspect the live state by reference, including objects that likely would not survive serialization.
  • Plugins themselves (scripts after qunit.js in HTML, or imports from setup.js in Node) would be nice to handle automatically somehow so that things work transparently.
    • I believe your current approach works correctly for browsers, as (I think?) you load all files in all workers and the developer would simply add plugins among the list of files of source code and test suites.
    • For Node.js, I guess it would work as well, given the requires and files are also loaded in all workers.
  • We support config.reorder. This is on by default and allows us to re-order things, but by default we rarely do that. Tests within the same module generally run in order. This would likely emphasize pre-existing issues. Probably fine.
  • The main distribution of QUnit is as a standalone file. I'm fine with the parallel feature being opt-in and having edge cases where it doesn't work. However, I do think it is a must that it not depend npm or hardcode node_modules, and should work through standalone distribution as well. This could potentially involve 1 or 2 additional files, if that helps, but one point of complication is that in browsers it is difficult to address sibling files (unlike in CSS where paths are resolved relatively), and there is also the matter of bundlers and other integrations including QUnit and serving it in another way entirely. It would be ideal if we can find a way to do everything purely from memory. Perhaps a Data URIs and Blob could work for the JS, and an inline string for the HTML. (Example on MDN, Example in Wikimedia code)

Some thinking of my own (though feel free to ignore, as I'm not the one writing it!):

  • I'd consider distributing test suites as a whole rather than individual tests. I suspect this would make things faster overall by not loading all files in all workers. It would also reduce duplication of module setup and teardown, which can be avoided. See also node-tap which, for example, is a generic way of running parallel processes and each would independently run QUnit. (Today, this doesn't yet work for us beyond simple cases.)
  • I'd consider aggregating information over TAP and having the main thread basically not run QUnit at all. This would likely require a tap-parser that we can safely embed in Core and use in browsers and CLI. It would also need the current HTML interface to consume events (Separate HTML runner and HTML reporter? #1118).

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.

@Krinkle
Copy link
Member

Krinkle commented Mar 28, 2022

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 npm run lint-fix command that automatically formats code for you.

@izelnakri
Copy link
Contributor

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 --browser mode and cli mode to behave as close to as possible, for this reason I'll first try to make tests parallel in JS runtime instead of CLI runtime.

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.

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

Successfully merging this pull request may close these issues.

3 participants