On 12/11/2016 06:29 PM, Sruthi Chandran wrote: > On 12/11/2016 06:18 PM, Guus Sliepen wrote: >> Isn't s/user-home/os-homedir/ not enough? In any case, maybe you should >> try to get upstream to switch to os-homedir instead. > os-homedir is not packaged, we have been patching that with os.homedir. > In this case, the patching is partially working, 4 tests are passing and > 2 are failing. > > I have pushed v8flags to alioth [1]. Please have a look. Test is "mocha > -R spec test.js".
Side-note 1: there appears to be a dependency problem here when running tests: for mocha to work (at least in this case), you also need the package chai to be installed (otherwise it will fail with an error). I think this is because test.js requires chai, but I'm not sure. (I suspect the dependencies of mocha and node-v8flags themselves are OK because it's not required for the package directly. OTOH you could add the test suite to the package's autopkgtests and then add chai and mocha to the Depends there.) Problem 1: - cleanup hook is missing some patching out: delete require.cache[require.resolve('user-home')]; If that line is not removed, then the cleanup hook will fail with require.resolve('user-home'), because that module is not installed. Problem 2: - after patching that out I get the following test results: v8flags ✓ should cache and call back with the v8 flags for the running process ✓ should not append the file when multiple calls happen concurrently and the config file does not yet exist 1) should fall back to writing to a temp dir if user home can't be found ✓ should fall back to writing to a temp dir if user home is unwriteable 2) should return flags even if an error is thrown ✓ should back with an empty array if the runtime is electron The following errors occur: 1) v8flags should fall back to writing to a temp dir if user home can't be found: Uncaught AssertionError: expected false to be true at /srv/external/Debian/Debugging/node-user-home-replacement/node-v8flags/test.js:77:46 at /srv/external/Debian/Debugging/node-user-home-replacement/node-v8flags/index.js:108:14 at /srv/external/Debian/Debugging/node-user-home-replacement/node-v8flags/index.js:36:12 at /srv/external/Debian/Debugging/node-user-home-replacement/node-v8flags/index.js:47:7 at nextTickCallbackWith0Args (node.js:420:9) at process._tickCallback (node.js:349:13) 2) v8flags should return flags even if an error is thrown: Uncaught AssertionError: expected null not to be null at /srv/external/Debian/Debugging/node-user-home-replacement/node-v8flags/test.js:98:28 at /srv/external/Debian/Debugging/node-user-home-replacement/node-v8flags/index.js:108:14 at /srv/external/Debian/Debugging/node-user-home-replacement/node-v8flags/index.js:36:12 at /srv/external/Debian/Debugging/node-user-home-replacement/node-v8flags/index.js:47:7 at nextTickCallbackWith0Args (node.js:420:9) at process._tickCallback (node.js:349:13) In test.js there is a routine eraseHome() which unsets a lot of environment variables. This is to trick the user-home module (which we rejected from Debian, if you remember) to think it can't fetch the user's home directory, and therefore trigger the fallback. This is problematic because os.homedir() falls back to reading /etc/passwd (as it should, that's a feature, not a bug), so it won't fail. That means that the fallback to a temporary directory won't happen. This means that in the first case the fallback won't be triggered, and the expectation of the test is wrong. The second failure is related: since it tries to trigger the "home dir doesn't exist" error to see if the module will succeed anyway without throwing an exception, it does expect the error in the callback passed to the module to be non-null - since it expects an error to have occurred. However, in this case there was no error. This is not a failure in the module itself, this is only a problem with the test suite. There's an easy fix though: monkey-patch os.homedir to a function that just returns null in eraseHome(), and restore it in cleanup(). I've attached an updated use-os-homedir.patch that does this (including the removal of the require.resolve() line above), and the test suite now passes for all 6 tests: v8flags ✓ should cache and call back with the v8 flags for the running process ✓ should not append the file when multiple calls happen concurrently and the config file does not yet exist ✓ should fall back to writing to a temp dir if user home can't be found ✓ should fall back to writing to a temp dir if user home is unwriteable ✓ should return flags even if an error is thrown ✓ should back with an empty array if the runtime is electron 6 passing (61ms) Hope that helps. Regards, Christian
use os.homedir instead of user-home --- a/index.js +++ b/index.js @@ -26,7 +26,7 @@ function fail (err) { } function openConfig (cb) { - var userHome = require('user-home'); + var userHome = os.homedir(); if (!userHome) { return tryOpenConfig(path.join(os.tmpdir(), configfile), cb); } --- a/test.js +++ b/test.js @@ -7,6 +7,8 @@ const expect = require('chai').expect; const env = process.env; +const old_os_homedir = os.homedir; + function eraseHome() { delete env.HOME; delete env.USERPROFILE; @@ -16,6 +18,8 @@ function eraseHome() { delete env.USER; delete env.LNAME; delete env.USERNAME; + + os.homedir = function() { return null; } } function setTemp(dir) { @@ -24,7 +28,7 @@ function setTemp(dir) { function cleanup () { var v8flags = require('./'); - var userHome = require('user-home'); + var userHome = os.homedir(); if (userHome === null) userHome = __dirname; var files = [ @@ -37,7 +41,7 @@ function cleanup () { } catch (e) {} }); - delete require.cache[require.resolve('user-home')]; + os.homedir = old_os_homedir; delete process.versions.electron; } @@ -47,7 +51,7 @@ describe('v8flags', function () { it('should cache and call back with the v8 flags for the running process', function (done) { var v8flags = require('./'); - var configfile = path.resolve(require('user-home'), v8flags.configfile); + var configfile = path.resolve(os.homedir(), v8flags.configfile); v8flags(function (err, flags) { expect(flags).to.be.a('array'); expect(fs.existsSync(configfile)).to.be.true; @@ -62,7 +66,7 @@ describe('v8flags', function () { it('should not append the file when multiple calls happen concurrently and the config file does not yet exist', function (done) { var v8flags = require('./'); - var configfile = path.resolve(require('user-home'), v8flags.configfile); + var configfile = path.resolve(os.homedir(), v8flags.configfile); async.parallel([v8flags, v8flags, v8flags], function (err, result) { v8flags(function (err2, res) { done();