Clean up jest process management #215

marcinczenko · 2018-01-04T17:29:17Z

I added two basic abstractions to make jest process management easier to managed: JestProcess and JestProcessManager. JestProcess job is to isolate the rest of the extension from the underlying jest-editor-support. It takes care for starting and stoping of the underlying Runner from jest-editor-support. It also handles restarting if the underlying process exits unintentionally (i.e. without being explicitly stopped) and filters out redundant events for debuggerProcessExit (only first event is handled, the remaining are ignored).

JestProcessManager manages multiple instances of JestProcess. In our current setup we will always have one process running at a time, but JestProcessManager will handle multiple process as well. JestProcessManager also takes care for the watch mode when runAllTestsFirst plugin setting is true. In such a case, it will first start a JestProcess to run all the tests, and when it finishes, it will automatically create another JestProcess in the watch mode.

Both abstractions have accompanying tests which should also document the actual usage.

This pull-request, also adds possibility to restart Jest when snapshots get updated. This is controlled by the plugin setting: restartJestOnSnapshotUpdate.

orta

@seanpoulter I'll leave merging to you, looks great for me

orta · 2018-01-04T18:19:35Z

src/JestExt.ts

+            })
+            .onJestEditorSupportEvent('terminalError', (error: string) => {
+              this.channel.appendLine('\nException raised: ' + error)
+            })


very elegant 👍

orta · 2018-01-04T18:20:01Z

src/JestExt.ts

+              this.startProcess()
+              vscode.window.showInformationMessage('Updated Snapshots and restarted Jest.')
+            } else {
+              vscode.window.showInformationMessage('Updated Snapshots. It will show in your next test run.')


Lovely touch

orta · 2018-01-04T18:20:35Z

src/JestExt.ts

+  private shouldIgnoreOutput(_: string): boolean {
+    // this fails when snapshots change - to be revised - returning always false for now
+    // return text.includes('Watch Usage')
+    return false


want to do anything about this?

Yes. I work continuously with snapshots so I will keep eye on it. I have to look a bit closer on how output is being processed.

Any progress on this one @marcinczenko? I'd prefer if we kept the existing behavior that's hiding the "Watch Usage" prompt and open a new issue to fix the problem when the snapshots change. Want me to see if we can create a test case for it?

No, no really progress on it. I was a bit idle waiting for this PR to be finished and merged. It takes very long time and it is a bit demotivating so I started to put my attention somewhere else for a moment. I also wasn't sure what will be the effect of some other investigations on how to communicate with Jest Process that was mentioned in some other discussions before. For this reason I did not rush up. I am less concerned with the console output, which I found buggy anyway - it seems to swallow lots of the contents because of this "Watch Usage" filtering? I was more concerned that Snapshots work. So maybe we keep snapshots working and we open the issue on filtering the output, which I think requires review anyway.

It takes very long time and it is a bit demotivating

Yea, sorry for the delay. Life got busy, and when I did find some time it wasn't enough to get through a PR of this size. It's even slower when you aren't familiar with the new or old code, and we don't have integration tests to make sure we've preserved behavior.

I am less concerned with the console output, which I found buggy anyway - it seems to swallow lots of the contents because of this "Watch Usage" filtering?

That's fair. I've reverted it and opened #228 for the filtering. We filter out the Watch Usage prompt since it's extra noise we don't need to see. It shouldn't swallow anything else.

orta · 2018-01-04T18:26:14Z

tests/JestProcessManagement/JestProcess.test.ts

+      expect(onExit).toHaveBeenCalledTimes(2)
+    })
+  })
+})


very impressive

orta · 2018-01-04T18:26:28Z

tests/JestProcessManagement/JestProcessManager.test.ts

+      expect(jestProcessMock.mock.calls[0][0]).toHaveProperty('watchMode', true)
+    })
+  })
+})


also very impressive

👍

connectdotz · 2018-01-07T13:25:37Z

src/JestExt.ts

+    this.jestProcessManager.startJestProcess({
+      watch: true,
+      keepAlive: true,
+      exitCallback: (_, jestProcessInWatchMode) => {


maybe I am missing something, this looks to me that all the jest output are only processed upon process exit... but during watch mode, the process will not exit isn't it? does the output get processed upon change?

This is good one. Actually it is other way around. I have assigned the handlers correctly for the watch mode (the exit handler for the first process - the one running all the tests first - provides two arguments: one for the process that just exited, second for the jest process for the watch mode that just started), but I did not assigned them for the first process that runs all the tests first.

I have just fixed it - please see the most recent commit.

connectdotz · 2018-01-07T13:27:35Z

src/JestProcessManagement/JestProcess.ts

+  public watchMode: boolean
+
+  private startRunner() {
+    this.exited = false


you might encounter a race condition here

startRunner is a private method. It is only called in two circumstances:

When the JestProcess instance is created (startRunner is called in the constructor).

From method restart, which is also private and called only after the process already exited. Note that restart removes all the handlers from the underlying Runner before calling startRunner.

So, no race condition as far as I can see, but please elaborate if you thought about something else.

if this.exited is only used by the closure below, just make it a local variable, it will be captured by the closure.

It is related to the other comment below. Making it part of the closure would actually indicate that we are protecting it from something at that would be misleading.

the code is cleaner when there is no state, or at least we should keep state as tight as possible, definitely avoid unnecessary state beyond its context.

Suggested change:

private startRunner() { let exited = false // here this.runner = new Runner(this.projectWorkspace) this.runner.start(this.watchMode) this.runner.on('debuggerProcessExit', () => { if (!exited) { //here exited = true // here this.onExitCallback(this) if (this.keepAlive) { this.restart() } } }) }

Ok, the argument about keeping the state to the minimum is speaking to me. I will add that.

connectdotz · 2018-01-07T13:29:34Z

src/JestProcessManagement/JestProcess.ts

+  private restart() {
+    this.runner.removeAllListeners()
+    this.startRunner()
+  }


should restart() stop the running process before starting?

No, I suppose. restart is a private method and it is called only internally when process already exited.

I see, if it is only used by startRunner() and depends on that context, maybe better off making this method an inner function within startRunner(), it will prevent accidental misuse and confusion for future maintenance.

I do not know... Isn't private not communicating that it is...private? What other people think? Could you please give an example of how you would like it to be?

it is indeed private, but not for the context of the JestProcess class, but for the startRunner() method, because it only makes sense to be invoked after the process is exited as you indicated. Similar to the comment above, we need to be tight about scope/context to avoid confusion and future maintenance overhead.

Suggested change:

private startRunner() { // move the function here or inline will be fine too function restart() { this.runner.removeAllListeners() this.startRunner() } this.exited = false this.runner = new Runner(this.projectWorkspace) this.runner.start(this.watchMode) this.runner.on('debuggerProcessExit', () => { if (!this.exited) { this.exited = true this.onExitCallback(this) if (this.keepAlive) { restart() //here } } }) }

one more question, does the keepAlive has a boundary? I have seen sometimes jest can't start, without the upper bound, will the process just keep spawning jest process until it exhausted all system resources?

In-line functions is not me, but I do not have time to argue about it. I will add your change (and close my eyes when typing :)).

Adding a boundary is a good thing. I will add that.

In-line functions is not me

I don't like nested function either, but I dislike out-of-context/running-away-state OOP more ;-)... good news is we don't need a nested function here at all, giving it is only used in 1 place and is only 2 lines, you can further simplify it:

private startRunner() { let exited = false this.runner = new Runner(this.projectWorkspace) this.runner.start(this.watchMode) this.runner.on('debuggerProcessExit', () => { if (!exited) { exited = true this.onExitCallback(this) if (this.keepAlive) { // here, even simplier... this.runner.removeAllListeners() this.startRunner() } } }) }

hopefully that will keep your eyes open when you type... ;-)

marcinczenko · 2018-01-10T21:17:14Z

Is there anything that is still blocking this from being merged? Shall I squash and make a ready to merge commit?

seanpoulter · 2018-01-10T21:39:11Z

If anyone else wants to take on the merge, feel free.
I haven't had a moment since the discussion with @connectdotz and changes from the weekend.

seanpoulter

Thanks for all the work that went into this @marcinczenko and your patience for me to find time to step through the details. It's looking pretty good, but there are a few changes I think we should address before we merge. The highest priority to me is catching when Jest has restarted too many times and getting feedback to the user. There have been quite a few issues opened from folks who can't tell what's happened.

seanpoulter · 2018-01-14T08:12:20Z

src/JestExt.ts

+  private shouldIgnoreOutput(_: string): boolean {
+    // this fails when snapshots change - to be revised - returning always false for now
+    // return text.includes('Watch Usage')
+    return false


Any progress on this one @marcinczenko? I'd prefer if we kept the existing behavior that's hiding the "Watch Usage" prompt and open a new issue to fix the problem when the snapshots change. Want me to see if we can create a test case for it?

seanpoulter · 2018-01-14T19:29:47Z

package.json

+        "jest.restartJestOnSnapshotUpdate": {
+          "description": "Restart Jest runner after updating the snapshots",
+          "type": "boolean",
+          "default": false


Should this be the default behavior @marcinczenko? Based on your description in #179 it seems like we'll always want to restart Jest and don't need the option.

I wanted this change to be not invasive so that current users do not get something they do not expect. I am open to making it a default if there is agreement.

seanpoulter · 2018-01-14T20:52:24Z

src/JestExt.ts

+              this.startProcess()
+              vscode.window.showInformationMessage('Updated Snapshots and restarted Jest.')
+            } else {
+              vscode.window.showInformationMessage('Updated Snapshots. It will show in your next test run.')


seanpoulter · 2018-01-14T23:47:05Z

src/JestProcessManagement/JestProcess.ts

+  private runner: Runner
+  private projectWorkspace: ProjectWorkspace
+  public keepAliveCounter: number
+  public onExitCallback: Function


Can we switch this to private so we don't get it confused with the onExit(callback)?

Should be possible :).

seanpoulter · 2018-01-15T01:01:06Z

src/JestProcessManagement/JestProcessManager.ts

+
+  private startJestProcessInWatchMode(exitCallback, keepAlive) {
+    return this.handleNonWatchMode(true, exitCallback, keepAlive)
+  }


I'm confused by the contradicting method names here with "watch mode" and "non-watch mode" together. What's your reasoning behind them @marcinczenko?

Oh, nothing special - just the effect of refactoring and apparently I did not realised that the naming is not really to the point. I will give it a better name.

I refactored it a bit. Do you think it is better now?

seanpoulter · 2018-01-15T01:02:28Z

src/JestProcessManagement/JestProcessManager.ts

+  }
+
+  private onJestProcessExit(jestProcess, exitCallback, keepAlive) {
+    const jestProcessInWatchMode = this.startJestProcessInWatchMode(exitCallback, keepAlive)


Should we call handleNonWatchMode inline here?

Yes, after I refactor the naming it might be better to inline it.

seanpoulter · 2018-01-15T05:58:46Z

src/JestProcessManagement/JestProcess.ts

+    this.projectWorkspace = projectWorkspace
+    this.keepAliveCounter = keepAlive ? JestProcess.keepAliveLimit : 1
+
+    this.startRunner()


How do you feel about moving this out of the constructor to separate concerns?

This one I would prefer to keep in the constructor. From my perspective it simplifies the design by removing some ugly corner cases against which I would have to perform additional checks. I like the fact that thanks to this API is very narrow and safe.

If this really feels not right for you, let me know. Then I will try to reorganise things a bit. On the other hand I am not convinced if this is right investment at the moment. There are other things I would prefer to do, and this feels really good enough. JestProcess will most probably undergo significant changes anyway when we find a better way to talk to jest.

This one I would prefer to keep in the constructor. ... There are other things I would prefer to do, and this feels really good enough.

Sure, let's leave it for now.

seanpoulter · 2018-01-16T06:06:43Z

src/JestExt.ts

-        if (maxRestart-- <= 0) {
-          console.warn('jest has been restarted too many times, please check your system')
-          status.stopped('(too many restarts)')
-          return


We're missing this behavior in the PR. When the Jest process in watch mode fails:

the status bar spinner will keep spinning with "Starting watch mode" shown

there's no annunciation on the output channel

This is good point. The user should be notified. I will add this behaviour.

Working on it - when the restart threshold has been reached the callback will be called so that we can update the channel and the status bar appropriately.

Ok, so I suggest the following. For processes in watch mode I call exit callback only if they failed for a number of times (JestProcesses.keepAliveLimit). In the response to this, I update the channel with text "Starting Jest in Watch mode failed too many times and has been stopped. Please check your system configuration." and I set the status bar to status.stopped(). This way the user will see that jest is no longer running and should start jest again. We also do not have additional callback and just use one. I will still rebase everything (if needed), and give it a spin locally today to see if nothing unexpected happens.

Hmm...there is still one issue with this. When stopping Jest explicitly, we will get a misleading message in the channel. I suppose I will need to add an argument to the stop handler to mark that it is the result of explicit user action.

Ok, maybe there is a better way to accomplish that without adding any extra argument to the handlers. Let me check.

One more small thing. Just to see how it feels, on a separate branch I created a version where JestProcess.stop returns a promise. I tested it out when updating snapshots and it feels quite nice. I remember that @connectdotz proposed more synchronous flow. If you think it is better like this, I will probably add a test for this. Let me know. Here is the link to the commit on a separate branch: marcinczenko@5727128

@seanpoulter I am running it now for two days using the version where JestProcess.stop returns a promise and it feels quite good. Never had any surprise. I will merge the "promise" version to the PR and I hope we will be able to merge it quickly (I would like to avoid another rebasing...). I could then proceed to issue #215 as this effectively prevents snapshots from working (I am testing snapshots by modifying the shouldIgnoreOutput() function).

Sounds good. I'll wrap up my own dev efforts then focus on the review. I'm pretty short on spare time these days, so sorry in advance for it taking a while. If anyone else has the bandwidth, feel free to take the lead.

seanpoulter · 2018-01-16T06:24:57Z

src/JestExt.ts

-    }
-    this.forcedClose = true
-    this.jestProcess.closeProcess()
+          this.assignHandlers(this.jestProcess)


When watch mode fails, this output is only assigned once.

You might be right. Let me investigate this.

You are right @seanpoulter. So I want to avoid adding more callbacks and taken that we already have separate JestProcess to deal with this kind of things, I will record registered callbacks and restore them when process is restarted inside JestProcess class. Feels like a right place to handle that. I will have it tomorrow.

seanpoulter · 2018-01-18T06:22:53Z

Let me know if I can help with any of those comments @marcinczenko. I'm happy to dig in and send you a PR.

seanpoulter · 2018-01-19T15:27:11Z

src/JestProcessManagement/JestProcessManager.ts

+        watch: watch,
+        keepAlive: keepAlive,
+        exitCallback: exitCallback,
+      })


Thanks @marcinczenko. That's a lot easier to read.

@seanpoulter Sorry for small delay on my side this time. The remaining changes are on the way!

Don't be sorry @marcinczenko. It's a big change! After I wrap up some integration tests for jest-editor-support, I'll try and write some integration tests for the status bar and output channel.

@seanpoulter Just added restoring jest editor support events. These are not really big changes, but to write tests always requires a bit more focus. The last thing to add is to notify the user that the allowed number of Jest process restarts has been reached. I hope to have it done by the end of Monday. Then I think we will be good to go ;).

…t of the exit handler for the jest process running all the tests

…lying runner

…f the jestProcess when it exits without being explicitly stopped

…eds revising.

…blic method

…many times

…y requested stop

stephtr · 2018-02-04T00:07:20Z

src/JestProcessManagement/JestProcess.ts

+  private startRunner() {
+    this.stopRequested = false
+    let exited = false
+    this.runner = new Runner(this.projectWorkspace)


It would be useful to create the Runner with the shell option enabled (requires jest-editor-support 22.1.3) at least on Windows in order to fix #98.

Hi @stephtr. Thanks for pointing this out. I will take a look. Hopefully, we all got some slack very soon to start review.

orta · 2018-02-12T13:10:03Z

@seanpoulter think you can get time to review this? Otherwise I'll take a serious look at it tomorrow 👍

seanpoulter · 2018-02-12T14:03:31Z

You might get to it first @orta. Just wrapping up my first sprint at a new job and have been spending my spare time getting up to speed with it.

orta

Sorry this took so long to get through @marcinczenko - you did a great job explaining your changes, and the code definitely reads much better

Great job!

code changes

seanpoulter · 2018-02-20T16:36:08Z

Hurrah! I'll second that, thanks @marcinczenko for the thorough PR. 🎉

marcinczenko · 2018-02-20T16:46:44Z

Wow! I am very glad it went through 🍿. Big thanks to @connectdotz, @orta, and @seanpoulter for taking your time reviewing it and giving feedback.

Now I can start dealing with other issues - especially those related to snapshots.

We have a bit busy time at work at the moment, but very soon I should be able to do some work.

orta · 2018-02-20T17:28:28Z

I've invited you to the org because this is a great big contribution, you're welcome to contribute as little or as much as you like - we'll be here to support you 👍

…-process-management Clean up jest process management

marcinczenko force-pushed the clean-up-jest-process-management branch from 82ac478 to beed961 Compare January 4, 2018 17:52

orta reviewed Jan 4, 2018

View reviewed changes

connectdotz reviewed Jan 7, 2018

View reviewed changes

seanpoulter previously requested changes Jan 16, 2018

View reviewed changes

seanpoulter reviewed Jan 19, 2018

View reviewed changes

seanpoulter added the state: in progress label Jan 26, 2018

marcinczenko added 19 commits January 30, 2018 14:07

Updated jest process management to make it more comprehendible

c138da2

Added restarting Jest on snapshot update

b75f7a5

Added basic wrapper around jest-editor-support Runner

6a591d1

Removed a bit of duplication from JestProcess tests

d567591

Added watchMode argument to the JestProcess constructor

56cd93f

Added JestProcessManager that supports non-watch mode only

0ce0768

Added support for watchMode

9551f6a

Passing jestProcess representing the watch mode as the second argumen…

6d1a973

…t of the exit handler for the jest process running all the tests

JestProcessManager stores jest processes in array

c6d2a70

Added stop method to JestProcess that calls closeProcess on the under…

07a6e16

…lying runner

Updated jest in devDependencies

5bb442e

Jest Process manager allows stopping the most recent jest process

dc9b3f7

Removing references to jest processes that exited on its own

8f7e228

Added keepAlive flag to jestProcess to control automatic restarting o…

ef183b6

…f the jestProcess when it exits without being explicitly stopped

Added forwarding call to runJestWithUpdateForSnapshots.

ebacfcb

Updated JestExt tests

11fa686

New process management integrated with JestExt.

b04be74

shouldIgnoreOutput brought back, but always returns false for now. Ne…

ed8a5b1

…eds revising.

Checked with linter

5e5bca1

marcinczenko and others added 6 commits January 30, 2018 14:17

Added limit to keepAlive

b6de8cc

Revert change to shouldIgnoreOutput()

265240a

onExitCaalback member made private to remove confusion with onExit pu…

4e7da5a

…blic method

Refactored to make naming more precise

8a59c89

Added restoring jest editor support events when keepAlive is true

9c8ecfe

Updated how user is notified when process in watch mode restarts too …

67d1309

…many times

marcinczenko force-pushed the clean-up-jest-process-management branch from c8f0324 to 67d1309 Compare January 30, 2018 13:19

marcinczenko added 3 commits January 30, 2018 16:16

Added stopRequested flag to distinguish between failure and explicitl…

4256eae

…y requested stop

restoring jest events before starting runner

dd97170

Calling testsHaveStartedRunning from one place only

6714a90

marcinczenko force-pushed the clean-up-jest-process-management branch from b44f8f4 to 6714a90 Compare January 30, 2018 19:19

JestProcess.stop returns promise to support more sequential flow

5727128

seanpoulter mentioned this pull request Feb 2, 2018

Switching branches sometimes breaks watcher #246

Closed

stephtr mentioned this pull request Feb 3, 2018

Spawn Jest command in separate shell on Windows #248

Merged

stephtr reviewed Feb 4, 2018

View reviewed changes

stephtr mentioned this pull request Feb 4, 2018

Plugin not reading jest.config.json file even when settings are updated #223

Closed

seanpoulter mentioned this pull request Feb 20, 2018

[Feature request] Ability to just run the last changed test file #119

Closed

orta approved these changes Feb 20, 2018

View reviewed changes

Merge branch 'master' into clean-up-jest-process-management

8479a21

orta merged commit ae6be66 into jest-community:master Feb 20, 2018

seanpoulter removed the state: in progress label Feb 20, 2018

This was referenced Apr 11, 2019

Update failing snapshots [HOW TO] #276

Open

How to update outdated snapshots or generate new ones? #368

Open

legend1202 pushed a commit to legend1202/vscode-jest that referenced this pull request Jun 18, 2023

Merge pull request jest-community#215 from marcinczenko/clean-up-jest…

a9d8fdc

…-process-management Clean up jest process management

Clean up jest process management #215

Clean up jest process management #215

Conversation

marcinczenko commented Jan 4, 2018 • edited Loading

orta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcinczenko commented Jan 10, 2018 • edited Loading

seanpoulter commented Jan 10, 2018

seanpoulter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcinczenko Jan 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanpoulter commented Jan 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcinczenko Jan 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orta commented Feb 12, 2018

seanpoulter commented Feb 12, 2018

orta left a comment

Choose a reason for hiding this comment

seanpoulter commented Feb 20, 2018

marcinczenko commented Feb 20, 2018

orta commented Feb 20, 2018

marcinczenko commented Jan 4, 2018 •

edited

Loading

marcinczenko commented Jan 10, 2018 •

edited

Loading

marcinczenko Jan 30, 2018 •

edited

Loading

seanpoulter commented Jan 18, 2018 •

edited

Loading

marcinczenko Jan 29, 2018 •

edited

Loading