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(test_runner): Provide a proper way to run side effects #49994

Closed
rozzilla opened this issue Oct 1, 2023 · 14 comments
Closed

feat(test_runner): Provide a proper way to run side effects #49994

rozzilla opened this issue Oct 1, 2023 · 14 comments
Labels
feature request Issues that request new features to be added to Node.js. stale

Comments

@rozzilla
Copy link

rozzilla commented Oct 1, 2023

What is the problem this feature will solve?

Hey 👋🏼
I've not found a proper way to run side effects (f.e. truncating tables in between different tests).

In Jest I was able to do something like that:

// test1.test.js
describe('test1', () => {
  before(async () => {
    await truncateTables()
  })
  // test1 code
})

// test2.test.js
describe('test1', () => {
  before(async () => {
    await truncateTables()
  })
  // test2 code
})

while keeping truncateTables running only before every other testing code.

In the Node test runner instead, I found that before actions can run at the same time on test1 while test2 is executing (apart of course when concurrency is false).

This cause the impossibility of running anything that cause side effects to other tests, since the before code is not really isolated when running in concurrency mode.

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

Propose a way (a new method, an existing one, whatever), so that I can have the same behavior as in Jest: running a beforeAll asyncronous code can't affect in any way other tests.

What alternatives have you considered?

No response

@rozzilla rozzilla added the feature request Issues that request new features to be added to Node.js. label Oct 1, 2023
@github-project-automation github-project-automation bot moved this to Pending Triage in Node.js feature requests Oct 1, 2023
@benjamingr
Copy link
Member

Thanks for opening a report!

I'm not sure how you expect concurrency to work?

Like, these suites are in different files and you run with concurrency so they will be executed in parallel (that's the feature!). In order for them to run in parallel they need to be written in a way where each suite does not impact other suites. In your example that'd likely be each suite getting its own database instead of resetting shared tables.

I'm pretty sure Jest will also do this in your above example and you just didn't run into a race condition. If you parallelize... stuff will run in parallel.

@cjihrig
Copy link
Contributor

cjihrig commented Oct 1, 2023

Is this covered by #49487? If so, can we close this?

@MoLow
Copy link
Member

MoLow commented Oct 1, 2023

you can probably use

// test1.test.js
beforeEach(async () => {
  await truncateTables()
})
describe('test1', () => {
  // test1 code
})

// test2.test.js
describe('test1', () => {
  // test2 code
})

@benjamingr
Copy link
Member

@MoLow @cjihrig I don't think so, they want to run tests in parallel and having the code work

@rozzilla
Copy link
Author

rozzilla commented Oct 1, 2023

Hey @benjamingr 👋🏼

I'm pretty sure Jest will also do this in your above example and you just didn't run into a race condition. If you parallelize... stuff will run in parallel.

What I did was converting a codebase with all Jest tests to Node.
Regarding the tests mentioned in the example above, what I did was keeping it as it was in Jest, by just replacing the beforeAll of Jest with the before of Node.

Looking also at the documentation, what I think is that the beforeAll of Jest, even though you're running tests in parallel, allows you to run this code before every other test. Is like Jest doesn't runs all of the other pararrelised inner code test only after the beforeAll of all of 'em is executed.

@rozzilla
Copy link
Author

rozzilla commented Oct 1, 2023

Hi @cjihrig 😊

Is this covered by #49487? If so, can we close this?

Mmm, I'd say nope 'cause what I'm referring to is that the current behaviour of beforeAll from Jest is different than the Node before method. As I mentioned above, with Jest the code inside the beforeAll actually runs before every other parallelized test (it's similar to a setup function for a test suite). This difference with the Node test runner makes converting existing codebases hard since you need to take care of side effects, f.e. when handling DBs.

@rozzilla
Copy link
Author

rozzilla commented Oct 1, 2023

Hello @MoLow 👋🏼

Nope, I can confirm this code will have side effects, because when the truncateTables of test1 can run while running test2 code, causing of course flakiness or unexpected behaviours if f.e. you're getting/checking table rows.

@cjihrig
Copy link
Contributor

cjihrig commented Oct 1, 2023

Maybe I'm still missing something, but it seems like the problem is that with Node's test runner, test1.test.js is being run in parallel with test2.test.js and both test files are truncating your database at the same time. Like Benjamin said, that is pretty much expected behavior (remember that the two test files are run as separate child processes, but at the same time).

I proposed looking at #49487, which is a request to run test1.test.js and once it finishes run test2.test.js. You can still use the existing APIs to control how much concurrency is used within each separate file.

@rozzilla
Copy link
Author

rozzilla commented Oct 1, 2023

I proposed looking at #49487, which is a request to run test1.test.js and once it finishes run test2.test.js. You can still use the existing APIs to control how much concurrency is used within each separate file.

Yes, I already looked at it. The thing is that disabling concurrency makes tests way slower than Jest too. And apparently, Jest is able to run side effect code (with the
beforeAll) before every other test, while keeping the concurrency.

@benjamingr
Copy link
Member

Basically what we're saying is that your Jest test s flakey and the fact Jest is kind of slow saved you from running into that edge case. I'm happy to find questions from StackOverflow or a Jest issue if that helps.

This is actually worse since it means when your tests fail it will be even harder to debug.

You need to redesign your test suites to not interfere with each other :]

@rozzilla
Copy link
Author

rozzilla commented Oct 2, 2023

Nope @benjamingr, I'm saying something different 😉
The way Jest handle beforeAll (f.e. explained here) is that:

Jest will wait for this promise to resolve before running tests

So, even with concurrency, when there is a beforeAll Jest waits for it to be completed.
I can completely assure you of this behaviour because the codebase I'm working on (previously in Jest) didn't have any flakiness and worked just fine by truncating all of the tables before every test suite.

To be honest, if Node will not support such feature, it will be really hard to migrate existing and legacy codebases from Jest to Node

@cjihrig
Copy link
Contributor

cjihrig commented Oct 2, 2023

Node's test runner also waits for promises returned from before(), beforeEach(), etc. to be resolved (if it's not doing that it's a bug).

Copy link
Contributor

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot added the stale label Mar 31, 2024
Copy link
Contributor

github-actions bot commented May 1, 2024

There has been no activity on this feature request and it is being closed. If you feel closing this issue is not the right thing to do, please leave a comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale May 1, 2024
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. stale
Projects
None yet
Development

No branches or pull requests

4 participants