From d9dade2f004a340e49c9a633177576200c286404 Mon Sep 17 00:00:00 2001 From: Yaroslav Admin Date: Fri, 21 Jan 2022 17:19:02 +0100 Subject: [PATCH] fix(helper): make mkdirIfNotExists helper resilient to concurrent calls The main motivation for this change is https://github.com/karma-runner/karma-coverage/issues/434#issuecomment-1017939333 where concurrent calls to the helper fail because of the race between the check and directory creation. This is a temporary solution and should be replaced with the native [`mkdir`](https://nodejs.org/api/fs.html#fsmkdirpath-options-callback) with the `recursive` option once minimum supported Node is bumped to 12. --- lib/helper.js | 16 ++-------------- package-lock.json | 5 ++--- package.json | 2 +- test/unit/helper.spec.js | 33 ++++++++++----------------------- 4 files changed, 15 insertions(+), 41 deletions(-) diff --git a/lib/helper.js b/lib/helper.js index e77418039..cd239ecc3 100644 --- a/lib/helper.js +++ b/lib/helper.js @@ -1,8 +1,7 @@ 'use strict' -const fs = require('graceful-fs') -const path = require('path') const _ = require('lodash') +const mkdirp = require('mkdirp') const useragent = require('ua-parser-js') const mm = require('minimatch') @@ -141,18 +140,7 @@ const replaceWinPath = (path) => { exports.normalizeWinPath = process.platform === 'win32' ? replaceWinPath : _.identity exports.mkdirIfNotExists = (directory, done) => { - // TODO(vojta): handle if it's a file - /* eslint-disable handle-callback-err */ - fs.stat(directory, (err, stat) => { - if (stat && stat.isDirectory()) { - done() - } else { - exports.mkdirIfNotExists(path.dirname(directory), () => { - fs.mkdir(directory, done) - }) - } - }) - /* eslint-enable handle-callback-err */ + mkdirp(directory, done) } exports.defer = () => { diff --git a/package-lock.json b/package-lock.json index d1c4a2e7d..64d119dbb 100644 --- a/package-lock.json +++ b/package-lock.json @@ -5224,6 +5224,7 @@ "log4js": "^6.4.1", "mime": "^2.5.2", "minimatch": "^3.0.4", + "mkdirp": "^0.5.5", "qjobs": "^1.2.0", "range-parser": "^1.2.1", "rimraf": "^3.0.2", @@ -6020,8 +6021,7 @@ "minimist": { "version": "1.2.5", "resolved": "https://registry.npmjs.org/minimist/-/minimist-1.2.5.tgz", - "integrity": "sha512-FM9nNUYrRBAELZQT3xeZQ7fmMOBg6nWNmJKTcgsJeaLstP/UODVpGsr5OhXhhXg6f+qtJ8uiZ+PUxkDWcgIXLw==", - "dev": true + "integrity": "sha512-FM9nNUYrRBAELZQT3xeZQ7fmMOBg6nWNmJKTcgsJeaLstP/UODVpGsr5OhXhhXg6f+qtJ8uiZ+PUxkDWcgIXLw==" }, "minimist-options": { "version": "4.1.0", @@ -6059,7 +6059,6 @@ "version": "0.5.5", "resolved": "https://registry.npmjs.org/mkdirp/-/mkdirp-0.5.5.tgz", "integrity": "sha512-NKmAlESf6jMGym1++R0Ra7wvhV+wFW63FaSOFPwRahvea0gMUcGUhVeAg/0BC0wiv9ih5NYPB1Wn1UEI1/L+xQ==", - "dev": true, "requires": { "minimist": "^1.2.5" } diff --git a/package.json b/package.json index 718ee8603..f8a1f71fe 100644 --- a/package.json +++ b/package.json @@ -441,6 +441,7 @@ "log4js": "^6.4.1", "mime": "^2.5.2", "minimatch": "^3.0.4", + "mkdirp": "^0.5.5", "qjobs": "^1.2.0", "range-parser": "^1.2.1", "rimraf": "^3.0.2", @@ -479,7 +480,6 @@ "karma-mocha": "^1.0.1", "karma-mocha-reporter": "^2.0.0", "karma-script-launcher": "^1.0.0", - "mkdirp": "^0.5.0", "mocha": "^4.1.0", "mocks": "^0.0.15", "proxyquire": "^2.1.3", diff --git a/test/unit/helper.spec.js b/test/unit/helper.spec.js index 8c5808c50..2d4abc204 100644 --- a/test/unit/helper.spec.js +++ b/test/unit/helper.spec.js @@ -229,36 +229,23 @@ describe('helper', () => { }) describe('mkdirIfNotExists', () => { - const fsMock = require('mocks').fs const loadFile = require('mocks').loadFile - const fs = fsMock.create({ - home: { 'some.js': fsMock.file() } - }) + const spy = sinon.spy() // load file under test - const m = loadFile(path.join(__dirname, '/../../lib/helper.js'), { 'graceful-fs': fs, lodash: require('lodash') }) - - const mkdirIfNotExists = m.exports.mkdirIfNotExists - - it('should not do anything, if dir already exists', (done) => { - mkdirIfNotExists('/home', done) - }) - - it('should create directory if it does not exist', (done) => { - mkdirIfNotExists('/home/new', () => { - const stat = fs.statSync('/home/new') - expect(stat).to.exist - expect(stat.isDirectory()).to.equal(true) + const m = loadFile(path.join(__dirname, '/../../lib/helper.js'), { + mkdirp: (path, done) => { + spy(path) done() - }) + } }) - it('should create even parent directories if it does not exist', (done) => { - mkdirIfNotExists('/home/new/parent/child', () => { - const stat = fs.statSync('/home/new/parent/child') - expect(stat).to.exist - expect(stat.isDirectory()).to.equal(true) + const mkdirIfNotExists = m.exports.mkdirIfNotExists + + it('should call mkdirp', (done) => { + mkdirIfNotExists('/path/to/dir', () => { + expect(spy).to.have.been.calledOnceWith('/path/to/dir') done() }) })