Skip to content
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

instanceof checks may not be sufficient in node #21

Closed
tandrewnichols opened this issue Dec 2, 2014 · 4 comments
Closed

instanceof checks may not be sufficient in node #21

tandrewnichols opened this issue Dec 2, 2014 · 4 comments
Assignees
Milestone

Comments

@tandrewnichols
Copy link

This is sort of an edge case, so I'd understand if you didn't want to bother with it. I just added this package to our jasmine suite, and .toBeFunction() is reporting "Expected Function to be function." The reason is that we are using sandboxed-module, which loads the subject under test using the vm module (in a separate context). instanceof doesn't seem to work when comparing objects from different contexts. E.g. (in our case):

// vm context - i.e. the subject-under-test
exports.a = function() {}
// test
expect(obj.a).toBeFunction() // fails because obj.a instanceof Function is false

For what's it worth, I don't blame you for this. The vm context instanceof problem is super annoying, and I've run into it before too. You might be able to do other checks in addition to instanceof to cover such cases. Something like return this.actual isntanceof Function || typeof this.actual === 'function' || this.actual.apply.

@JamieMason
Copy link
Owner

Very interesting, thanks for this. I see no harm in hardening the checks to cover this. Thank you.

@tandrewnichols
Copy link
Author

Also for what it's worth, it looks like these are known issues in sandboxed-module that they are trying to fix. See felixge/node-sandboxed-module#44 and felixge/node-sandboxed-module#13.

@JamieMason JamieMason modified the milestone: 1.5x.x Dec 31, 2014
@JamieMason JamieMason self-assigned this Dec 31, 2014
@JamieMason
Copy link
Owner

I think just return typeof this.actual === 'function'; should be enough to cover this, do you agree or is more needed @tandrewnichols?

@tandrewnichols
Copy link
Author

Yeah, I imagine that would be enough.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants