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

Ext won't refresh after code change #308

Closed
garyking opened this issue Apr 24, 2018 · 14 comments · Fixed by #674
Closed

Ext won't refresh after code change #308

garyking opened this issue Apr 24, 2018 · 14 comments · Fixed by #674
Assignees

Comments

@garyking
Copy link
Contributor

I purposefully broke one of my tested methods, and this ext gives me an error. I undo my changes, and the ext still gives me an error.

Is there any way to force a refresh of the test results so that the error disappears, after it's been fixed?

@benhamlin
Copy link

I believe this is just a shortcoming of Jest itself - it determines what tests to run based off differences from the last commit. So if you add in a throw new Error('barf'), it will run the test, but when you delete that line, the file is identical to HEAD and so no test is run.

You can verify this by running jest in a separate console, the same behavior occurs.

Hopefully this doesn't actually occur very often in practice.

@garyking
Copy link
Contributor Author

Oh okay thanks for the explanation. I assume that this ext runs Jest with the -o flag then?

@seanpoulter seanpoulter reopened this Apr 27, 2018
@seanpoulter seanpoulter self-assigned this Apr 27, 2018
@seanpoulter seanpoulter added bug topic: core Running tests and showing results labels Apr 27, 2018
@seanpoulter
Copy link
Member

Yes @garyking , jest is called with the --watch CLI option which will run for only the changed files (-o). Am I right to assume that you've been running your tests in a git or hg repo?

I've re-opened the issue so we can double check @benhamlin's hypothesis. 😉

@garyking
Copy link
Contributor Author

Yep I've been testing this in a Git repo.

@abner
Copy link
Contributor

abner commented May 14, 2018

i think it was fixed in PR #325

@donaldpipowitch
Copy link

donaldpipowitch commented Aug 1, 2018

Same here. I tried to manually set the path to jest with --watchAll and the behaviour changed, but it hasn't really worked well. Even though the initial tests finished after some seconds the runner still said it was running for example.

Maye vscode-jest could "focus" opened files (basically like $ jest unit --watch --testPathPattern path/to/my/test.js). When I run this, add an error and then undo my change Jest correctly reports that my test is working again.

@connectdotz
Copy link
Collaborator

first, we need to decide if it is a jest or the plugin issue... if jest wasn't able to detect the change, there is not much this plugin can do, you will be better off researching from the jest side.

You can perform the following steps and let us know what you see:

  1. remove the custom options and turn on the debugMode to see what command was used to launch jest
  2. check if jest detected the change: If jest detect the change, it will rerun the test and you should see them in the jest OUTPUT channel.

Please also make sure you have the latest vscode-jest version 2.9.0.

@donaldpipowitch
Copy link

I guess depending on how you look it is neither a Jest nor a Plugin issue 😄As far as I can tell this plugin basically calls jest --watch and this is what happens:

What Jest does on jest --watch:

  1. Jest starts the runner. Nothing is reported.
  2. When I change a test so that it fails Jest reports a failing test.
  3. When I revert my change Jest says "No tests found related to files changed since last commit." and nothing is reported.

What vscode-jest does on start (as far as I can tell):

  1. The Jest runner is started. Depending on your "Run all tests first" setting there will be nothing reported or the state of your current tests.
  2. When I change a test so that it fails Jest reports a failing test and so does vscode-jest.
  3. When I revert my change Jest says "No tests found related to files changed since last commit." and nothing is reported from Jest, but vscode-jest keeps the error message from step 2.

So while it is maybe technically correct that vscode-jest doesn't report a successful test, because Jest also doesn't report a successful test here one could also say that it isn't correct, because vscode-jest reports a failing test while Jest reports nothing. It is right that both tools don't run the test again on "undo", but both tools show different results.

I personally would like to use a different API from Jest. When I run jest --watch I can press p to filter by a filename regex pattern. Then my file (or files) become focused and than Jest reports the "undo" step as a successful test. vscode-jest could call the same API based on some conditions like open filed, saved change to file system and so on.


Nevertheless here are the things you asked for:

  1. Shows [Extension Host] spawning process with command=/Users/.../node_modules/.bin/jest, args=--json,--useStderr,--outputFile,/var/folders/47/bh5ywmg14rn1m1pskv1yn5r00000gn/T/jest_runner.json and [Extension Host] spawning process with command=/Users/.../node_modules/.bin/jest, args=--json,--useStderr,--outputFile,/var/folders/47/bh5ywmg14rn1m1pskv1yn5r00000gn/T/jest_runner.json,--watch.
  2. I changed a test so that it should fail and it was detected. I reverted my change and got No tests found related to files changed since last commit. from and the error message wasn't removed by vscode-jest. As expected.

@connectdotz
Copy link
Collaborator

@donaldpipowitch thanks for the info.

Looks like the plugin is behaving as it is designed, i.e. this is not a bug. However, it doesn't mean that the plugin can't be improved...

it would be nice to "force" a test run in watch mode... I remember @seanpoulter was looking to make the jest child_process interactive so we can utilize the watch mode commands. It seems like a great idea that can potentially address this common ask...

@tkivela
Copy link

tkivela commented Aug 25, 2018

Stumbled into this same problem today.

I would say it's a bug, because the plugin does not clean the problem markers from VSCode correctly when doing incremental Jest run.

One way to look at it would be:
-Having a code without errors results no errors when running jest from command line
-Having exactly same code within VSCode might have jest errors (on problems view) or it might report no errors depending if the procedure outlined by @donaldpipowitch has been done or not.

I think the problem arises from not clearing the problem markers before doing incremental jest run. On first glance I would think the code which has this logic fail is in onDidChangeTextDocument(event: vscode.TextDocumentChangeEvent) -method of the extension.

One way currently to clear the markers is to run "Jest: Stop Runner" and then "Jest: Start Runner" which clears the false problem markers.

@connectdotz connectdotz added help wanted and removed bug topic: core Running tests and showing results labels Aug 26, 2018
@connectdotz
Copy link
Collaborator

vscode-jest works like this:

  1. it starts up by doing the full test run to get the test results for every test in the project
  2. then it enters the jest watch mode to incrementally update test status as jest reported.

this plugin doesn't use vscode to detect file change, it defers to jest's mechanism.

jest watch mode will only re-run tests for the files it considers "changed". I hope you can see that when a file doesn't show up in jest watch mode report, it doesn't mean that it is failed, only that it hasn't changed (as far as jest concern), therefore vscode-jest will not update the test status for these unchanged files. Just imagine if we adopt your logic: that any file not shown in the watch mode report would be considered "error" (or should it be "success"?), then all the non-changed file would be marked either error or success based on this assumption... is that really right?

It is a bug if jest reported explicit test error/success but this plugin failed to reflect it, which is NOT the case for @donaldpipowitch. @tkivela you can go through the same debug process mentioned above to determine if your use case is different.

I know you all wish that this plugin can correctly show your test status, but as you can see the root cause is in jest who sometimes has problem detecting changes... for the greater good of the community, I encourage you guys to file an issue on jest so every jest user, with or without vscode-jest, can benefit from the fix.

Also as I mentioned earlier, there is a potential enhancement to lessen your pain meanwhile: by connecting this plugin to jest's interactive mode so you can force jest to re-run the changed tests should it fail to detect. We look forward to such PR...

@tkivela
Copy link

tkivela commented Aug 26, 2018

No pain using the plugin, it's great and a pleasure to use @connectdotz :)

Jest works fine and reports errors correctly in this case, the problem is not in Jest.

The logic for this extension should in my opinion go as:
-If plugin has reported Jest error(s) in "foofile" and in a new run jest runs tests for "foofile", then plugin should first clear all old error reports concerning "foofile" before adding new ones.

This logic would handle all the cases correctly both when no changes to files reported by jest and also cases where code was correct or had test errors. No need to make more complex logic, just a simple cleanup of previous runs reports.

@connectdotz
Copy link
Collaborator

connectdotz commented Aug 26, 2018

Jest works fine and reports errors correctly in this case, the problem is not in Jest.

if jest does reports error then your use case is different than @donaldpipowitch, which shows "No tests found related to files changed since last commit."

-If plugin has reported Jest error(s) in "foofile" and in a new run jest runs tests for "foofile", then plugin should first clear all old error reports concerning "foofile" before adding new ones.

this plugin doesn't control which tests jest should run. When jest decides to run tests, we got callback with its output. the logic is dead simple: If the output contains test status for "foofile", we will update ours accordingly; if the output does not contain "foofile", we will not do anything about "foofile".

In the case when jest failed to detect "foofile" change and reported "No tests found related to files changed since last commit.", (again, keep in mind we don't track what files have changed and should be re-run by jest, jest did), what status should be cleared? everything? On the other hand, if jest did explicitly report "foofile" test status and we fail to update, then it is a bug and we should definitely address it, but that is not what we discovered in this thread.

I think we are going around the circle, maybe I misunderstood your issue completely, feel free to create your own issue with more detail and a sample repo, or if you have some idea on how to optimize this plugin, we always welcome PR...

@connectdotz
Copy link
Collaborator

For people with problems with watchman or "jest watch", there is an alternative now: we have added a new "interactive mode" independent of the watchman in the latest v4.0.0-alpha.5, please feel free to give it a try and let us know if it works for you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants