-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Allow tests to tamper with Node.js built-ins without breaking AVA #1359
Comments
Same without Sinon actually: import fs from 'fs';
import test from 'ava';
test(t => {
const _ = fs.readFileSync;
fs.readFileSync = () => 'foo';
const isFoo = require('.');
fs.readFileSync = _;
t.true(isFoo);
});
|
Assuming this is related then.
|
@OmgImAlexis Not necessarily. Doesn't look like that error is coming from AVA or a dependent. |
Going off the warning from c01ac05 I had assumed it was related. If not feel free to remove my comments. |
I seem to get a similar error whenever I change
|
I'm not 100% sure if this is related, but in ava 1.2.x with sinon 7.2.3, when I create a sandbox with mocked timers, I get an error that Ava 1.1.0 is not affected, so I think this has a specific issue with 1.2.0's clearTimeout methods. I'm not certain on the interaction with sinon's sandbox, though. |
If you could open a new issue that'd be great. This issue is more about changing the |
@mrozbarry this should be fixed with #2057. |
@IssueHunt has funded $60.00 to this issue.
|
IMHO achieving this kind of isolation will have such an overhead as to not be worthwhile. |
With latest master (c01ac05) and Node.js 4 and 7.
I think we need to protect all builtins from being overridden, so mocking in the tests doesn't affect our usage or our dependencies' usage of core APIs. Maybe running the test files in a new
vm
context would help with that?The text was updated successfully, but these errors were encountered: