-
Notifications
You must be signed in to change notification settings - Fork 42
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
discover native modules from process.moduleLoadList ... #34
Changes from 1 commit
805a5a0
4eceb5a
d66bd1b
8f2c425
88d31a7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,2 +1,4 @@ | ||
*.un~ | ||
/node_modules | ||
**/temp/** | ||
.idea |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
//jshint node:true | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As much as I am a fan of JSHint and There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm good with that ... kind of. I'm 1/2 tempted to make the whole project pass. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not in this commit, at least. |
||
'use strict'; | ||
|
||
// ordinarily I'd use lodash, but let's use native to avoid the dependency | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need for this editorial comment in the source code; in the pull request is fine. |
||
|
||
var moduleLoadList = process.moduleLoadList; | ||
// an array of 'NativeModule assert' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From the code below, it sounds like a more descriptive comment would be: // Looks like ['NativeModule assert', 'NativeModule cluster', ...] or similar. I got confused with the existing comment. |
||
|
||
var natives = moduleLoadList.map(function (item) { | ||
// split by space | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need for comments that duplicate source code. |
||
return item.split(' '); | ||
}).filter(function (item) { | ||
// filter out invalid entries (if any) | ||
// filter out non 'NativeModule' like 'Binding' | ||
// filter out hidden modules (start with _) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm why would we want to filter out hidden modules? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't think they worked. A quick test shows I was incorrect. |
||
return item && item.length === 2 && item[0] === 'NativeModule' && item[1][0] !== '_'; | ||
}).map(function (item) { | ||
// grab the name | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need for comments that duplicate source code. |
||
return item[1]; | ||
}); | ||
|
||
if (natives.indexOf('repl') === -1) { | ||
natives.push('repl'); // No idea why repl doesn't show up in process.moduleLoadList | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indentation in this file is inconsistent with the rest of the code, which uses two spaces. |
||
} | ||
|
||
module.exports = natives; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
//jshint node:true | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This file also needs to conform to the style of the rest of the codebase. |
||
'use strict'; | ||
|
||
// Assert that we can load all the native modules | ||
// FRAGILE: this tests that all the native modules we know about work | ||
// FRAGILE: it doesn't test that the native_modules list is exhaustive | ||
|
||
var assert = require('assert'); | ||
var fs = require('fs'); | ||
var path = require('path'); | ||
var SandboxedModule = require('../..'); | ||
var nativeModules = require('../../lib/native_modules'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After thinking about this test for a while, I am not sure it benefits much to test every single native module. Especially since you're relying on the code that enumerates them inside the test itself. It might be best to just create a single fixture that does There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I debated just picking my favorites, but it's this very scenario where our favorite ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it'd be OK based on the error message from your original bug. And more importantly, it would allow us to check in the fixture, instead of generating it at runtime, which makes the test much easier to understand. You could still use the list from require('../../lib/native_modules').forEach(require); in the fixture. |
||
|
||
var temp = path.join(__dirname, 'temp'); | ||
|
||
fs.mkdir(temp, function (err) { | ||
if (err && err.code === 'EEXIST') { | ||
err = null; // ignore "directory already exists" | ||
} | ||
if (err) { | ||
throw err; | ||
} | ||
|
||
// are there any modules? | ||
assert.ok(nativeModules.length > 5, 'error discovering native modules'); | ||
|
||
nativeModules.forEach(function (modName) { | ||
var filename = path.join(temp, 'fixture_'+modName+'.js'); | ||
var requireName = './temp/fixture_'+modName; | ||
var content = 'module.exports.'+modName+' = require(\''+modName+'\');'; | ||
|
||
// write a file that depends on this module | ||
fs.writeFile(filename, content, function (err) { | ||
if (err) { | ||
throw err; | ||
} | ||
|
||
// load that file | ||
var requireModule = SandboxedModule.require(requireName); | ||
var nestDependency = requireModule[modName]; | ||
var directDependency = require(modName); | ||
// if we didn't die, we can load this module | ||
assert.ok(requireModule, modName + ' is blank'); // assert not blank | ||
if (modName !== 'module' && modName !== 'repl') { // no idea why these two are different | ||
assert.strictEqual(requireModule[modName], require(modName), modName + ' is different'); // assert we get the same result as just requiring it | ||
} | ||
|
||
}); | ||
}); | ||
}); |
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.
Don't add these here, especially not ones that are environment-specific (like .idea). For those you should use your global machine .gitignore.
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.
As part of the test, I create the temp folder inside the integration folder. I'm good with removing
.idea
, but not so much with removing/temp/
. I'm good with a more specific path though. See https://github.com/robrich/node-sandboxed-module/blob/master/test/integration/test-all-native-modules.js#L14There 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.
Oh, you should remove any files you create during a test; tests need to clean up after themselves.