Shared worker feedback & support

[novemberborn]

We're pretty excited about shared workers! But we need more real-world experience in building AVA plugins before we can make it generally available. Please give feedback and build plugins. We'd be more than happy to promote them.

Not sure what to build? Previously folks have expressed a desire for mutexes, managing Puppeteer instances, starting (database) servers and so forth.

We could also extend the shared worker implementation in AVA itself. Perhaps so you can run code before a new test run, even with watch mode. Or so you can initialize a shared worker based on the AVA configuration, not when a test file runs.

Please comment here with ideas, questions and feedback.

[jakobrosenberg]

Sharing use-case here by request 🙂

I'm working on a public template where I need to test both dev and build commands.

I have a folder structure as such

test/
  build/
  common/
  dev/

and the package.json tests

"test": "npm run test:dev && npm run test:build",
"test:dev": "node test/wrapper dev ava test/{common,dev}/**/*.test.js",
"test:build": "node test/wrapper build ava test/{common,build}/**/*.test.js"

The test/wrapper script

// assign `dev` or `build` to mode and `ava` to cmd
const [mode, cmd, ...args] = process.argv.slice(2)

async function wrapSpawn() {
    const teardown = await setups[mode]()
    spawnSync(npx, [cmd, ...args], { stdio: 'inherit' })
    teardown()
}

entire wrapper script

const { spawnSync, spawn } = require('child_process')
const { checkPort, wait } = require('./utils')
const fkill = require('fkill')

const npm = /^win/.test(process.platform) ? 'npm.cmd' : 'npm'
const npx = /^win/.test(process.platform) ? 'npx.cmd' : 'npx'

// assign `dev` or `build` to mode and `ava` to cmd
const [mode, cmd, ...args] = process.argv.slice(2)

const setups = {
    async dev() {
        const child = await spawn(npm, ['run', 'dev'], )
        await checkPort(5000, 15000)
        await wait(500)
        return () => fkill(child.pid, { tree: true, force: true })
    },
    async build() {
        spawnSync(npm, ['run', 'build'], )
        const child = await spawn(npm, ['run', 'serve'], )
        await checkPort(5000, 15000)
        return () => fkill(child.pid, { tree: true, force: true })
    }
}

async function wrapSpawn() {
    const teardown = await setups[mode]()
    spawnSync(npx, [cmd, ...args], { stdio: 'inherit' })
    teardown()
}

wrapSpawn()

I'm somewhat happy with the script in its current state. It's not 100% self explanatory in package.json, but works flawlessly.

---

I had previously used shared context and being able to use reserve was great for spinning up a single server. However I couldn't get the teardowns working. Either the server shut down along with the test file that spawned it or it didn't shut down at all.

---

For my use case the best option would be an argument that lets me provide a setup script. (I guess I could DIY that for now)

ava test/dev/*.js --setup test/_dev-setup.js
ava test/build/*.js --setup test/_build-setup.js

The setup file:

//test/_dev-setup.js
module.exports = async function() {
  const handle = await createMyDevServer()
  
  // return a teardown function
  return function(){
    handle.kill()
  }
}

[djake]

Hi! Did anything ever come of this?

[novemberborn]

@jakobrosenberg so the way I was thinking this might work:

• We add support in AVA config files for loading plugins
• We allow plugins to register tasks that run before and after test runs
• (And they'd be aware of watchers, to perhaps keep resources alive for longer)
• The message passing capabilities already available would allow you to load the plugin in a test file and access URLs and whatnot

What do you think?

[jakobrosenberg]

Sounds good.

What are your thoughts on being able to override the plugins through the CLI (npx ava path/to/test --plugin path/to/plugin.js) and/or dynamic higher order components? (Am I using this term correctly?)

//myplugin.js
module.exports = payload => {
  payload.foo = 'bar'
  return process.argv.includes('--my-custom-flag')
    ? require('./alt-plugin')(payload)
    : require('./main-plugin')(payload)
}

[novemberborn]

I'm reluctant to add CLI options for things that you would apply on every test run, I'd rather only expose flags for the stuff you need to do occasionally to debug a test run.

Passing arguments to plugins that have been registered through the configuration could be interesting though.

[nathanforce]

Not too sure if this is the right place for this or if a new issue would be better, but I'll start here!

I'll start this all of with a bug caveat that I don't have much experience with workers, memory sharing, or any of the other nitty gritty stuff involved here.

My ultimate goal is to create a jsdom instance for each test in my frontend application and avoid polluting the global namespace. I want to do this so that I can benefit from Ava's concurrency.

To run my app in jsdom I'm bundling the whole thing up using esbuild. When a <script> on the page requests my app bundle jsdom will respond with a Buffer created from esbuild's OutputFile.contents, a Uint8Array.

https://github.com/evanw/esbuild/blob/4e336e4f313a1f8a4bd6032f037d23f725f52e75/lib/types.ts#L101

This all works nicely and, importantly, it avoids serializing the contents of the JS file.

This works fine but has a scaling issue as each test suite/process needs to build the app. I wondered if workers might help solve this by building once and providing the output to all tests that need it.

I've put together a rudimentary example using workers that does indeed reduce the runs of esbuild to just 1, but the test time is far slower than multiple runs. My guess is that this is because the communication between the workers is serializing the build output.

This is where my limited knowledge of this type of thing starts to fade. It seems based on the docs for postMessage that the expected solution here would be to use SharedArrayBuffer to avoid copying the data. Would it be possible to expose that API in Ava's shared worker API or is it purposefully not exposed?

Sorry for the long winded backstory! Thanks for all your work.

[novemberborn]

@nathanforce if you could repost that as a Discussion that would be more appropriate. Happy to explore this with you.

[djake]

NODE_ENV is set to test when running tests but not set when running a shared worker.

I'm a little torn on what I think the behavior should be, though as a baseline suggestion I think the behavior should be documented.

It seems like maybe the default should be NODE_ENV="test" for consistency, but whatever the default, I think this should be customizable (without having to write to process.env from within the worker). This would probably mean adding a property to the SharedWorker constructor, which creates a problem similar to the existing initialData property.

[novemberborn]

Nice observation! Currently the worker thread inherits the NODE_ENV of the main AVA process, which does not care about the environment it's in. If you set NODE_ENV when invoking AVA then that should be inherited by the worker thread.

I'm not convinced the worker thread environment should be manipulatable by the environmentVariables configuration. Those are meant for the test workers.

One could also argue that NODE_ENV=test is only relevant for the code being tested, not for shared worker code.

Could you elaborate on your use case?

[djake]

I am doing http endpoint testing. I spin up a postgres docker container with dockerode and launch supertest/express in before hooks, then kill the container in an after.always. The application, including some singletons, as well as the test setup code are configured using node-config and rely on NODE_ENV to select configuration from an appropriate yaml file, including things like the db name. This is a new small project and all the tests are in one file (yay macros!), and share state via the database. This is fine for starters but I thought it would be a nice opportunity for me to tryout sharedWorkers by spinning up multiple containers or multiple dbs in the one container and then letting each file request a set of db credentials from the worker.

For starters, I just moved my existing setup code from a before hook to a worker. Things stopped working which ultimately turned out to be because the worker was using config from development.yml and the tests were using config from test.yml.

One could also argue that NODE_ENV=test is only relevant for the code being tested, not for shared worker code.

I don't necessarily disagree with this, which is why the only suggestion I'm confident of is that it be documented. I offered adding a property to the SharedWorker constructor as a potential solution because I agree with your comment that environmentVariables should be reserved for the test workers.

I think the use case for the existing environmentVariables is probably something like a) I can do npx ava without having to worry about a dotenv file or configuring env vars in my shell and/or b) ava doesn't willy nilly pass along other environment vars to test workers. That's just a guess, and I agree the sharedWorkers and testWorkers shouldn't necessarily share config, but I think the same logic applies to sharedWorkers as the test workers.

Conversely, I think my current option for configuring a sharedWorker's env vars would be:

• to actually set the environment variable in my shell. That is worse dx (no more simple npx ava) and could provoke confusion, e.g. if you want MY_DB_CREDS=something npx ava with ava configuration environmentVariables: { MY_DB_CREDS: 'somethingelse' }, that's a recipe for a very annoying first experience with ava for a future developer.
• pass some config via initialData and write it to process.env. I have a strong aversion to such direct access of process.env (which is why I use node-config in the first place) and tend to lint it.

Hope that helps. Happy to elaborate further, and thanks for your consideration!

[novemberborn]

For starters, I just moved my existing setup code from a before hook to a worker. Things stopped working which ultimately turned out to be because the worker was using config from development.yml and the tests were using config from test.yml.

Yea that's not good.

I think the use case for the existing environmentVariables is probably something like a) I can do npx ava without having to worry about a dotenv file or configuring env vars in my shell and/or b) ava doesn't willy nilly pass along other environment vars to test workers.

You're right on a. But all environment variables are passed to the test worker, so b doesn't hold.

I agree the sharedWorkers and testWorkers shouldn't necessarily share config, but I think the same logic applies to sharedWorkers as the test workers.

That's another view. Any worker, test or shared, should have the same environment. So NODE_ENV defaults to 'test', and environmentVariables are applied.

This would be the most consistent approach. What do you think?

[djake]

Consistent sounds good to me!

[timdp]

I've been experimenting with running Puppeteer in a shared worker (and having it connect to Fastify in another one). It's been smooth sailing so far but there's one thing I can't figure out: robust disposal.

The Node docs mention that there's no process.on(...) inside worker threads. As a result, I can't use that to kill the browser on exit, Ctrl+C, etc.

What I've done so far is use the message bus to have workers accept a dispose command. I trigger that command from the teardown hook on the outside. However, if the message bus itself gets stuck waiting for a reply, or if the process gets killed, that doesn't do anything since the message is never received.

In my case, if disposal doesn't run, I get a headless Chrome process that I need to kill manually. Similarly, I use ngrok to add SSL to my Fastify server, and that also spawns a process that doesn't get killed automatically.

It'd be great if AVA itself would let me register a (guaranteed) teardown function inside a worker as well. As far as I can tell, that's not available from AVA today and I also wouldn't know what underlying API to use.

Any guidance around this? Thanks!

[timdp]

Just to make sure I understand, the intent would be this:

• The default behavior (non-watch mode, optionally serial) would be to keep the shared worker (and e.g. Puppeteer as a result) alive for the duration of all tests, and gracefully terminate it afterwards.
• In watch mode, the shared worker would stay active until I send Ctrl+C (with a timeout, and another Ctrl+C to forcibly terminate).

This seems to align with what you wrote under #2734 (at least the parts I understand without digging deeper) and it makes perfect sense to me.

I appreciate that it's a big refactor though so I'm open to workarounds. Unfortunately, the best I can come up with would be to sidestep shared workers entirely, and instead fork my own thread or daemon process that wraps Puppeteer and includes some keepalive logic - i.e., if it stays idle for a few seconds, assume no new tests are going to come in, and gracefully terminate. However, that would be heavily redundant if AVA can do all the heavy lifting for me.

[novemberborn]

Maybe it's OK if Puppeteer is closed, and you start it up again if another test shows up?

[timdp]

Is that about the last paragraph or about the intended behavior for AVA? In both cases, I'd argue for a deterministic code path where all tests are handled by the same Puppeteer instance.

Conversely, it'd be even more robust if all tests were handled by their own Puppeteer instance, but since that's too slow, I'd rather have the complete opposite than something in between. For what it's worth, I can also imagine cases where we want to clear Puppeteer's session between tests, but that's completely out of scope here. I do have some ideas around it, and they're compatible with the behavior I'm proposing.

[novemberborn]

Sure, just saying, in the meantime you can achieve closing Puppeteer when there are no connected test workers, which assuming concurrent execution is at least better than having each test file spin up its own instance.

[timdp]

True, that's easier than maintaining a separate process. I've introduced a Recyclable class that's essentially a cyclic state machine in timdp/ava-shared-workers-experiment@9881aff. Not exactly pleasant but it's a decent workaround for now. Thanks for the tip!

[cspotcode]

Is there a shared worker to run a setup operation once, with all test files waiting for it to complete before running their tests? This task configures something in the host environment.

@ava/cooperate only gets you part of the way there.

An un-managed semaphore can ensure that only one worker does the setup task. A lock ensures all other workers wait for it to complete.

The missing piece is error reporting. If the setup task fails, all workers should error. This could be as simple as:

import {setValue, getValue} from @ava/shared-values';
const {errorMessage} = getValue('setup-task-result');

[novemberborn]

No, but that'd be great to have. Some of the points here are relevant #3004:

• A way to load the worker when AVA starts up. This could be declarative in the configuration, or perhaps we can use the factory style of the ava.config.* files and provide a registerSharedWorker() function that follows ava/plugin
  ava/plugin.d.ts
• A way so that test runs don't start before these workers are ready.

[cspotcode]

I implemented a plugin to do this: https://github.com/cspotcode/ava-shared-setup
Unfortunately, I can't use it myself because I'm stuck using child processes instead of worker_threads.

It implements:
a) a shared key-value store
b) a way to execute one-time setup tasks

Importantly, a setup task does not run inside the shared worker! Rather, it runs in the same thread as your tests, but only in one of them. All other test threads wait for it to finish and throw the same error message if a failure happens.

This means setup code lives alongside your tests, and running ava is always sufficient to execute setup tasks. No need to some-setup-command && ava in the shell. IDE integrations should work. And when running a subset of tests that don't require this task to run, it can be skipped.

Usage looks like this:

import {Context} from 'ava-shared-setup';
const ctx = new Context('my-context-id');
test('foo', async () => {
  await ctx.task('my-task-id', async () => {
    await exec('start-shared-server');
  });
});

[novemberborn]

Very cool!