Repository: karma-runner/karma
Pull request #3750 (2022-01-23 20:43:14)

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.

jginsburgn: @devoto13 do you consider this PR ready to be merged?
devoto13: @jginsburgn Yes, it is ready. It is a companion PR for the https://github.com/karma-runner/karma-coverage/pull/463, so both should be merged to resolve the coverage generation race.

Patch:
diff --git a/lib/helper.js b/lib/helper.js
index e7741803..cd239ecc 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 d1c4a2e7..64d119db 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 718ee860..f8a1f71f 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",


Test patch:
diff --git a/test/unit/helper.spec.js b/test/unit/helper.spec.js
index 8c5808c5..2d4abc20 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()
       })
     })