search

jest-community / jest-editor-support

A module for handling editor to jest integration
MIT License
28 stars 21 forks source link

Add --bail and --runInBand #26

Closed jmarceli closed 4 years ago

jmarceli commented 4 years ago

Using --bail and --runInBand may sometimes improve DX and possibly mitigate performance issues. It might be the first approach to address issues like https://github.com/jest-community/vscode-jest/issues/455

stephtr commented 4 years ago

First of all thanks for this PR! As @connectdotz already mentioned in #455, we should think about just adding flags to jest-editor-support, which will be used by vscode-jest (or similar tools). For bail I currently can't see the use case, since in VS Code at least I would like to have an overview over which test is passing and which one is failing. If we would abort Jest at the first failing test, we wouldn't have any information available about the remaining ones. Besides that, it only shortens runtime/CPU consumption when having failing tests. In contrast to that runInBand would make sense for me to include, since it apparently can speed up jest in resource limited environments. Since it depends on the environment and not on the individual project, I would be also in favor of adding a setting to vscode-jest controlling (provided that the performance boost is noticeable for a larger audience) this flag.

connectdotz commented 4 years ago

@seanpoulter nice to see you back!

Regarding this issue: my gut feeling is no need to add these flags because:

  1. users are already able to customize with these flags users can easily achieve the same effect, and maybe more, by just add these flags in their jest.pathToJest or package.json script.

  2. it doesn't save any user effort Unless we want all users to run with these flags, otherwise, If the user had to customize their env to enable this behavior anyway, wouldn't it be just as easy to customize their cmd line as described above?

  3. it may or may not address the performance issue Why stop at --runInBand? How about --maxWorkers and maybe more? The good news is we don't have to stand in the way! Users can do all these today with the customization mentioned above.

seanpoulter commented 4 years ago

@seanpoulter nice to see you back!

Nice to see you two too! My apologies for not having more time to contribute. I don't get to work with Jest or the VS Code extension much at all.

--

I'll admit my initial reaction was pretty similar to you @connectdotz. It seems strange that we're so selective with what we've included in our wrapper/facade here. I can't think of a use case where I'd want to support another copy of the API or the Jest documentation here or in any caller. 😓

Do we close it until we have the performance warning resolved?

stephtr commented 4 years ago

users are already able to customize with these flags

In my opinion the main issue with jest.pathToJest is that it isn't possible to separate between flags which are set on a project basis and flags that are set on a machine basis. For example let's assume that pathToJest is set on a project basis, there is no way to append --maxWorkers on old, slow machines. One could append it to the project's pathToJest settings, but that would also affect fast machines when working on the project.

jmarceli commented 4 years ago

Hi, I can agree that those new flags are not required if we can address them using jest.pathToJest (I didn't think about that).

Regarding performance, it would be nice if anyone else can try running their test suites with and without --runInBand flag. In every project and on every machine I can run my tests (including CI servers) it was always faster to run tests with --runInBand flag. It would be nice if you can then look at https://github.com/jest-community/vscode-jest/issues/455 which is an issue connected with the performance of vscode-jest.

seanpoulter commented 4 years ago

In every project and on every machine I can run my tests (including CI servers) it was always faster to run tests with --runInBand flag

Wow, that's huge performance feedback @jmarceli!


It would be nice if you can then look at jest-community/vscode-jest#455 which is an issue connected with the performance of vscode-jest.

It sounds like there'd be value in some A/B testing and something that the Jest team may have already investigated. @thymikee confirms your observation that --runInBand is suggested for CI deployments in facebook/jest#3765, especially when tests are running with more than one version of Node. I'm curious if anyone's already found a way to optimize the number of workers.

After some brief searching:

There's a lot of valuable points from their conversation:

I think we've got a hypothesis or two that we could test. It'd be lovely if we could run some A/B tests and collect performance data from our users to make an informed decision about it. 🤔