feat(watcher): Debounce autoWatchBatchDelay

[danielcompton]

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 least autoWatchBatchDelay ms have elapsed since the last file change.

Closes #2331

[dignifiedquire]

Thanks @danielcompton, I agree this change makes sense. There are probably some tests that need updating after this change.

[danielcompton]

Yeah there's some tests that fail, and I still need to verify the behaviour is correct.

[danielcompton]

(Probably also adding more tests)

[wesleycho]

I think all that is needed here is to just adjust the delay in the tests to bounce it back a little further.

[danielcompton]

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 :)

[wesleycho]

I don't think {leading: false} needs to be passed into the function here - debouncing never fires the leading call.

[danielcompton]

Thanks, fixed.

[wesleycho]

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

[danielcompton]

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.

list.removeFile, list.changeFile, and list.addFile all return promises and call self._emitModified when a file modification is accepted as valid. There is an important difference in behaviour between list.removeFile and the other two though. Due to the promise structure of list.removeFile, self._emitModified is called synchronously. The test code was calling all of these functions one after another without waiting on the result. The only function call that had any effect on the debouncing function were the list.removeFile calls. The other two still called self._emitModified, but only after exiting the test scope. You can verify this by commenting out the two list.removeFile calls, and watch the test fail.

        list.changeFile('/some/b.js')
        // list.removeFile('/some/a.js') // /some/b.js, /a.txt
        // list.removeFile('/a.txt')     // /some/b.js
        list.addFile('/a.txt')        // /some/b.js, /a.txt
        list.addFile('/some/0.js')    // /some/0.js, /some/b.js, /a.txt

        clock.tick(99)
        expect(modified).to.not.have.been.called

        clock.tick(2)
        expect(modified).to.have.been.calledOnce // This will fail

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.

[danielcompton]

This is ready for review now.

[danielcompton]

@wesleycho would you able to take another review please?

[dignifiedquire]

Thanks a lot for getting through this and especially updating the tests to correctly test what they should test!