-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
feat(watcher): Debounce autoWatchBatchDelay #2562
feat(watcher): Debounce autoWatchBatchDelay #2562
Conversation
4e4cdb0
to
0b99191
Compare
Thanks @danielcompton, I agree this change makes sense. There are probably some tests that need updating after this change. |
Yeah there's some tests that fail, and I still need to verify the behaviour is correct. |
(Probably also adding more tests) |
I think all that is needed here is to just adjust the delay in the tests to bounce it back a little further. |
0b99191
to
5ae3254
Compare
Thanks for the tip, you're right that did fix those tests. I modified the 'batch interval' test block to test the debouncing, and it seems like that isn't working correctly. I'm not sure if this is because the tests need to mock more of lodash, or if there is something else going on? I'm at about the limits of my JS knowledge, if you can take a quick look to see if there's something obvious I'm not doing, I'd appreciate it :) |
lib/file-list.js
Outdated
// to avoid spamming the listener. | ||
function emit () { | ||
self._emitter.emit('file_list_modified', self.files) | ||
} | ||
var throttledEmit = _.throttle(emit, self._batchInterval, {leading: false}) | ||
var debouncedEmit = _.debounce(emit, self._batchInterval, {leading: false}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think {leading: false}
needs to be passed into the function here - debouncing never fires the leading call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, fixed.
Not sure if this helps you, but in another PR for upgrading dependencies (including lodash), I had to make this adjustment: https://github.com/wesleycho/karma/blob/c80a3161dd4c345be2ec00c6691811b5dc9b2a5d/test/unit/file-list.spec.js#L770-L805 |
64ee461
to
7d906f7
Compare
Phew! I think I've got it all. In the old version, the tests were passing, but they weren't testing what they seemed to be testing.
In my testing for this feature, I extended the test with more calls to modify and change files to extend the debouncing period. However because the promises weren't being waited on, they didn't run until after the test block had finished. This lead to very confusing behaviour where removing a file made the debouncing test work, but changing or adding a file doesn't. I've converted the tests to wait on the promises and added more tests, and they all pass. |
00d9274
to
1aff005
Compare
- renamed batchInterval to autoWatchBatchDelay to aid in greppability. - Modified debouncing tests to wait on promises. - Added documentation explaining how list.removeFile is different from list.addFile and list.changeFile. Closes karma-runner#2331
1aff005
to
2f8c049
Compare
This is ready for review now. |
@wesleycho would you able to take another review please? |
Thanks a lot for getting through this and especially updating the tests to correctly test what they should test! |
I'm opening this up for feedback early, to make sure I'm on the right track here. My intention is to change the behaviour of
autoWatchBatchDelay
to debounce file change events, and not run the tests until at leastautoWatchBatchDelay
ms have elapsed since the last file change.Closes #2331