From b2d9ba3a0c301630748c6faecf1b492419409de9 Mon Sep 17 00:00:00 2001 From: Yaroslav Admin Date: Fri, 28 Feb 2020 17:51:27 +0100 Subject: [PATCH] refactor: use native Promise instead of Bluebird All supported Node versions support native promises and async/await, current code has a minimal usage of Bluebird-specific methods, so dropping a third-party library in favor of native Promises. Tests were using `Promise.setScheduler()` from Bluebird, which allowed to customize how Promises are scheduled and made test work very different from the real code, which created tricky issues (like https://github.com/karma-runner/karma/pull/3060#discussion_r284797390). Affected tests were updated to not rely on `Promise.setScheduler()`, which improved readability in some situations. Also removed some test helpers as they are not necessary and not used anymore. BREAKING CHANGE: Karma plugins which rely on the fact that Karma uses Bluebird promises may break as Bluebird-specific API is no longer available on Promises returned by the Karma core --- lib/file-list.js | 14 ++-- lib/helper.js | 1 - lib/launcher.js | 1 - lib/launchers/base.js | 1 - lib/server.js | 1 - lib/utils/bundle-utils.js | 1 - lib/utils/net-utils.js | 1 - package-lock.json | 14 ++-- package.json | 1 - test/.eslintrc | 3 +- test/unit/file-list.spec.js | 24 ++----- test/unit/launcher.spec.js | 22 ++---- test/unit/launchers/base.spec.js | 74 ++++++++------------- test/unit/launchers/capture_timeout.spec.js | 14 ++-- test/unit/launchers/process.spec.js | 9 ++- test/unit/middleware/runner.spec.js | 9 --- test/unit/mocha-globals.js | 37 ----------- test/unit/utils/net-utils.spec.js | 1 + 18 files changed, 69 insertions(+), 159 deletions(-) diff --git a/lib/file-list.js b/lib/file-list.js index 19d0cb180..2120ce6d3 100644 --- a/lib/file-list.js +++ b/lib/file-list.js @@ -1,9 +1,10 @@ 'use strict' -const Promise = require('bluebird') +const { promisify } = require('util') const mm = require('minimatch') const Glob = require('glob').Glob -const fs = Promise.promisifyAll(require('graceful-fs')) +const fs = require('graceful-fs') +fs.statAsync = promisify(fs.stat) const pathLib = require('path') const _ = require('lodash') @@ -60,8 +61,8 @@ class FileList { const matchedFiles = new Set() let lastCompletedRefresh = this._refreshing - lastCompletedRefresh = Promise - .map(this._patterns, async ({ pattern, type, nocache, isBinary }) => { + lastCompletedRefresh = Promise.all( + this._patterns.map(async ({ pattern, type, nocache, isBinary }) => { if (helper.isUrlAbsolute(pattern)) { this.buckets.set(pattern, [new Url(pattern, type)]) return @@ -86,7 +87,7 @@ class FileList { if (nocache) { log.debug(`Not preprocessing "${pattern}" due to nocache`) } else { - await Promise.map(files, (file) => this._preprocess(file)) + await Promise.all(files.map((file) => this._preprocess(file))) } this.buckets.set(pattern, files) @@ -97,6 +98,7 @@ class FileList { log.warn(`All files matched by "${pattern}" were excluded or matched by prior matchers.`) } }) + ) .then(() => { // When we return from this function the file processing chain will be // complete. In the case of two fast refresh() calls, the second call @@ -202,7 +204,7 @@ class FileList { if (!file) { log.debug(`Changed file "${path}" ignored. Does not match any file in the list.`) - return Promise.resolve(this.files) + return this.files } const [stat] = await Promise.all([fs.statAsync(path), this._refreshing]) diff --git a/lib/helper.js b/lib/helper.js index b3d64b24f..0d03eada5 100644 --- a/lib/helper.js +++ b/lib/helper.js @@ -4,7 +4,6 @@ const fs = require('graceful-fs') const path = require('path') const _ = require('lodash') const useragent = require('useragent') -const Promise = require('bluebird') const mm = require('minimatch') exports.browserFullNameToShort = (fullName) => { diff --git a/lib/launcher.js b/lib/launcher.js index fd63c8d3d..cc383c3a1 100644 --- a/lib/launcher.js +++ b/lib/launcher.js @@ -1,6 +1,5 @@ 'use strict' -const Promise = require('bluebird') const Jobs = require('qjobs') const log = require('./logger').create('launcher') diff --git a/lib/launchers/base.js b/lib/launchers/base.js index b8349ce3a..e4a562aed 100644 --- a/lib/launchers/base.js +++ b/lib/launchers/base.js @@ -1,6 +1,5 @@ const KarmaEventEmitter = require('../events').EventEmitter const EventEmitter = require('events').EventEmitter -const Promise = require('bluebird') const log = require('../logger').create('launcher') const helper = require('../helper') diff --git a/lib/server.js b/lib/server.js index 5fd6c08cd..dc20c3f54 100644 --- a/lib/server.js +++ b/lib/server.js @@ -3,7 +3,6 @@ const SocketIO = require('socket.io') const di = require('di') const util = require('util') -const Promise = require('bluebird') const spawn = require('child_process').spawn const tmp = require('tmp') const fs = require('fs') diff --git a/lib/utils/bundle-utils.js b/lib/utils/bundle-utils.js index 143815985..760c61d31 100644 --- a/lib/utils/bundle-utils.js +++ b/lib/utils/bundle-utils.js @@ -1,7 +1,6 @@ 'use strict' const PathUtils = require('./path-utils') const fs = require('fs') -const Promise = require('bluebird') const BundleUtils = { bundleResource (inPath, outPath) { diff --git a/lib/utils/net-utils.js b/lib/utils/net-utils.js index f6c371f7e..097455485 100644 --- a/lib/utils/net-utils.js +++ b/lib/utils/net-utils.js @@ -1,6 +1,5 @@ 'use strict' -const Promise = require('bluebird') const net = require('net') const NetUtils = { diff --git a/package-lock.json b/package-lock.json index 1d3089f57..094c00365 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1281,7 +1281,8 @@ "bluebird": { "version": "3.5.5", "resolved": "https://registry.npmjs.org/bluebird/-/bluebird-3.5.5.tgz", - "integrity": "sha512-5am6HnnfN+urzt4yfg7IgTbotDjIT/u8AJpEt0sIU9FtXfVeezXAPKswrG+xKUCOYAINpSdgZVDU6QFh+cuH3w==" + "integrity": "sha512-5am6HnnfN+urzt4yfg7IgTbotDjIT/u8AJpEt0sIU9FtXfVeezXAPKswrG+xKUCOYAINpSdgZVDU6QFh+cuH3w==", + "dev": true }, "bn.js": { "version": "4.11.8", @@ -2014,7 +2015,8 @@ "version": "1.1.0", "resolved": "https://registry.npmjs.org/console-control-strings/-/console-control-strings-1.1.0.tgz", "integrity": "sha1-PXz0Rk22RG6mRL9LOVB/mFEAjo4=", - "dev": true + "dev": true, + "optional": true }, "constants-browserify": { "version": "1.0.0", @@ -4236,7 +4238,8 @@ "version": "2.1.1", "resolved": "https://registry.npmjs.org/ansi-regex/-/ansi-regex-2.1.1.tgz", "integrity": "sha1-w7M6te42DYbg5ijwRorn7yfWVN8=", - "dev": true + "dev": true, + "optional": true }, "is-fullwidth-code-point": { "version": "1.0.0", @@ -4265,6 +4268,7 @@ "resolved": "https://registry.npmjs.org/strip-ansi/-/strip-ansi-3.0.1.tgz", "integrity": "sha1-ajhfuIU9lS1f8F0Oiq+UJ43GPc8=", "dev": true, + "optional": true, "requires": { "ansi-regex": "^2.0.0" } @@ -6660,6 +6664,7 @@ "resolved": "https://registry.npmjs.org/minipass/-/minipass-2.3.5.tgz", "integrity": "sha512-Gi1W4k059gyRbyVUZQ4mEqLm0YIUiGYfvxhF6SIlk3ui1WVxMTGfGdQ2SInh3PDrRTVvPKgULkpJtT4RH10+VA==", "dev": true, + "optional": true, "requires": { "safe-buffer": "^5.1.2", "yallist": "^3.0.0" @@ -9721,7 +9726,8 @@ "version": "3.0.3", "resolved": "https://registry.npmjs.org/yallist/-/yallist-3.0.3.tgz", "integrity": "sha512-S+Zk8DEWE6oKpV+vI3qWkaK+jSbIK86pCwe2IF/xwIpQ8jEuxpw9NyaGjmp9+BoJv5FV2piqCDcoCtStppiq2A==", - "dev": true + "dev": true, + "optional": true }, "yaml": { "version": "1.7.2", diff --git a/package.json b/package.json index f0fa1d647..cbb1f8080 100644 --- a/package.json +++ b/package.json @@ -390,7 +390,6 @@ "Karl Lindmark " ], "dependencies": { - "bluebird": "^3.3.0", "body-parser": "^1.16.1", "braces": "^3.0.2", "chokidar": "^3.0.0", diff --git a/test/.eslintrc b/test/.eslintrc index 3ca008abd..090e94a75 100644 --- a/test/.eslintrc +++ b/test/.eslintrc @@ -4,8 +4,7 @@ }, "globals": { "expect": true, - "sinon": true, - "scheduleNextTick": true + "sinon": true }, "rules": { "no-unused-expressions": "off" diff --git a/test/unit/file-list.spec.js b/test/unit/file-list.spec.js index ba2617b2b..002c56549 100644 --- a/test/unit/file-list.spec.js +++ b/test/unit/file-list.spec.js @@ -1,6 +1,5 @@ 'use strict' -const Promise = require('bluebird') const EventEmitter = require('events').EventEmitter const mocks = require('mocks') const proxyquire = require('proxyquire') @@ -448,8 +447,7 @@ describe('FileList', () => { helper: helper, glob: glob, 'graceful-fs': mockFs, - path: pathLib.posix, - bluebird: Promise + path: pathLib.posix }) list = new List(patterns('/some/*.js', '*.txt'), ['/secret/*.txt'], emitter, preprocess, 100) @@ -457,7 +455,6 @@ describe('FileList', () => { afterEach(() => { clock.restore() - Promise.setScheduler((fn) => process.nextTick(fn)) }) it('does not add excluded files', () => { @@ -514,14 +511,14 @@ describe('FileList', () => { return list.refresh().then(() => { preprocess.resetHistory() - sinon.spy(mockFs, 'stat') + sinon.spy(mockFs, 'statAsync') return Promise.all([ list.addFile('/some/d.js'), list.addFile('/some/d.js') ]).then(() => { expect(preprocess).to.have.been.calledOnce - expect(mockFs.stat).to.have.been.calledOnce + expect(mockFs.statAsync).to.have.been.calledOnce }) }) }) @@ -555,7 +552,6 @@ describe('FileList', () => { beforeEach(() => { patternList = PATTERN_LIST mg = MG - Promise.setScheduler((fn) => fn()) emitter = new EventEmitter() @@ -576,8 +572,7 @@ describe('FileList', () => { helper: helper, glob: glob, 'graceful-fs': mockFs, - path: pathLib.posix, - bluebird: Promise + path: pathLib.posix }) mockFs._touchFile('/some/a.js', '2012-04-04') @@ -586,7 +581,6 @@ describe('FileList', () => { afterEach(() => { clock.restore() - Promise.setScheduler((fn) => process.nextTick(fn)) }) it('updates mtime and fires "file_list_modified"', () => { @@ -680,7 +674,6 @@ describe('FileList', () => { beforeEach(() => { patternList = PATTERN_LIST mg = MG - Promise.setScheduler((fn) => fn()) emitter = new EventEmitter() @@ -701,8 +694,7 @@ describe('FileList', () => { helper: helper, glob: glob, 'graceful-fs': mockFs, - path: pathLib.posix, - bluebird: Promise + path: pathLib.posix }) modified = sinon.stub() @@ -711,7 +703,6 @@ describe('FileList', () => { afterEach(() => { clock.restore() - Promise.setScheduler((fn) => process.nextTick(fn)) }) it('removes the file from the list and fires "file_list_modified"', () => { @@ -762,7 +753,6 @@ describe('FileList', () => { beforeEach(() => { patternList = PATTERN_LIST mg = MG - Promise.setScheduler((fn) => { fn() }) emitter = new EventEmitter() @@ -786,14 +776,12 @@ describe('FileList', () => { helper: helper, glob: glob, 'graceful-fs': mockFs, - path: pathLib.posix, - bluebird: Promise + path: pathLib.posix }) }) afterEach(() => { clock.restore() - Promise.setScheduler((fn) => process.nextTick(fn)) }) it('debounces calls to emitModified', () => { diff --git a/test/unit/launcher.spec.js b/test/unit/launcher.spec.js index 19455d412..827097406 100644 --- a/test/unit/launcher.spec.js +++ b/test/unit/launcher.spec.js @@ -1,6 +1,5 @@ 'use strict' -const Promise = require('bluebird') const di = require('di') const events = require('../../lib/events') @@ -65,14 +64,6 @@ describe('launcher', () => { let lastGeneratedId = null launcher.Launcher.generateId = () => ++lastGeneratedId - before(() => { - Promise.setScheduler((fn) => fn()) - }) - - after(() => { - Promise.setScheduler((fn) => process.nextTick(fn)) - }) - beforeEach(() => { lastGeneratedId = 0 FakeBrowser._instances = [] @@ -252,7 +243,7 @@ describe('launcher', () => { expect(browser.forceKill).to.have.been.called }) - it('should call callback when all processes killed', () => { + it('should call callback when all processes killed', (done) => { const exitSpy = sinon.spy() l.launch(['Fake', 'Fake'], 1) @@ -264,18 +255,17 @@ describe('launcher', () => { let browser = FakeBrowser._instances.pop() browser.forceKill.resolve() - scheduleNextTick(() => { + setImmediate(() => { expect(exitSpy).not.to.have.been.called - }) - scheduleNextTick(() => { // finish the second browser browser = FakeBrowser._instances.pop() browser.forceKill.resolve() - }) - scheduleNextTick(() => { - expect(exitSpy).to.have.been.called + setImmediate(() => { + expect(exitSpy).to.have.been.called + done() + }) }) }) diff --git a/test/unit/launchers/base.spec.js b/test/unit/launchers/base.spec.js index f6a85794d..f711bbc4d 100644 --- a/test/unit/launchers/base.spec.js +++ b/test/unit/launchers/base.spec.js @@ -81,7 +81,7 @@ describe('launchers/base.js', () => { }) }) - it('should not restart when being force killed', (done) => { + it('should not restart when being force killed', async () => { const spyOnStart = sinon.spy() const spyOnKill = sinon.spy() launcher.on('start', spyOnStart) @@ -98,22 +98,18 @@ describe('launchers/base.js', () => { launcher._done() spyOnKill.callArg(0) - onceKilled.done(() => { - expect(spyOnStart).to.not.have.been.called - done() - }) + await onceKilled + expect(spyOnStart).to.not.have.been.called }) }) describe('kill', () => { - it('should manage state', (done) => { + it('should manage state', async () => { const onceKilled = launcher.kill() expect(launcher.state).to.equal(launcher.STATE_BEING_KILLED) - onceKilled.done(() => { - expect(launcher.state).to.equal(launcher.STATE_FINISHED) - done() - }) + await onceKilled + expect(launcher.state).to.equal(launcher.STATE_FINISHED) }) it('should fire "kill" and wait for all listeners to finish', (done) => { @@ -152,51 +148,37 @@ describe('launchers/base.js', () => { spyOnKill.callArg(0) }) - it('should not fire "kill" if already being killed, but wait for all listeners', (done) => { + it('should not fire "kill" second time if already being killed', async () => { const spyOnKill = sinon.spy() launcher.on('kill', spyOnKill) - const expectOnKillListenerIsAlreadyFinishedAndHasBeenOnlyCalledOnce = () => { - expect(spyOnKill).to.have.been.called - expect(spyOnKill.callCount).to.equal(1) - expect(spyOnKill.finished).to.equal(true) - expect(launcher.state).to.equal(launcher.STATE_FINISHED) - } - launcher.start('http://localhost:9876/') - const firstKilling = launcher.kill().then(() => { - expectOnKillListenerIsAlreadyFinishedAndHasBeenOnlyCalledOnce() - }) - - const secondKilling = launcher.kill().then(() => { - expectOnKillListenerIsAlreadyFinishedAndHasBeenOnlyCalledOnce() - }) + launcher.kill() + const killing = launcher.kill() expect(launcher.state).to.equal(launcher.STATE_BEING_KILLED) - _.defer(() => { - spyOnKill.finished = true - spyOnKill.callArg(0) - }) + spyOnKill.callArg(0) - // finish the test once everything is done - firstKilling.done(() => secondKilling.done(() => done())) + await killing + + expect(spyOnKill).to.have.been.called + expect(spyOnKill.callCount).to.equal(1) + expect(launcher.state).to.equal(launcher.STATE_FINISHED) }) - it('should not kill already crashed browser', (done) => { + it('should not kill already crashed browser', async () => { const spyOnKill = sinon.spy((killDone) => killDone()) launcher.on('kill', spyOnKill) launcher._done('crash') - launcher.kill().done(() => { - expect(spyOnKill).to.not.have.been.called - done() - }) + await launcher.kill() + expect(spyOnKill).to.not.have.been.called }) }) describe('forceKill', () => { - it('should cancel restart', (done) => { + it('should cancel restart', async () => { const spyOnStart = sinon.spy() launcher.on('start', spyOnStart) @@ -204,14 +186,13 @@ describe('launchers/base.js', () => { spyOnStart.resetHistory() launcher.restart() - launcher.forceKill().done(() => { - expect(launcher.state).to.equal(launcher.STATE_FINISHED) - expect(spyOnStart).to.not.have.been.called - done() - }) + await launcher.forceKill() + + expect(launcher.state).to.equal(launcher.STATE_FINISHED) + expect(spyOnStart).to.not.have.been.called }) - it('should not fire "browser_process_failure" even if browser crashes', (done) => { + it('should not fire "browser_process_failure" even if browser crashes', async () => { const spyOnBrowserProcessFailure = sinon.spy() emitter.on('browser_process_failure', spyOnBrowserProcessFailure) @@ -223,10 +204,9 @@ describe('launchers/base.js', () => { }) launcher.start('http://localhost:9876/') - launcher.forceKill().done(() => { - expect(spyOnBrowserProcessFailure).to.not.have.been.called - done() - }) + await launcher.forceKill() + + expect(spyOnBrowserProcessFailure).to.not.have.been.called }) }) diff --git a/test/unit/launchers/capture_timeout.spec.js b/test/unit/launchers/capture_timeout.spec.js index f5f0c6cc7..730ff474d 100644 --- a/test/unit/launchers/capture_timeout.spec.js +++ b/test/unit/launchers/capture_timeout.spec.js @@ -38,7 +38,7 @@ describe('launchers/capture_timeout.js', () => { expect(launcher.kill).not.to.have.been.called }) - it('should clear timeout between restarts', (done) => { + it('should clear timeout between restarts', async () => { CaptureTimeoutLauncher.call(launcher, timer, 10) // simulate process finished @@ -49,12 +49,10 @@ describe('launchers/capture_timeout.js', () => { launcher.start() timer.wind(8) - launcher.kill().done(() => { - launcher.kill.resetHistory() - launcher.start() - timer.wind(8) - expect(launcher.kill).not.to.have.been.called - done() - }) + await launcher.kill() + launcher.kill.resetHistory() + launcher.start() + timer.wind(8) + expect(launcher.kill).not.to.have.been.called }) }) diff --git a/test/unit/launchers/process.spec.js b/test/unit/launchers/process.spec.js index eeb3a3ee0..6dfc29b40 100644 --- a/test/unit/launchers/process.spec.js +++ b/test/unit/launchers/process.spec.js @@ -133,7 +133,7 @@ describe('launchers/process.js', () => { }) // the most common scenario, when everything works fine - it('start -> capture -> kill', (done) => { + it('start -> capture -> kill', async () => { // start the browser launcher.start('http://localhost/') expect(mockSpawn).to.have.been.calledWith(BROWSER_PATH, ['http://localhost/?id=fake-id']) @@ -150,10 +150,9 @@ describe('launchers/process.js', () => { mockSpawn._processes[0].emit('exit', 0) mockTempDir.remove.callArg(1) - killingLauncher.done(() => { - expect(launcher.state).to.equal(launcher.STATE_FINISHED) - done() - }) + await killingLauncher + + expect(launcher.state).to.equal(launcher.STATE_FINISHED) }) // when the browser fails to get captured in default timeout, it should restart diff --git a/test/unit/middleware/runner.spec.js b/test/unit/middleware/runner.spec.js index ba601c07f..190a22229 100644 --- a/test/unit/middleware/runner.spec.js +++ b/test/unit/middleware/runner.spec.js @@ -1,7 +1,6 @@ const path = require('path') const EventEmitter = require('events').EventEmitter const mocks = require('mocks') -const Promise = require('bluebird') const _ = require('lodash') const Browser = require('../../../lib/browser') @@ -38,14 +37,6 @@ describe('middleware.runner', () => { ) } - before(() => { - Promise.setScheduler((fn) => fn()) - }) - - after(() => { - Promise.setScheduler((fn) => process.nextTick(fn)) - }) - beforeEach(() => { mockReporter = { adapters: [], diff --git a/test/unit/mocha-globals.js b/test/unit/mocha-globals.js index d2905e56e..8afdd587b 100644 --- a/test/unit/mocha-globals.js +++ b/test/unit/mocha-globals.js @@ -2,8 +2,6 @@ const sinon = require('sinon') const chai = require('chai') const logger = require('../../lib/logger') -require('bluebird').longStackTraces() - // publish globals that all specs can use global.expect = chai.expect global.should = chai.should() @@ -46,38 +44,3 @@ chai.use((chai, utils) => { `expected response body to not be set, it was '${response._body}'`) }) }) - -// TODO(vojta): move it somewhere ;-) -const nextTickQueue = [] -const nextTickCallback = () => { - if (!nextTickQueue.length) throw new Error('Nothing scheduled!') - nextTickQueue.shift()() - - if (nextTickQueue.length) process.nextTick(nextTickCallback) -} -global.scheduleNextTick = (action) => { - nextTickQueue.push(action) - - if (nextTickQueue.length === 1) process.nextTick(nextTickCallback) -} -const nextQueue = [] -const nextCallback = () => { - // if not nextQueue.length then throw new Error 'Nothing scheduled!' - nextQueue.shift()() -} - -global.scheduleNextTick = (action) => { - nextTickQueue.push(action) - - if (nextTickQueue.length === 1) process.nextTick(nextTickCallback) -} -global.scheduleNext = (action) => { - nextQueue.push(action) -} - -global.next = nextCallback - -beforeEach(() => { - nextTickQueue.length = 0 - nextQueue.length = 0 -}) diff --git a/test/unit/utils/net-utils.spec.js b/test/unit/utils/net-utils.spec.js index e7051b970..859b688f1 100644 --- a/test/unit/utils/net-utils.spec.js +++ b/test/unit/utils/net-utils.spec.js @@ -22,6 +22,7 @@ describe('NetUtils.bindAvailablePort', () => { const port = boundServer.address().port expect(port).to.be.equal(9877) expect(boundServer).not.to.be.null + boundServer.close() server.close(done) }) })