discover native modules from process.moduleLoadList ... #34
Conversation
…a hard-coded list
| @@ -1,2 +1,4 @@ | |||
| *.un~ | |||
| /node_modules | |||
| **/temp/** | |||
There was a problem hiding this comment.
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.
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#L14
There was a problem hiding this comment.
Oh, you should remove any files you create during a test; tests need to clean up after themselves.
|
Very impressive discovery; excited to have this. Lots of nits, but good stuff overall. |
| var fs = require('fs'); | ||
| var path = require('path'); | ||
| var SandboxedModule = require('../..'); | ||
| var nativeModules = require('../../lib/native_modules'); |
There was a problem hiding this comment.
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 require(require('../lib/native_modules')[0]), or maybe does a few of those, and then test that single fixture, in a similar fashion to the rest of the tests.
There was a problem hiding this comment.
I debated just picking my favorites, but it's this very scenario where our favorite (path) wasn't the problem. Flexing them all runs nearly instantaneously, so I don't see the harm in it. I debated writing one file that required them all, then running SandboxedModule.require() on that file, but then it'd be much less discoverable which module did bad things.
There was a problem hiding this comment.
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 lib/native_modules, just do
require('../../lib/native_modules').forEach(require);in the fixture.
|
Changes made, Travis went green. |
|
What I really like about using the discovered list to test them all is it's also future-proof. If they include another native module or deprecate one, the test still works. I grant it's not as discoverable, thus the excessive comments, but I do think it's a safer design. |
|
Yeah, let's use the discovered list, but with a static checked-in fixture instead of writing a bunch of temp files. |
…ch to get at NativeModule which has the exhaustive list.
…oesn't use temp files
|
I rebooted, and suddenly |
|
Let me be clearer: I don't want any tests in this repo that create temporary files on the disk :) |
|
Thank you for clarifying. |
|
Will this be re-pulled at some point? I'm running into an issue with a module loading winston-syslog, which has native code to it, on which the sandbox loaders pukes. |
|
Yes, I've been meaning to revive this and just remove the temp-file creating tests before merging. Thanks for chiming in; that helps me prioritize. |
|
Use proxyquire. It has no such issues. |
|
domenic: Great. :) |
... instead of using a hard-coded list