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

introduce interactive mode and new process model #674

Merged
merged 6 commits into from
Apr 3, 2021

Conversation

connectdotz
Copy link
Collaborator

@connectdotz connectdotz commented Mar 5, 2021

part of the v4 initiative...


This PR introduces a new "interactive" mode that allows users to control when and what tests should be run, in addition to the current "watch" mode where jest/watcher will run the related tests whenever relevant files changes. This is a common pain point for projects with many tests and or a single source file change can trigger many test re-run, which can all cause high CPU, especially during the development phase.

While working on this change, needed to refactor our process management model that we have wanted to do for a long time. Replaced the nested process model with a queue-based model, so no more keep-alive, callback, and assumption of the test sequence, etc.

Another significant change is to split the JestExt into multiple files and move to a new directory. This helps better componentization and better test coverage. Hopefully will make future change easier as well.

The new process model added a few benefits that should make the extension smarter and liter not only for the interactive but the watch mode as well, such that we know what are test files without doing the full test run, and we can update snapshots without restarting the jest process.

The last but not least, I think this PR is moving us closer to support the vscode's test API proposal, which is an interactive test-item-driven provider framework. We still have some work to do and the API is not finalized yet, but it is clear that we do need to support an interactive mode anyway.

Change Outline

  • interactive mode

    • added a new setting jest.autoRun to let users control when the extension should run tests automatically. This supports:
      1. the current "watcher" based test run. e.g.: "jest.autoRun": {"watch": true, "onStartup": ["all-tests"]}
      2. vscode file-change-event driven test run that can either run on only test file save or both test and source files. e.g.: "jest.autoRun": {"watch": false, "onSave": "test-file"} or "jest.autoRun": {"watch": false, "onSave": "any-file", "onStartup": ["all-tests"]}
      3. the full manual mode where users trigger a test run via command or context-menu. e.g.: {"jest.autoRun": "off"}
    • user can trigger running the related tests for the current file from the editor context-menu:Jest: Run Related Tests. This menu will only appear in interactive mode and for both test and source files.
    • the coverage mode will obey the same autoRun scope.
    • The extension will invalidate the stale test result when the test file is changed but has not yet been run, or mark the project/workspace test stats are out-of-sync if the source/test file is changed but tests have not been run.
  • commands

    • added new commands for document-based jest commands, such as running a give test file, run all tests in the active workspace, toggle watch for the active workspace, etc.
    • remove the "Restart Runner" command. The "Start Runner" commands will restart if JestExt is already running.
    • adopt a more structured command id scheme to reflect the scope of the command.
  • new context menu

    • added a context menu in the editor for running all tests in the file, which will only be available for non-watch interactive mode.
  • new keyboard shortcut

    • for the context menu
  • StatusBar

    • show workspace stats (success, fail, and unknown) for single and multi-root projects.
    • show autoRun mode
    • show workspace stats out of sync indicator when there are code changes that have not triggered related tests run.
  • editor visual enhancement

    • remove debug code lens when the document is changed but tests have not been run to avoid interfering with the coding activity.
    • mark all tests unknown after the test file is saved but tests have not been run to accurately reflect the state of the tests.
  • Snapshot enhancement

  • refactor jest process management

    • removed any inter-process dependency, i.e. move from the nested process structure (all-tests followed by a watch run) to a queue-schedule model, which removed all the keepAlive, onExit callback, and other complicated state management logic.
    • removed any assumption about how the jest process should be started, it simply executes whatever is in the queue, which is scheduled by JestExt based on the jest.autoRun.
    • It supports both synchronous execution like before, i.e. watch won't start until all-test run completed; as well as concurrent runs so users can schedule updateSnapshot or other single file test run without blocked by a watch or other test runs.
    • make JestProcess fully promise-based, the start and stop will return a promise that enables non-listener based interactions
    • support many more types of jest processes, such as getting all test files with "--listFiles", or running any test with --updateSnapshot flag.
  • refactor jest run handling / JestExt

    • split JestExt.ts into multiple files and move to a new folder.
    • introduce a ProcessSession concept to explicitly bound test results/cache lifecycle.
    • take advantage of the new knowledge of the test files for stats tracking and reduce unnecessary test parsing.

Open Questions

  • debugCodeLens was using the file's basename for debugging, which could potentially cause multiple files being executed if they share the same file-name (under different directories). Don't see why we should not use the full file name, so I switched to the full fileName in the PR. Please let me know if there is a legit reason otherwise.
  • we used to re-register all the JestExt in ExtensionManager when config changed. This could lead to memory leaks as the previously registered JestExt were not being "deactivated". I did not see why they have to be registered again, so removed that logic altogether. Please let me know if there is a legit reason otherwise.
  • the new ProjectManager's non-blocking queue allows a max of 3 concurrent processes. Right now only the "onSave" autoRun uses this queue, so unless users do "save all", there should not be many concurrent use cases. I didn't make this configurable but if there is some performance issue, we could easily make it configurable.

Testing

Added quite a bit of test along the way, which served as a live-testing for this PR. I did like the interactive mode with autoRun on test file save. Even though it starts a new process for every test file saves, at least on my mac, it still feels snappy compared to the watch mode. The big difference comes in when I change the source file like JestExt.ts, which means triggering 25 test files to be executed, not surprisingly a lot slower than just run JestExt.test.ts with interactive mode. But would be interested to see if how it performs in other OS/machine/projects...

Anyway, I am sure we need more testing and so probably earlier we can get people to start playing with it the better. I hope to cut a jest-editor-support release in the next couple of days so reviewers can run the actual extension during the review.

ToDo


resolves #613
resolves #668
resolves #119
resolves #308
resolves #176
resolves #408
resolves #530
resolves #36
resolves #574

@coveralls
Copy link

coveralls commented Mar 5, 2021

Pull Request Test Coverage Report for Build 903

  • 770 of 783 (98.34%) changed or added relevant lines in 19 files are covered.
  • 3 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+3.9%) to 94.078%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/JestExt/core.ts 212 213 99.53%
src/JestExt/process-session.ts 44 45 97.78%
src/JestProcessManagement/JestProcessManager.ts 59 60 98.33%
src/TestResults/TestResultProvider.ts 62 63 98.41%
src/DebugCodeLens/DebugCodeLensProvider.ts 3 5 60.0%
src/StatusBar.ts 58 61 95.08%
src/JestExt/process-listeners.ts 102 106 96.23%
Files with Coverage Reduction New Missed Lines %
src/StatusBar.ts 1 92.73%
src/helpers.ts 2 92.08%
Totals Coverage Status
Change from base Build 895: 3.9%
Covered Lines: 1564
Relevant Lines: 1641

💛 - Coveralls

@connectdotz connectdotz mentioned this pull request Mar 7, 2021
15 tasks
@connectdotz
Copy link
Collaborator Author

I know this is a HUGE PR, sorry about that, trying to take advantage of the alpha status for this scale of changes. It is probably not feasible for one person to review everything, so I tagged a few reviewers. Please feel free to do a partial review for the part that you are most familiar with or interested in.

In addition to code, I also made a pretty significant change to the README, would appreciate some editorial review and feedback.

@marcinczenko I think you are pretty familiar with the process-management, do you mind taking a look in that area? @stephtr would you mind looking at the JestExt and extension management? @orta and @seanpoulter feel free to jump in if you have some bandwidth...

@@ -39,10 +52,6 @@ A comprehensive experience when using [Facebook's Jest](https://github.com/faceb

<img src="https://github.com/jest-community/vscode-jest/raw/master/images/vscode-jest.gif" alt="Screenshot of the tool" width="100%">

## Maintainers

Orta Therox ([@orta](https://github.com/orta)), Vincent Voyer ([@vvo](https://github.com/vvo)) & ConnectDotz ([@connectdotz](https://github.com/connectdotz)).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, you should switch this to you 👍🏻

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just happened to have some freecycle recently so decided to take on these big-ticket items. Unfortunately, I didn't set a good example for the more "progressive" PR, but I think this kind of change is probably not a good fit for that. Nevertheless, I probably should have open a discussion thread before pulling the trigger... Please do feel free to voice your opinions, PR is not merged in, it's not too late.

Ok, now to the actual point, I think we need more maintainers, not less, so we can have more people bouncing ideas and widening perspectives. How do you decide the maintainers? Do you have any suggestions on who else we can add? I know @stephtr is busy recently, but he has been very helpful in our recent PRs, do you think we could add him as the maintainer?

IMO, you should switch this to you 👍🏻

You mentioned this couple of times, I might change my mind in the future, but right now I quite enjoy you being the owner so I have somebody to discuss and learn from 😄

@marcinczenko
Copy link
Member

@connectdotz I will take a look. Most probably tomorrow.

@marcinczenko
Copy link
Member

@connectdotz I could not find much time last week. Started today. I had to refresh myself a bit over the overall structure as I forgot most of the stuff ;). This is what I will be doing next: I am looking at processes-related stuff to see if I spot something that I feel needs some attention - not really deep - more in the spirit "can I follow it and does it make sense". I will also run it on some projects to see if I hit any troubles. I will try to reserve like an hour a day in the next week or so. I may also have some suggestions for the README.md.

In general the proposal looks good and it should improve the overall developer experience. In case I would be blocking anything please let me know.

@connectdotz
Copy link
Collaborator Author

@marcinczenko sounds good! Do what you can, any extra test and eyeballs is a plus, appreciate it! 🙏

Aiming to start alpha testing next week if possible, let's see what we can get through before the end of the week...

connectdotz referenced this pull request Mar 17, 2021
* use Map to store jest instances

* remove enbledWorkspaceFolders, update README, handle disabledWorkspaceFolders change on the fly

* review fixes

* Fix typo
@connectdotz
Copy link
Collaborator Author

@marcinczenko did you get a chance to test or review this PR further?

@connectdotz
Copy link
Collaborator Author

@orta I updated README to address your comment and tidy it up a bit.

Looks like we are not going to get more testing or review by leaving the PR here... what do you think we should do? Should we merge it in and cut an alpha release so we can get a bigger community to try it?

@marcinczenko
Copy link
Member

marcinczenko commented Mar 28, 2021

@marcinczenko did you get a chance to test or review this PR further?

@connectdotz Yes, but it is going slow. I would explain myself better with a DM, but I do not know if my "whys" really matter. Shortly, I still need some time. Sorry about that... I like your comment about an alpha or a "canary" release so that we can win some time but already provide it for those who already want to give it a spin.

@orta
Copy link
Member

orta commented Mar 29, 2021

The vscode marketplace doesn't really allow for separate versions of the same extension without having it as an entirely different app. You could se tup nightlies if you wanted, I did that for Svelte's vscode extension: https://github.com/sveltejs/language-tools/blob/master/.github/workflows/Deploy.yml

I'd double check that you're fine with it then ship it, and expect bug reports. It's OSS and in your spare time.

@connectdotz
Copy link
Collaborator Author

@orta, thanks for sharing the idea! 👍

I'd double check that you're fine with it then ship it, and expect bug reports. It's OSS and in your spare time.

True, I don't think it is ready for production yet, but keeping it here also does not get us closer to the finish line... Ideally, it would be better for people in this thread to test run it before open to the public. But it looks like everybody is super busy; therefore, I think a "private" alpha release might be an acceptable alternative to try moving forward.

Let's do this: I will cut an alpha-5 release, including this PR and the wizard, but won't publish to the marketplace; announce it in the related issue threads to hopefully get more people testing it (it seems to work out ok with prior alpha releases).

Meanwhile, @marcinczenko, thanks for trying to find time to review 👍 . I know how hard it is sometimes... Please continue and do not hesitate to ask any questions. After merging, this PR might be automatically closed, but you should still be able to review it. I will look out for your comments.

@connectdotz connectdotz merged commit d50d127 into jest-community:master Apr 3, 2021
@connectdotz connectdotz deleted the interactive-mode branch April 3, 2021 20:07
@marcinczenko
Copy link
Member

@connectdotz Thanks for your understanding and patience. I will do my best.

legend1202 pushed a commit to legend1202/vscode-jest that referenced this pull request Jun 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment