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

Abort previous run on --watch change. #1100

Merged
merged 2 commits into from
Jan 9, 2014

Conversation

benjie
Copy link
Contributor

@benjie benjie commented Jan 4, 2014

Currently when a file change is detected the previous test suite continues running and a new one starts in parallel. This can cause issues with the context sharing issue fixed in #1099; but I also suspect it's not the desired behaviour.

This change aborts the current test suite before starting the fresh one.

(An alternative option would be to queue a new test suite run after the current one completes - I am not sure which is the more desirable option; I think running the fresh tests sooner probably wins from the point of view of giving the programmer up to date feedback?)

@azweb76
Copy link

azweb76 commented Jan 6, 2014

Great change. Please accept pull.

@travisjeffery
Copy link
Contributor

@benjie do you have an example of an issue that occurs with this and #1099?

@benjie
Copy link
Contributor Author

benjie commented Jan 6, 2014

@travisjeffery
Copy link
Contributor

thanks.

that's passing every time for me, which is expected behavior - no?

btw, i agree that queueing makes more sense than running them in parallel, and queuing them makes less sense than starting the new run immediately.

@benjie
Copy link
Contributor Author

benjie commented Jan 6, 2014

Added a README with instructions; note the behaviour is only exhibited without the #1099 fix (which was merged) so make sure you're using the release version of 1.16.2 (I had to npm install mocha@1.16.2 to reproduce it as I have my fixed version installed globally).

https://github.com/benjie/mocha-issue-1100

I've written the code to do proper queuing as abort() is not synchronous; will push it a little later today.

@benjie
Copy link
Contributor Author

benjie commented Jan 6, 2014

I've also updated the test repo to have two tests to show how with this latest commit you can trigger a number of watches and it will complete the test that's currently running before moving on and starting the test suite afresh; e.g. running:

for I in `seq 1 10`; do touch test/test.js; sleep 1; done

will result in "1 passing" 5 times followed finally by "2 passing". Other than the lack of output that the test suite was aborted (e.g. "1 passing, 1 aborted") I think this is the desired behaviour. (Note: I've also decreased the delay variable from 10 seconds to 2 seconds in the test repo.)

It would be simple to add a command line option to allow the user to specify that they want the previous test run to complete fully (no runner.abort()) before starting a new one; however I don't think this is desired despite how simple it is. What are your thoughts on this?

travisjeffery added a commit that referenced this pull request Jan 9, 2014
Abort previous run on --watch change.

* benjie/watch-abort:
  Old test suite finishes up before next --watch run
  Abort previous run on --watch change.
@travisjeffery travisjeffery merged commit 0d3762d into mochajs:master Jan 9, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants