Automatically rerun tests after updating snapshots.

[marcinczenko]

Environment

1. node -v: [v9.0.0]

2. npm -v: [5.5.1], yarn v1.3.2

3. npm ls react-scripts (if you haven’t ejected): [fill]

4. Operating system: [[email protected]]

Steps to Reproduce

[Pre-cond]: all tests passing.

1. Change the code so that there is an expected snapshot failing.
2. Output shows correct information that there is a snapshot failing and the editor offers replacing the snapshots with the new ones.
3. Confirm by clicking on Replace them in the Info box.
4. The info box shows Updated Snapshots. It will show in your next test run..
5. Close the Info box by clicking on Close button.
6. The OUTPUT still shows the test failure.
7. Perform Save on the file that caused the snapshot failure.

Expected Behavior

8. The relevant tests will re-run and the relevant errors will be cleared in Problems tab.

Actual Behavior

8. Instead, the OUTPUT sometimes shows: No tests found related to files changed since last commit. Press a to run all tests, or run Jest with --watchAll. In such a case the PROBLEMS tab still shows failing Snapshot test.

---

It would be nice to get an Option (e.g. via CMD-SHIFT-P) to force re-run all the tests. Otherwise it is hard to clean up the PROBLEMS tab and it is annoying. I see there is an enhancement proposed for just doing this, but this is still not optimal solution for a nice snapshot-testing flow.

A natural behaviour with snapshot testing is to re-run the relevant tests immediately after replacing snapshots. The Updated Snapshots. It will show in your next test run. confirmation is redundant and off-the flow. Also doing 'dummy' save on the file that caused the snapshot failure, hopping to trigger a test rerun is not natural (and it does not always work).

Reseting Jest Runner (Stop/Start) will clear the errors in the PROBLEMS tab, but it is also not how one would like to go through the process.

[marcinczenko]

I am working on pull-request for that. Unfortunately, the way the jest process management is done is a bit hard to follow for me. As soon when start playing with starting/restarting/stopping jest process and you want to fill in control some things needs to be simplified/corrected. Especially, making startProcess killing the currently running process when restarting is just causing confusion and the way this.forcedClose is currently used is asking for trouble. For instance, when the handler for debuggerProcessExit is invoked for the previously killed process but this.forceClose is already cleared for the more recent process that was just started. As a result we get redundant closeJest followed by startWatchMode. Using startWatchMode inside of the close handler is also making reasoning harder. There should be only one method used to start the process and this should be startProcess. There is more to it, but instead of enumerating problems I will create first a pull-request for simplifying the process management and then I will follow with two pull-requests for restarting jest after updating snapshot and I will also add pull-request for #176 as this is then no-brainer. I still need to update the tests a bit to reflect the changes.

In the meantime, you may like to check:

https://github.com/marcinczenko/vscode-jest/tree/clean-up-jest-process-management

and

https://github.com/marcinczenko/vscode-jest/tree/restart-jest-on-snapshot-update

and let me know if this ok.

[seanpoulter]

Is it possible to update the snapshots and run all tests like you're hoping to using the interactive watch mode @marcinczenko? I've been experimenting with sending commands to Jest over stdin. It'd add a tiny bit of overhead, but it'd simplify the process management quite a bit.

[orta]

Related: There's also discussion in Jest around having a JS API for running a test, which could mean that some of this process faffing could be simplified: jestjs/jest#5048 (comment)

[connectdotz]

@marcinczenko I think you are right that the process management can definitely be simplified and streamlined:

• closeJest
  this method mainly calls jestProcess.closeProcess() in jest-editor-support to work around the fact that it doesn't clean up its internal state (debugprocess) when the process exited thus prevented the start() to be called again. If you fix the jest-editor-support then we don't need to call the closeJest() at all because the process is obviously closing if debuggerProcessExit event is invoked...

• forcedClose
  this is pretty hacky I agree, so let's not piling more stuff on it... I think a better approach will be to eliminate forcedClose, maxRestart altogether and consolidate the jest process management logic into a separate class that does something more intuitive like this (pseudo code):

class TaskManager {

    shouldTakeRequest: boolean /* default false */
    maxTaskCount: number /* control max retry */
    taskCount: number
    jestProcess: Runner

    constructor(maxTaskCount = 3...){...}

    // should be called in startProcess()
    start() { this.shouldTakeRequest = true; this.taskCount = 0; this.jestProcess = Runner(...); }

    // should be called in stopProcess()
    stop() { this.shouldTakeRequest =false; this.jestProcess.closeProcess() }

   // replace all other jest run with this
    runJest(watchMode:boolean){
        if(!this.shouldTakeRequest || this.taskCount > this.maxTaskCount) return;
        this.taskCount += 1;
        this._execute(watchMode);
   }
  // could introduce delay if needed
   _execute(watchMode) { Promise.resolve(() => this.jestProcess.start(watchMode)); }
}

• invoke startWatchMode in debuggerProcessExit:
  This is so we can 1) start watch-mode after the full test-run 2) auto-restart should the process exit unexpected. Nothing wrong with that, in short, the idea is there should always be a jest process running unless explicitly stopped. Once we have done the refactoring mentioned above, it should be a lot easier to understand/reason:

this.jestProcess.on('debuggerProcessExit', () => {
    this.channel.appendLine('Closed Jest')

    // we should always have a jest running, if possible
     this.taskManager.runJest(true /* watchMode */)
}

[marcinczenko]

Thanks for the input - I will look into it a bit later - I can only be active late in the late evening on it :(.

Just a quick reply to @connectdotz. Introducing a proper separate abstraction is definitely what should be done here. So thanks for triggering that! I just took a small step so that I understand myself how it works right now.

We have to remember that Jest is not always playing very nice - e.g. it forwards both onExit and onClose events to debuggerProcessExit. So when the full test run finishes by itself, we get two debuggerProcessExit events, and we have to ignore one of them. Unfortunately, this is not happening always - in my current implementation, when I stop Jest explicitly by calling stopProcess only one event occurs. I think forcedClose was also in some way helping out to handle that. To handle that with a proper abstraction, I would propose to have two abstractions: one say JestTask to take care for initialisation and proper cleanup of the Jest process and the second TaskManager that will provide the interface for starting, stopping, restarting of JestTasks.

We also may want to think about naming - isn't process closer to reality? At least we should agree to use one term to describe this Jest process thing? Also In your proposal I see maxTaskCount and taskCount. It may give an impression that we run multiple tasks (or processes) which is not the case. We only want one to be active at the given moment (maybe with exception of updating snapshots - but I will comment on that later), so I would keep maxRetries in the naming somehow to make that clear to the reader.

I would suggest to separate the two issues: cleaning up the process management, and then doing other stuff like rerunning all tests after updating snapshots.

If you agree, I can separate the two issues. As I think I have pretty good understanding of how process work I can work on introducing the two abstractions for process management. Just let me know :).

[seanpoulter]

It may give an impression that we run multiple tasks (or processes) which is not the case.

We're only running one for now. As we look at getting test results in the editor quicker we'll probably want another Jest process. That'd help test:

• the last saved file without changing the Jest test scheduler ([Feature request] Ability to just run the last changed test file #119)
• unsaved document content (in a language service)

I would suggest to separate the two issues: cleaning up the process management, and then doing other stuff like rerunning all tests after updating snapshots.

I'm all for an issue per symptom. 👍

[connectdotz]

I just took a small step so that I understand myself how it works right now.

👍

We have to remember that Jest is not always playing very nice - e.g. it forwards both onExit and onClose events to debuggerProcessExit.

This seems like a bug on the jest-editor-support, onClose means stdin closed, it should not be confused with onExit... you can fix it either with a backward compatible solution: passing a new flag indicating which signal triggered the debuggerProcessExit, or a cleaner one: make 2 distingue methods.

I would propose to have two abstractions: one say JestTask to take care for initialisation and proper cleanup of the Jest process and the second TaskManager that will provide the interface for starting, stopping, restarting of JestTasks.

hmmm... I am not quite sure what you have in mind for JestTask... I thought we already have one, it's called Runner in jest-editor-support. I don't think vscode-jest should worry about the actual node process state, it should be managed from where it was spawned, i.e. the Runner. I suggest starting from there and see what is missing... (for example, it should reset the debugProcess in onExit()...)

regarding the naming:

@marcinczenko your naming suggestion makes sense to me for a single jest process environment. But as @seanpoulter suggested, if we ever going to move toward multiple jest runs, then maybe a more generic naming scheme will be easier to transition to. Either way, once the TaskManager refactoring is done, it should be much easier to introduce Runner pool/queue for multi-jest runs later.

[marcinczenko]

@connectdotz Perhaps I am missing something, but this looks to me to be a conscious choice:

https://github.com/facebook/jest/blob/3ea2e106635e908579630b17f84a33602a891b69/packages/jest-editor-support/src/Runner.js#L87:

this.debugprocess.on('exit', () => {
  this.emit('debuggerProcessExit');
});

https://github.com/facebook/jest/blob/3ea2e106635e908579630b17f84a33602a891b69/packages/jest-editor-support/src/Runner.js#L95:

this.debugprocess.on('close', () => {
  this.emit('debuggerProcessExit');
});

I thought we already have one, it's called Runner in jest-editor-support.

Yes, that's exactly the reason - it is external dependency that demands its own abstraction on the plugin side.

@marcinczenko your naming suggestion makes sense to me for a single jest process environment.

Hmm...was your proposal already taking into account multiple tasks? Then sorry. You were still referring to maxRuns and this is clearly not for managing multiple tasks but to shot the whole thing down when it keeps crashing. This is why I thought you are thinking about running just one process for the moment.

Anyway - I have created a separate issue for this (#189), leaving this thread to the comments only about rerunning tests after updating snapshots.

I do care to have a more stable plugin, so if you guys need some help please let me know. If you prefer to work on the TaskManager abstractions yourself, it is completely fine with me. I have plenty of work and I would prefer to avoid working on something that you do not value at the moment.

[marcinczenko]

@seanpoulter, @orta So coming back to rerunning tests after updating snapshots. Does it make sense to create the pull request for the temporary solution I have now. I am using it now, so I kind of achieved what I wanted. It adds a new config option to rerun the tests after snapshot update which is false by default (which means it does not touch the current behaviour in any way). I also refactored the process management a bit so this would have to include that as well.

We can work on a better approach in the mean time, but maybe this is already useful to some other people. What's your idea?

[connectdotz]

was your proposal already taking into account multiple tasks?

no, the current implementation is definitely for the single process/runner, that's why I was agreeing with your naming suggestion under this assumption.

[orta]

@seanpoulter, @orta So coming back to rerunning tests after updating snapshots. Does it make sense to create the pull request for the temporary solution I have now?

Sure 👍

[connectdotz]

@marcinczenko now I have a bit more time to reply: I sensed you are a bit frustrated... please don't, we are all trying to do the right thing to help our favor tool ;-). Not sure if this is the best place to comment these kind of things, but I figure it's better to address them than not...

I would prefer to avoid working on something that you do not value at the moment.

ouch, if I made you feel that way, it is my bad. It's great that you feel the code should be improved for simplicity and that's why we love fresh perspectives! I am sure there are many ways to refactor the process and it's hard to fully understand each other's idea via these comments... I just want to share the lesson we learned and balance short-term hack vs long-term fix...

Perhaps I am missing something, but this looks to me to be a conscious choice

Just because the code is out there, doesn't mean its right or unchangeable ;-) when uncertain, I will usually do the backward compatible change, so whoever needs it can get the benefit without breaking others.

Yes, that's exactly the reason - it is external dependency that demands its own abstraction on the plugin side.

I guess it all depends on what this local abstraction needs to do... If it makes more sense to happen in the external package, then we should fix it there (better encapsulation too) instead of hacking on our side, which is exactly what led us to the process management issue now... just trying to learn from our mistake.

regarding restarting the process after snapshot update: I see you are trying to call stopProcess() followed immediately by startProcess(). Given stopProcess() will not "complete" right away (like an async operation), the startProcess() should fail to keep the "single-process" mode true. Will it be better to wait after the process is fully stopped before invoking startProcess()? What do you think if we use a promise (or something like that), resolved by debuggerProcessExit, to trigger the startProcess()...? I could be wrong but I think you might not even need to touch the forcedClose or any other part of the system if we can truly serialize the stop/start sequence...

[marcinczenko]

@connectdotz I moved discussion on this to #189. I will replay to your comments there.