diff options
author | isaacs <i@izs.me> | 2019-09-30 20:21:10 +0300 |
---|---|---|
committer | isaacs <i@izs.me> | 2019-10-08 02:25:04 +0300 |
commit | 44a2b036b34324ec85943908264b2e36de5a9435 (patch) | |
tree | 15e719501072ca71ee3fda7c35103ce238596f7f | |
parent | dcff367a742257b00a21ad870ef370e4ecbd2f5d (diff) |
fix root-ownership race conditions in meta-test
Currently all of our tests verify on teardown that there are no
root-owned files in the cache.
However, owing to some race conditions and slippery stream event
deferral behavior that won't be fixed until v7, occasionally cacache's
chown doesn't get processed until _after_ the promise resolves and the
test ends.
As a result, sometimes this check occurs before the chown has happened,
resulting in flaky hard-to-reproduce failures.
The somewhat-kludgey solution here is to move the ownership check from
t.teardown to process.on('exit'). In npm v7, we should move it back to
t.teardown, because we should never have a test that resolves in such a
way as to leave the cache in an invalid state.
PR-URL: https://github.com/npm/cli/pull/262
Credit: @isaacs
Close: #262
Reviewed-by: @isaacs
-rw-r--r-- | test/common-tap.js | 44 | ||||
-rw-r--r-- | test/tap/meta-test-flaky-root-ownership-test.js | 20 |
2 files changed, 43 insertions, 21 deletions
diff --git a/test/common-tap.js b/test/common-tap.js index 83a61f4bd..d8dc8a10d 100644 --- a/test/common-tap.js +++ b/test/common-tap.js @@ -60,29 +60,31 @@ const find = require('which').sync('find') require('tap').teardown(() => { // work around windows folder locking process.chdir(returnCwd) - try { - if (isSudo) { - // running tests as sudo. ensure we didn't leave any root-owned - // files in the cache by mistake. - const args = [ commonCache, '-uid', '0' ] - const found = spawnSync(find, args) - const output = found && found.stdout && found.stdout.toString() - if (output.length) { - const er = new Error('Root-owned files left in cache!') - er.testName = main - er.files = output.trim().split('\n') - throw er + process.on('exit', () => { + try { + if (isSudo) { + // running tests as sudo. ensure we didn't leave any root-owned + // files in the cache by mistake. + const args = [ commonCache, '-uid', '0' ] + const found = spawnSync(find, args) + const output = found && found.stdout && found.stdout.toString() + if (output.length) { + const er = new Error('Root-owned files left in cache!') + er.testName = main + er.files = output.trim().split('\n') + throw er + } + } + if (!process.env.NO_TEST_CLEANUP) { + rimraf.sync(exports.pkg) + rimraf.sync(commonCache) + } + } catch (e) { + if (process.platform !== 'win32') { + throw e } } - if (!process.env.NO_TEST_CLEANUP) { - rimraf.sync(exports.pkg) - rimraf.sync(commonCache) - } - } catch (e) { - if (process.platform !== 'win32') { - throw e - } - } + }) }) var port = exports.port = 15443 + testId diff --git a/test/tap/meta-test-flaky-root-ownership-test.js b/test/tap/meta-test-flaky-root-ownership-test.js new file mode 100644 index 000000000..24dd9e3d9 --- /dev/null +++ b/test/tap/meta-test-flaky-root-ownership-test.js @@ -0,0 +1,20 @@ +const t = require('tap') +if (!process.getuid || process.getuid() !== 0 || !process.env.SUDO_UID || !process.env.SUDO_GID) { + t.pass('this test only runs in sudo mode') + t.end() + process.exit(0) +} + +const common = require('../common-tap.js') +const fs = require('fs') +const mkdirp = require('mkdirp') +mkdirp.sync(common.cache + '/root/owned') +fs.writeFileSync(common.cache + '/root/owned/file.txt', 'should be chowned') +const chown = require('chownr') + +// this will fire after t.teardown() but before process.on('exit') +setTimeout(() => { + chown.sync(common.cache, +process.env.SUDO_UID, +process.env.SUDO_GID) +}, 100) + +t.pass('this is fine') |