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

Easier Implementation of isSerial? #7713

Closed
marbuser opened this issue Jan 26, 2019 · 9 comments
Closed

Easier Implementation of isSerial? #7713

marbuser opened this issue Jan 26, 2019 · 9 comments

Comments

@marbuser
Copy link

🚀 Feature Proposal

The current implementation of isSerial is overly “complicated” and it really shouldn't be this complex for people who need to run their tests sequence rather than in parallel. Ideally we should just be able to set a jest.config.js configuration option, rather than the current implementation which requires that we write our own test-runner.

Many of us are still using the --runInBand option, despite being discouraged from using it for our particular use-case, because of it's simplicity over the isSerial option and the fact that we get pretty much the same result out of it for 99% less effort. (It's a hell of a lot easier to add --runInBand and that's it rather than exporting a class to make my own test-runner...)

Motivation

This isn't just effecting a small group of people. Anyone who is using a database like MySQL (and others) will most likely need this option enabled in order to prevent huge amounts of table errors due to tests being run in parallel.

I'm also confident there are probably people out there that just like running their tests sequentially rather than in parallel.

Example

jest.config.js

module.exports = {
  testEnvironment: 'node',
  verbose: true,
  collectCoverage: true,
  isSerial: true,
};

Note: isSerial is purely a placeholder name based from the current implementation. I think it would probably make more sense to make this name a bit more user-friendly/relevant to what it's actually relating to in the config.

Pitch

Jest's very own motto on its homepage is; “Jest is a delightful JavaScript Testing Framework with a focus on simplicity.”

I believe with the current implementation of isSerial, it's going against this motto.

I'm not saying one implementation is better than the other, I'm sure the current implementation has some sort of use for some people. But why can't we have both so that both groups of people can be happy?

I feel having the current implementation (writing a test-runner), will benefit those who want full access to that kind of stuff and to be able to do their thing.

But I also feel that having the proposed implementation of having it settable in the config file will benefit users like myself who literally only need to enable this option to stop database errors and no other reason. I also feel this will benefit newcomers to Jest as well. I've only started using Jest the past week or so, and I can say for certain I almost swapped to another testing framework because of how somewhat overly complex and irritating it is to make test NOT run in parallel. (But I stuck with Jest for now since I'm quite a big fan of how it's run and just it's overall design in general. 👍)

Hopefully this is taken on board as I believe it can result in a win-win situation for everyone both noobie and experts alike.

@viridia
Copy link

viridia commented Feb 17, 2019

Same issue here. It is fairly common that integration tests that run against a shared resource with persistent state, such as a test instance of MongoDB or MySQL, cannot be safely run in parallel. Forcing us to use --runInBand is not the right answer, as it makes all tests sequential, as opposed to the just tests that really need it.

It's important to be able to run tests against a real database, as opposed to a mock; because database APIs are so complex and deep, no mock or fake can fully capture all of the nuances of behavior. Otherwise you end up merely testing your assumptions against your assumptions, which yields no useful information.

The other option - spinning up a brand new database for each individual test - is so expensive that it essentially destroys all of the benefits of running in parallel anyway. (It also removes one of the other advantages of running against a live db, which is the ability to manually query the rows after the fact to see what went wrong.)

Ideally, what we need is something like a semaphore or mutex that is associated with a specific test, possibly via an attribute or decorator, such that no two tests would run at the same time if they referenced the same shared resource. Failing that, a simple flag that says 'don't run this test at the same time as any other' would work in a pinch.

(I've thought about manually dropping a semaphore into beforeEach(), however this messes up the measurement of elapsed time for the test - it means the last test to run will have a duration greater than all the tests that it waited on.)

Note that I am in no way lobbying for a feature to run tests in a specific order, as some others have done. I understand the philosophical arguments for and against that proposal, but it is not something I need for my own projects. (I wrote a lot of tests during my 8 1/2 years at Google, which is a fairly testing-intense culture, so I have a fairly good notion of what is considered proper test hygiene.)

@SimenB
Copy link
Member

SimenB commented Feb 17, 2019

Forcing us to use --runInBand is not the right answer, as it makes all tests sequential, as opposed to the just tests that really need it.

That's exactly why we don't have it as a configuration option - it's not the correct solution to your problem, and you make the entire test run slower than it has to be. The point of sticking it in a custom runner was that you'd create a MyDbRunner and MyDbEnvironment pair which took care of setting up and tearing down a db, and it would be configured to only run for the tests that need it. If it's as easy as sticking runInBand: true in the config, people would do that (since it's quick and easy) instead of spending the time configuring their tests properly to be as efficient as possible.

I realize this might come of as a "you do tests wrong, we know the only way of doing it" type of thing, which is not the intention at all. It's the same thing with forceExit - it only ever treats a symptom, not an issue. And if we force you to be explicit about passing it as a command line option (or make it convoluted to do in config), you feel the pain of it and want to fix the underlying issue instead of setting and forgetting a workaround.

The thought is that you don't just have integration tests, and we don't want to slow all other tests just because some cannot be run in parallel.

We want to make it easy to configure Jest correctly - if we're unable to do that we want to make it hard to configure Jest incorrectly. That said, the use case of running against a DB is very real, so if we can make it easier that's very much welcome.

Ideally, what we need is something like a semaphore or mutex that is associated with a specific test, possibly via an attribute or decorator, such that no two tests would run at the same time if they referenced the same shared resource.

Is that different from a custom runner today with a testMatch that just hits integration tests? How would it look?

@viridia
Copy link

viridia commented Feb 17, 2019

Well, a google search for "jest custom runner" yields no useful results on how to write a custom runner. The Jest docs briefly mention the concept of a custom runner, but I can't find documentation as to what a runner is, what it's role is, or what the API looks like. So I wouldn't know where to begin.

I've heard the argument about "treating the symptom", but this is not a symptom of a problem - it is, in my opinion, the "best practice".

Also, it seems to me that it if you want to add extra friction to making tests serializable, there are other ways to achieve that - having to mark each individual test function as serial would be one option.

One final point: there are some GUIs that display Jest results (such as the VSCode plugin) which don't allow options to be passed in on the command line; so it's basically impossible to run my tests under that GUI.

@marbuser
Copy link
Author

marbuser commented Feb 20, 2019

Forcing us to use --runInBand is not the right answer, as it makes all tests sequential, as opposed to the just tests that really need it.

That's exactly why we don't have it as a configuration option - it's not the correct solution to your problem, and you make the entire test run slower than it has to be. The point of sticking it in a custom runner was that you'd create a MyDbRunner and MyDbEnvironment pair which took care of setting up and tearing down a db, and it would be configured to only run for the tests that need it. If it's as easy as sticking runInBand: true in the config, people would do that (since it's quick and easy) instead of spending the time configuring their tests properly to be as efficient as possible.

I realize this might come of as a "you do tests wrong, we know the only way of doing it" type of thing, which is not the intention at all. It's the same thing with forceExit - it only ever treats a symptom, not an issue. And if we force you to be explicit about passing it as a command line option (or make it convoluted to do in config), you feel the pain of it and want to fix the underlying issue instead of setting and forgetting a workaround.

The thought is that you don't just have integration tests, and we don't want to slow all other tests just because some cannot be run in parallel.

We want to make it easy to configure Jest correctly - if we're unable to do that we want to make it hard to configure Jest incorrectly. That said, the use case of running against a DB is very real, so if we can make it easier that's very much welcome.

Ideally, what we need is something like a semaphore or mutex that is associated with a specific test, possibly via an attribute or decorator, such that no two tests would run at the same time if they referenced the same shared resource.

Is that different from a custom runner today with a testMatch that just hits integration tests? How would it look?

Hey @SimenB ,

So while I do agree that runInBand is not the best "practice" to use in general, you have to understand it from a user perspective. I'm relatively new to writing tests, I've maybe been doing it about 6 months, so I'm not familiar with what I would consider "Advanced Topics" such as custom runners, and it can be hard understanding these somewhat "Advanced Topics" when there isn't much documentation on it, and not many external resources on Google either.

I'm sure a lot of others are in the same situation as me where you want to get into testing because you see the benefits it brings and allows us to develop more stable software, but you have to also understand that for most of us who don't know testing (or the Jest codebase) off the back of our hand like you do, it's hard for us to adopt topics like writing our own custom runner just to achieve the simple task of running tests sequentially.

If it's as easy as sticking runInBand: true in the config, people would do that (since it's quick and easy) instead of spending the time configuring their tests properly to be as efficient as possible.

I'd argue against this. Currently you can achieve this by adding --runInBand on the npm script, and that's still pretty easy.
My initial thoughts when I realised it wasn't a configuration option was "Well that's pretty stupid, guess I can't have all my config options in one place", and then proceeded to add it as a npm script argument.

People who want to setup their tests correctly WILL use a custom runner even if the runInBand config option is available, but for people like myself who;

  • Have no idea how to write a custom runner because the documentation and learning resources are limited.
  • Are working on projects where we have much more important things to do than screw around writing a custom runner for testing JUST to get our tests to run correctly.

We will likely find the easiest option to achieve what we are trying to do, I.e run our tests sequentially instead of in parallel.

So really there are 2 solutions you could take to solve this issue;

  • Add runInBand as a config option for the people who will use it anyway because they just want their tests to run, regardless of if custom runners are better.
  • Add/Create some better documentation for creating a Jest Runner that someone who is brand new to testing can follow. (Unrelated to this discussion, but I do feel some of the current Jest documentation is very anti-beginner imo)

Those are the only two solutions I can see resolving this issue, or ideally both would be the best option.
That way, people who have no interest in custom runners and want their tests to run sequentially in a quick and dirty way can just use the runInBand config option, and those who want to do it the "right" way can have the documentation and resources available to write a customer runner.

To give a further example;

The point of sticking it in a custom runner was that you'd create a MyDbRunner and MyDbEnvironment pair which took care of setting up and tearing down a db, and it would be configured to only run for the tests that need it.

Being relatively new to testing, I have no idea how to do any of this and there is no documentation at all that I can consult to get a better understanding of HOW to do it. So how can you expect me (and plenty of others who are new to testing) to write a customer runner when there is no documentation on HOW to do it.

@InExtremaRes
Copy link
Contributor

Forcing us to use --runInBand is not the right answer, as it makes all tests sequential, as opposed to the just tests that really need it.

That's exactly why we don't have it as a configuration option - it's not the correct solution to your problem, and you make the entire test run slower than it has to be. The point of sticking it in a custom runner was that you'd create a MyDbRunner and MyDbEnvironment pair which took care of setting up and tearing down a db, and it would be configured to only run for the tests that need it. If it's as easy as sticking runInBand: true in the config, people would do that (since it's quick and easy) instead of spending the time configuring their tests properly to be as efficient as possible.

@SimenB I'am also curious about how to do that. As have been told there is very difficult to find documentation. Is there a place where we can learn (documentation, and example, or anything)?

@InExtremaRes
Copy link
Contributor

Hi again @SimenB

It's not my intention to bother you but I recently came into this issue again and still not be able to find proper documentation. Maybe I'm not looking in the right places.

The situation is very common and usual: I have unit tests, all of them with extension .test.ts, and integration/e2e tests with extension .test.e2e.ts. The latter ones can't run in parallel.

How/What should I configure to correctly run e2e tests sequentially but unit tests in parallel? I'm using jest-circus BTW. I thought using multi project configuration (the projects property) would allow me to do that, but again, the config doesn't allow me to pass runInBand. Do I have to create my own test runner? If so, will it replace jest-circus? Is it enough to create a test environment instead and use it with multi projects? How?

To be clear, I'm not asking you to solve my particular use case, but instead asking if you could point me to the right documentation or example. Any guidance is very welcome since I'm not familiar with jest infrastructure and is a lot to learn.

@CarlOlson
Copy link

CarlOlson commented Dec 6, 2021

@SimenB This thread comes up as a top search result on google. Can we get an update on if or when there may be documentation? I can dig around in the code, but if it isn't documented I don't feel safe using the underlying API.

I realize this might come of as a "you do tests wrong, we know the only way of doing it" type of thing, which is not the intention at all. It's the same thing with forceExit - it only ever treats a symptom, not an issue.

We can't know this if it isn't in the documentation. And it is frustrating when search results point back here.

If it's as easy as sticking runInBand: true in the config, people would do that (since it's quick and easy) instead of spending the time configuring their tests properly to be as efficient as possible.

People are still going to choose the easiest solution:

{
  "scripts": {
    "test": "jest --runInBand --forceExit" 
  }
}

I would opt for a runInBand configuration option. We can at least add code comments to jest.config.js!

@github-actions
Copy link

github-actions bot commented Dec 6, 2022

This issue is stale because it has been open for 1 year with no activity. Remove stale label or comment or this will be closed in 30 days.

@github-actions github-actions bot added the Stale label Dec 6, 2022
@github-actions
Copy link

github-actions bot commented Jan 5, 2023

This issue was closed because it has been stalled for 30 days with no activity. Please open a new issue if the issue is still relevant, linking to this one.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Jan 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants