feat(watcher): Debounce autoWatchBatchDelay

- 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 #2331

Author: danielcompton

docs/config/01-configuration-file.md

@@ -89,8 +89,9 @@ These are all of the available configuration options.
 
 **Description:** When Karma is watching the files for changes, it tries to batch
 multiple changes into a single run so that the test runner doesn't try to start and restart running
-tests more than it should. The configuration setting tells Karma how long to wait (in milliseconds) after any changes
-have occurred before starting the test process again.
+tests more than it should, or restart while build files are not in a consistent state. The configuration setting
+tells Karma how long to wait (in milliseconds) from the last file change before starting
+the test process again, resetting the timer each time a file changes (i.e. [debouncing](https://davidwalsh.name/javascript-debounce-function)).
 
 
 ## basePath

lib/file-list.js

@@ -49,13 +49,13 @@ function byPath (a, b) {
 // emitter       - EventEmitter
 // preprocess    - Function
 // batchInterval - Number
-var List = function (patterns, excludes, emitter, preprocess, batchInterval) {
+var List = function (patterns, excludes, emitter, preprocess, autoWatchBatchDelay) {
   // Store options
   this._patterns = patterns
   this._excludes = excludes
   this._emitter = emitter
   this._preprocess = Promise.promisify(preprocess)
-  this._batchInterval = batchInterval
+  this._autoWatchBatchDelay = autoWatchBatchDelay
 
   // The actual list of files
   this.buckets = new Map()
@@ -71,14 +71,14 @@ var List = function (patterns, excludes, emitter, preprocess, batchInterval) {
   var self = this
 
   // Emit the `file_list_modified` event.
-  // This function is throttled to the value of `batchInterval`
-  // to avoid spamming the listener.
+  // This function is debounced to the value of `autoWatchBatchDelay`
+  // to avoid reloading while files are still being modified.
   function emit () {
     self._emitter.emit('file_list_modified', self.files)
   }
-  var throttledEmit = _.throttle(emit, self._batchInterval, {leading: false})
+  var debouncedEmit = _.debounce(emit, self._autoWatchBatchDelay)
   self._emitModified = function (immediate) {
-    immediate ? emit() : throttledEmit()
+    immediate ? emit() : debouncedEmit()
   }
 }
 

test/unit/file-list.spec.js

@@ -422,6 +422,8 @@ describe('FileList', () => {
   })
 
   describe('addFile', () => {
+    var clock = null
+
     beforeEach(() => {
       patternList = PATTERN_LIST
       mg = MG
@@ -435,14 +437,25 @@ describe('FileList', () => {
           statCache: mg.statCache
         })
       }
+
+      clock = sinon.useFakeTimers()
+      // This hack is needed to ensure lodash is using the fake timers
+      // from sinon
       List = proxyquire('../../lib/file-list', {
+        lodash: _.runInContext(),
         helper: helper,
         glob: glob,
+        'graceful-fs': mockFs,
         path: pathLib.posix || pathLib/* for node 0.10 */,
-        'graceful-fs': mockFs
+        bluebird: Promise
       })
 
-      list = new List(patterns('/some/*.js', '*.txt'), ['/secret/*.txt'], emitter, preprocess)
+      list = new List(patterns('/some/*.js', '*.txt'), ['/secret/*.txt'], emitter, preprocess, 100)
+    })
+
+    afterEach(() => {
+      clock.restore()
+      Promise.setScheduler((fn) => process.nextTick(fn))
     })
 
     it('does not add excluded files', () => {
@@ -487,6 +500,7 @@ describe('FileList', () => {
         modified.reset()
 
         return list.addFile('/some/d.js').then(() => {
+          clock.tick(101)
           expect(modified).to.have.been.calledOnce
         })
       })
@@ -534,9 +548,12 @@ describe('FileList', () => {
   })
 
   describe('changeFile', () => {
+    var clock = null
+
     beforeEach(() => {
       patternList = PATTERN_LIST
       mg = MG
+      Promise.setScheduler((fn) => fn())
 
       preprocess = sinon.spy((file, done) => process.nextTick(done))
       emitter = new EventEmitter()
@@ -548,20 +565,30 @@ describe('FileList', () => {
         })
       }
 
+      clock = sinon.useFakeTimers()
+      // This hack is needed to ensure lodash is using the fake timers
+      // from sinon
       List = proxyquire('../../lib/file-list', {
+        lodash: _.runInContext(),
         helper: helper,
         glob: glob,
+        'graceful-fs': mockFs,
         path: pathLib.posix || pathLib/* for node 0.10 */,
-        'graceful-fs': mockFs
+        bluebird: Promise
       })
 
       mockFs._touchFile('/some/a.js', '2012-04-04')
       mockFs._touchFile('/some/b.js', '2012-05-05')
     })
 
+    afterEach(() => {
+      clock.restore()
+      Promise.setScheduler((fn) => process.nextTick(fn))
+    })
+
     it('updates mtime and fires "file_list_modified"', () => {
       // MATCH: /some/a.js, /some/b.js
-      list = new List(patterns('/some/*.js', '/a.*'), [], emitter, preprocess)
+      list = new List(patterns('/some/*.js', '/a.*'), [], emitter, preprocess, 100)
       var modified = sinon.stub()
       emitter.on('file_list_modified', modified)
 
@@ -570,6 +597,7 @@ describe('FileList', () => {
         modified.reset()
 
         return list.changeFile('/some/b.js').then((files) => {
+          clock.tick(101)
           expect(modified).to.have.been.calledOnce
           expect(findFile('/some/b.js', files.served).mtime).to.be.eql(new Date('2020-01-01'))
         })
@@ -627,9 +655,12 @@ describe('FileList', () => {
   })
 
   describe('removeFile', () => {
+    var clock = null
+
     beforeEach(() => {
       patternList = PATTERN_LIST
       mg = MG
+      Promise.setScheduler((fn) => fn())
 
       preprocess = sinon.spy((file, done) => process.nextTick(done))
       emitter = new EventEmitter()
@@ -641,20 +672,30 @@ describe('FileList', () => {
         })
       }
 
+      clock = sinon.useFakeTimers()
+      // This hack is needed to ensure lodash is using the fake timers
+      // from sinon
       List = proxyquire('../../lib/file-list', {
+        lodash: _.runInContext(),
         helper: helper,
         glob: glob,
+        'graceful-fs': mockFs,
         path: pathLib.posix || pathLib/* for node 0.10 */,
-        'graceful-fs': mockFs
+        bluebird: Promise
       })
 
       modified = sinon.stub()
       emitter.on('file_list_modified', modified)
     })
 
+    afterEach(() => {
+      clock.restore()
+      Promise.setScheduler((fn) => process.nextTick(fn))
+    })
+
     it('removes the file from the list and fires "file_list_modified"', () => {
       // MATCH: /some/a.js, /some/b.js, /a.txt
-      list = new List(patterns('/some/*.js', '/a.*'), [], emitter, preprocess)
+      list = new List(patterns('/some/*.js', '/a.*'), [], emitter, preprocess, 100)
 
       var modified = sinon.stub()
       emitter.on('file_list_modified', modified)
@@ -667,6 +708,7 @@ describe('FileList', () => {
           '/some/b.js',
           '/a.txt'
         ])
+        clock.tick(101)
         expect(modified).to.have.been.calledOnce
       })
     })
@@ -685,6 +727,15 @@ describe('FileList', () => {
   })
 
   describe('batch interval', () => {
+    // IMPORTANT: When writing tests for debouncing behaviour, you must wait for the promise
+    // returned by list.changeFile or list.addFile. list.removeFile calls self._emitModified()
+    // in a different manner and doesn't *need* to be waited on. If you use this behaviour
+    // in your tests it can can lead to very confusing results when they are modified or
+    // extended.
+    //
+    // Rule of thumb: Always wait on the promises returned by list.addFile, list.changeFile,
+    // and list.removeFile.
+
     var clock = null
 
     beforeEach(() => {
@@ -723,7 +774,97 @@ describe('FileList', () => {
       Promise.setScheduler((fn) => process.nextTick(fn))
     })
 
-    it('batches multiple changes within an interval', () => {
+    it('debounces calls to emitModified', () => {
+      list = new List(patterns(), [], emitter, preprocess, 100)
+
+      return list.refresh().then(() => {
+        modified.reset()
+        list._emitModified()
+        clock.tick(99)
+        expect(modified).to.not.have.been.called
+        list._emitModified()
+        clock.tick(2)
+        expect(modified).to.not.have.been.called
+        clock.tick(97)
+        expect(modified).to.not.have.been.called
+        clock.tick(2)
+        expect(modified).to.have.been.calledOnce
+        clock.tick(1000)
+        expect(modified).to.have.been.calledOnce
+        list._emitModified()
+        clock.tick(99)
+        expect(modified).to.have.been.calledOnce
+        clock.tick(2)
+        expect(modified).to.have.been.calledTwice
+      })
+    })
+
+    it('debounces a single file change', () => {
+      list = new List(patterns('/some/*.js', '/a.*'), [], emitter, preprocess, 100)
+
+      return list.refresh().then((files) => {
+        modified.reset()
+        // Even with no changes, all these files are served
+        list.addFile('/some/0.js').then(() => {
+          clock.tick(99)
+          expect(modified).to.not.have.been.called
+
+          clock.tick(2)
+          expect(modified).to.have.been.calledOnce
+
+          files = modified.lastCall.args[0]
+          expect(pathsFrom(files.served)).to.be.eql([
+            '/some/0.js',
+            '/some/a.js',
+            '/some/b.js',
+            '/a.txt'
+          ])
+        })
+      })
+    })
+
+    it('debounces several changes to a file', () => {
+      list = new List(patterns('/some/*.js', '/a.*'), [], emitter, preprocess, 100)
+
+      return list.refresh().then((files) => {
+        modified.reset()
+        list.addFile('/some/0.js').then(() => {
+          clock.tick(99)
+          expect(modified).to.not.have.been.called
+
+          // Modify file, must change mtime too, or change is ignored
+          mockFs._touchFile('/some/0.js', '2020-01-01')
+          list.changeFile('/some/0.js').then(() => {
+            // Ensure that the debounce timer was reset
+            clock.tick(2)
+            expect(modified).to.not.have.been.called
+
+            // Ensure that debounce timer fires after 100ms
+            clock.tick(99)
+            expect(modified).to.have.been.calledOnce
+
+            // Make sure there aren't any lingering debounce calls
+            clock.tick(1000)
+
+            // Modify file (one hour later mtime)
+            expect(modified).to.have.been.calledOnce
+            mockFs._touchFile('/some/0.js', '2020-01-02')
+            list.changeFile('/some/0.js').then(() => {
+              clock.tick(99)
+              expect(modified).to.have.been.calledOnce
+              clock.tick(2)
+              expect(modified).to.have.been.calledTwice
+
+              // Make sure there aren't any lingering calls
+              clock.tick(1000)
+              expect(modified).to.have.been.calledTwice
+            })
+          })
+        })
+      })
+    })
+
+    it('debounces multiple changes until there is quiescence', () => {
       // MATCH: /some/a.js, /some/b.js, /a.txt
       list = new List(patterns('/some/*.js', '/a.*'), [], emitter, preprocess, 100)
 
@@ -734,20 +875,28 @@ describe('FileList', () => {
         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
-
-        files = modified.lastCall.args[0]
-        expect(pathsFrom(files.served)).to.be.eql([
-          '/some/0.js',
-          '/some/b.js',
-          '/a.txt'
-        ])
+        list.addFile('/some/0.js').then(() => {     // /some/0.js, /some/b.js, /a.txt
+          clock.tick(99)
+          expect(modified).to.not.have.been.called
+          mockFs._touchFile('/a.txt', '2020-01-01')
+          list.changeFile('/a.txt').then(() => {
+            clock.tick(2)
+            expect(modified).to.not.have.been.called
+
+            clock.tick(100)
+            expect(modified).to.have.been.calledOnce
+
+            clock.tick(1000)
+            expect(modified).to.have.been.calledOnce
+
+            files = modified.lastCall.args[0]
+            expect(pathsFrom(files.served)).to.be.eql([
+              '/some/0.js',
+              '/some/b.js',
+              '/a.txt'
+            ])
+          })
+        })
       })
     })