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

Expose an id for concurrent test runners (like JEST_WORKER_ID) #55842

Open
blimmer opened this issue Nov 13, 2024 · 22 comments
Open

Expose an id for concurrent test runners (like JEST_WORKER_ID) #55842

blimmer opened this issue Nov 13, 2024 · 22 comments
Labels
feature request Issues that request new features to be added to Node.js. good first issue Issues that are suitable for first-time contributors. test_runner Issues and PRs related to the test runner subsystem.

Comments

@blimmer
Copy link

blimmer commented Nov 13, 2024

What is the problem this feature will solve?

When running tests concurrently (the default setting in the Node test runner), it's common practice to split concurrent test runs across multiple local resources (like locally running servers, databases, etc).

Many other popular test runners offer this feature via an environment variable:

This makes it very easy to set up the proper database connection. For example, here's a snippet from my codebase:

if (process.env.STAGE === "test" && process.env.JEST_WORKER_ID) {
  process.env.DB_DATABASE = `myapp_tests_${process.env.JEST_WORKER_ID}`;
}

Then, in my database docker container, I create n databases, one for each worker. For example, if I run jest with --maxWorkers 8, I create myapp_tests_1 -> myapp_tests_8. During tests, the parallel test suites talk to a different database to avoid deadlocks.

What is the feature you are proposing to solve the problem?

The simplest solution for users coming from other frameworks would be to provide an environment variable just like the other test runners (e.g., NODE_TEST_WORKER_ID).

However, if that's not feasible, adding a workerId to the TestContext could also be a good solution. This solution is not as ideal as the environment variable because people would need to pass the test context to whatever establishes their db/server/etc connection. In my case, at least, this would require refactoring of code. Right now, I rely on that variable being set at file import time.

What alternatives have you considered?

I considered trying to find a solution based on pid, but it's not reliable enough. @redyetidev mentioned in Slack that there might be a solution using hooks. I have not dug into this yet.

@blimmer blimmer added the feature request Issues that request new features to be added to Node.js. label Nov 13, 2024
@github-project-automation github-project-automation bot moved this to Awaiting Triage in Node.js feature requests Nov 13, 2024
@avivkeller avivkeller added the test_runner Issues and PRs related to the test runner subsystem. label Nov 13, 2024
@avivkeller avivkeller moved this from Awaiting Triage to Triaged in Node.js feature requests Nov 13, 2024
@cjihrig
Copy link
Contributor

cjihrig commented Nov 16, 2024

We could add an environment variable fairly easily, but I have a question:

if (process.env.STAGE === "test" && process.env.JEST_WORKER_ID) {
  process.env.DB_DATABASE = `myapp_tests_${process.env.JEST_WORKER_ID}`;
}

There is already a NODE_TEST_CONTEXT environment variable that can be used to determine that the test runner is being used (if you actually need something like that). Why not use something like crypto.randomUUID() to create a unique ID for each file?

@blimmer
Copy link
Author

blimmer commented Nov 16, 2024

The problem is that, as an external step before the test runner is launched, I:

  • Start up the postgres instance via docker compose
  • Run an init script to create n databases (based on the parallelism I run the tests with)
  • Run my migration logic (node-pg-migrate) against all the test databases
  • Create a flush_database() function in each test database to run beforeEach test

I don't want to do this for each test file because the migration logic is quite slow. It's much faster to migrate once and have a fast flush_database function to reset database state between tests.

So a UUID, for my use case, would not be appropriate. Having a sequential integer between 1 and n (where n is the max concurrency) is required for the database setup logic that occurs outside the test runner context.

@cjihrig
Copy link
Contributor

cjihrig commented Nov 16, 2024

Got it. Thanks for the explanation. I think we should add support for this.

@MoLow MoLow added the good first issue Issues that are suitable for first-time contributors. label Nov 17, 2024
@cu8code
Copy link

cu8code commented Nov 18, 2024

I can work on this! will pick this up!

@cjihrig
Copy link
Contributor

cjihrig commented Nov 18, 2024

Thanks @cu8code. Please be sure to handle both cases of test isolation. When --experimental-test-isolation=process, each child process should get an environment variable from 1 to N. When --experimental-test-isolation=none, only the value of 1 should be used.

@hpatel292-seneca
Copy link

Hi @cu8code, are you working on this? If not I can give it a try.

@cu8code
Copy link

cu8code commented Nov 22, 2024

I am working on this 👍

@JakobJingleheimer
Copy link
Member

Is worker.threadId possibly what you're looking for?

@blimmer
Copy link
Author

blimmer commented Dec 1, 2024

We discussed threadId in Slack. It's not quite equivalent to what's available in other test runners:

Screenshot 2024-12-01 at 09 36 27

@JakobJingleheimer
Copy link
Member

Ahh. The same discussion in multiple places 😅

Is that truly a requirement though? Why is sequentiality actually required? Do you need to be able to guess ids relative to each other? If so, my spidy-senses are tingling. It seems merely having a unique and consistently available id to derive the table name is all you need:

myapp_tests_${threadId} should be fine within a test.

@blimmer
Copy link
Author

blimmer commented Dec 1, 2024

I described this in the issue description. TLDR;

My test suite interacts with a database. Recreating the database (with all tables) for each test suite is very slow because I'd need to re-run all migrations, which takes several seconds. Therefore, before the test suite runs, I prepare n test databases (where n is the maximum concurrency) with a flush_database() method that truncates the database between tests. Before the test suite starts running, I need to have static values to name the test databases.

The presence of this feature in Jest, Mocha, and Vtest suggests that many users interested in switching to the node native test runner will want this.

@JakobJingleheimer
Copy link
Member

You can use a "global" before to handle this; it will run within the thread, so threadId will be available (and other tests run within that same thread will have the same threadId for deriving the name).

@blimmer
Copy link
Author

blimmer commented Dec 1, 2024

I suppose that could work; it'd just require some refactoring of how I'm doing things now. With this strategy, though, it seems possible to end up with many orphaned test databases.

For example, if I was using a debugging session for a test (very common) and "CTRL+C"-ed out of the process, skipping the global after to clean up the my_test_db_{thread_id}, the database would be orphaned.

In general, to make it easy for people to switch from the top three most popular other test runners, we should provide the static 1->n environment vairable. It's a feature that I, and many others, will likely expect.

@JakobJingleheimer
Copy link
Member

It definitely does work. I've done it.

process 'beforeExit' event

@blimmer
Copy link
Author

blimmer commented Dec 1, 2024

I appreciate you providing alternatives. If you all decide not to match the feature from Mocha, Jest and Vite, I suppose I can go this direction.

However, if it's really as simple as what @cu8code has proposed with #56091, I don't see why Node shouldn't match the feature.

@stephenh
Copy link

stephenh commented Dec 1, 2024

@JakobJingleheimer adding more data points from our use case, our test suite has 1,250 individual test files (*.test.ts), the majority of which use a database connection.

Today with Jest, we run yarn db-setup to pre-configure 8 our_database_${n} schemas, so we can run have yarn test execute 8 tests in parallel, and finish in "only" ~5-10 minutes, depending on the machine specs.

(Granted, 5-10 minutes is not great--ofc faster is always better, which is why we're looking into node:test and specifically avoiding Jest's per-test-file isolation cost.)

If we did not have determinism in the our_database_${n} assignment, then every invocation of yarn test would pay a set up (and tear down) cost of a new/dedicated our_database_${random pid} which is going to add another ~30-60 seconds of setup time (we have 450 tables in our db schema, so it takes awhile even to init a clean/new db schema), per worker, per yarn test invocation.

Granted, this would be only a ~10-20% cost increase in overall yarn test time, but we're looking for every speed up we can get--as I imagine you would as well, when facing a ~5-10 minute yarn test time. 😅

As Ben has said, being able to have a deterministic/logical worker id (vs. the physical thread id) is key to letting our yarn db-setup and yarn test commands coordinate with each other.

Per your musings in #56091 about your beforeExit suggestion being a "complementary [...] and better" design than our approach, do these new data points change your mind?

I'm always happy to use the latest/best idea for a problem instead of "whatever we did before", but I'm not seeing how "create a new ${our_database_random id} per worker" for every yarn test invocation, at our scale, is a good/better idea.

@JakobJingleheimer
Copy link
Member

Unless I'm missing something, I think this is not new beyond what I understood (or assumed).

You must pay a setup cost at some point. At the start of each thread seems an appropriate time (my suggestion).

I did something similar at my last job (mongodb in memory) with the same seeding and migrations. For ~450 tests, total run-time was 7.8s, of which ~2.5s was setup.

I expect your use-case is not uncommon, so I'd be happy to lay out how to do this in an article.

@stephenh
Copy link

stephenh commented Dec 1, 2024

Right, the difference is whether the setup cost is once per "change to the migrations/schema" (after which the engineer manually runs yarn db-setup) or once for every single invocation to yarn test.

Granted, in CI this doesn't matter, because CI has to db-setup + test for every run, but if I'm an engineer doing local iterations/a TDD loop, I might make ~a few changes to the migrations/schema (and run yarn db-setup once), but then run the tests ~10-20 times w/o changing the migrations, and w/stable our_database_${n}s, we can avoid re-paying the db/schema setup cost.

Appreciate the offer to write up an article, but we don't need to be shown how to do per-worker schema setup/tear down--we could do that if we wanted, we just don't want to, for the reasons we've outlined. :-)

@JakobJingleheimer
Copy link
Member

Do you have some time this week to jump on a quick call? Specifically regarding "change to the migrations/schema".

@stephenh
Copy link

stephenh commented Dec 1, 2024

Hi @JakobJingleheimer ; we genuinely appreciate the offer to VC! If we were stuck on some super-intricate Node internals issue, it would be invaluable.

That said, I don't think there's a lot of unknowns/ambiguities at this point to resolve via discussion; we've described our use case (avoiding per-test/per-worker setup costs), and as Ben lightly alluded to, we think so many other major test frameworks supporting this feature makes it unlikely the ~2-3 of us will come up with a novel solution to something that many others have attempted to solve, and seem to have settled on "stable worker ids" as a suitable solution. It just doesn't seem controversial to us. 🤷

Granted, it's the node:test maintainers decision either way, so we ofc defer. We will likely have to use mocha in the short-term anyway, until this feature (should it get accepted) hits a node LTS release, but would love to switch over when/if that happens. Thanks!

@JakobJingleheimer
Copy link
Member

The reason I suggested the call is to understand "change to the migrations/schema", which I don't and I think the VC would be significantly easier than trying to explain over text. As it is, I do not believe this change is necessary. That could change my mind.

@stephenh
Copy link

stephenh commented Dec 3, 2024

change to the migrations/schema

We use node-pg-migrate to manage our db schema, so each time we need a new column / new table / etc, we write a migrations/(timestamp)-add-author-first-time.ts that, when executed by yarn db-setup, issues a ALTER TABLE ADD COLUMN to alter the db.

So what yarn db-setup does is: start from a clean/empty pg schema (or technically a ~recently new pg_dump of the schema), and apply all pending migrations in the migrations/ directory.

If the engineer hasn't changed any migrations/*.ts files (since their last yarn db-setup invocation), then the database schema itself will not have changed, and we don't have any reason to redo the CREATE TABLEs / ALTER TABLEs / etc (which, per prior comments, can take ~30 seconds on large db schemas), because each of the our_database_${n}s is already setup.

While we happen to use node-pg-migrate, this flow will generally/likely look extremely similar to apps that use postgres/likely any relational db. I.e. anyone that uses prisma will rerun the equivalent yarn db-setup each time they change their prisma schema file. Anyone that uses an "use annotations to define the db schema" (TypeORM, maybe MikroORM iirc) will rerun their own yarn db-setup each time they change their annotations.

But fundamentally engineers "change the schema" (the migration SQL files, the prisma model file, their entity's annotations) an order-or-two-magnitude less often than they run yarn test.

So we'd prefer for the test workers to just assume the our_database_${n} schema has been put in place for them, but they can only do that if they have a deterministic way to know ${n}.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. good first issue Issues that are suitable for first-time contributors. test_runner Issues and PRs related to the test runner subsystem.
Projects
Development

No branches or pull requests

8 participants