From 00d927448d65d860e0f147aeb897579309d6949a Mon Sep 17 00:00:00 2001 From: Daniel Compton Date: Thu, 9 Feb 2017 12:05:16 +1300 Subject: [PATCH] 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 --- docs/config/01-configuration-file.md | 5 +- lib/file-list.js | 10 +- test/unit/file-list.spec.js | 192 ++++++++++++++++++++++++--- 3 files changed, 178 insertions(+), 29 deletions(-) diff --git a/docs/config/01-configuration-file.md b/docs/config/01-configuration-file.md index a97730693..6deb8c1f6 100644 --- a/docs/config/01-configuration-file.md +++ b/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 diff --git a/lib/file-list.js b/lib/file-list.js index ca9c9e2e2..cac538907 100644 --- a/lib/file-list.js +++ b/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` + // This function is debounced to the value of `autoWatchBatchDelay` // 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._autoWatchBatchDelay) self._emitModified = function (immediate) { - immediate ? emit() : throttledEmit() + immediate ? emit() : debouncedEmit() } } diff --git a/test/unit/file-list.spec.js b/test/unit/file-list.spec.js index ccf10d897..e64d09679 100644 --- a/test/unit/file-list.spec.js +++ b/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) @@ -732,22 +873,29 @@ describe('FileList', () => { mockFs._touchFile('/some/b.js', '2020-01-01') 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 - - 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' + ]) + }) + }) }) })