Conversation
f6bbf54 to
84b9225
Compare
|
Tests against the template option (here:effective gid mismatch) failed partly due to the fact that tmp will not join paths with opts.dir nor the default tmpDir. This resulted in the fact that the temporary files and directories will be created in the CWD. The second cause of the failure is more involved as it requires the parent directory to have the setgid bit set, which in turn will set the gid of the file or directory to that of the parent directory and not the effective gid of the current process. While this is not exactly a common requirement, I don't think that we should incorporate this into our tests, as it would involve stat'ing the parent directory and from that get the gid and see whether the gid matches that of the temporary file or directory. Since these are system specific issues, we should not be testing against these. |
3bc7b78 to
d33a22b
Compare
|
Thank you @silkentrance for the hard work, this will help! |
|
@raszi You're welcome. The existing tests that spawn child processes are still missing. I need to go through the code to find out how to do it with mocha... |
854ca99 to
b9ce374
Compare
3f5cfc2 to
4f97577
Compare
8b2a42f to
2eb7397
Compare
|
rebased to master - failing tests due to #143 What do you think about the new tests? |
raszi
left a comment
There was a problem hiding this comment.
I started the review but didn't have time to finish it. Here are my previous comments.
|
|
||
| module.exports.assertName = function assertName(name, expected) { | ||
| assert.ok(typeof name == 'string'); | ||
| assert.ok(name.length > 0, 'an empty string is not a valid name'); |
There was a problem hiding this comment.
I think this should be put in the else branch of the following if condition, since that will be a stricter match and it would not add any value in the true case.
There was a problem hiding this comment.
I meant this:
if (expected) {
assert.equal(path.basename(name), expected, 'should be the expected name');
} else {
assert.ok(name.length > 0, 'an empty string is not a valid name');
}| module.exports = function spawnChildProcess(configFile, cb) { | ||
| var | ||
| node_path = process.argv[0], | ||
| command_args = [ path.join(__dirname, 'spawn.js') ].concat(configFile), |
There was a problem hiding this comment.
Please remove the whitespace after the opening [ and the closing ].
| describe('with invalid tries', function () { | ||
| it('should result in an error on negative tries', function () { | ||
| try { | ||
| tmp.dirSync({tries: -1}); |
There was a problem hiding this comment.
Please use whitespace after the opening { and before the closing }.
There was a problem hiding this comment.
@raszi like so?
it('should result in an error on negative tries', function () {
try {
tmp.dirSync({tries: -1});
assert.fail('should have failed');
} catch (err) {
assert.ok(err instanceof Error);
}
});
ah, forget the above, I think you mean { tries: -1 }... but then again, there are lot of occurrences in the code where I could change that...
|
@raszi I will look over the existing issues with the code and commit these then as I already merged into master. |
fix #137
WIP