-
Notifications
You must be signed in to change notification settings - Fork 93
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test: refactor tests for mocha #141
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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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');
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove the whitespace after the opening [
and the closing ]
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use whitespace after the opening {
and before the closing }
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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