Patches #73
Merged
Patches #73
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This follows-up on d56a4cd, which I accidentally passed `counter` as the third parameter to watchDirectory(), which is meant for `fn`. The consequence is that when watching directories recursively on Linux, it will call `self.on('change', fn)` multiple times. In the Travis CI output for Linux, it now says: > (node:4378) MaxListenersExceededWarning: > Possible EventEmitter memory leak detected. > 11 change listeners added. This is not a failure, but it is a good warning that helped expose this bug.
* Some tests assert inside the watch callback, and some tests
assert outside the watch callback.
Asserting from the outside, I think, makes the test easier to
understand when there is a failure. Asserting from inside the
watch callback, it can cause a confusing error if it gets called
multiple times (due to it calling 'done()' multiple times,
which Mocha does not allow).
* Use assert.equal(a, b) instead of assert(a === b) so that
Mocha can report in the command-line what "actual" value is.
* Use on('ready') inside the tests to avoid the delay of 200ms.
* Use on('close', done) inside beforeEach().
* Use delay:0 in most tests, except those that test the
de-duplication behaviour. And, reduce the time distance between
modifications from 300ms to 100ms so that the tests run much faster.
It seems that distances shorter than 100ms might not be detected
by fs.FSWatcher, so I did not make it smaller than that.
This is based on testing on macOS with Node 10, and testing on
Travis CI.
* Set `--slow 500` parameter.
The default in Mocha is 2000ms for error timeout,
and 75ms for "slow" (marked with red color).
The timeout is big enough, but the red color is too common
on node-watch, I think. Increase from 75ms to 500ms so that we
do not have red colors unless there is a problem.
The tests now take about 4 seconds (previously, 29s).
|
This improves a LOT !! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request is mostly the result of me making small changes while learning how node-watch works when investigating #69.
The actual bug fix I will submit separately :)
See commit messages.