Skip to content

Loading…

When js is interpreted using runInNewContext, and assert.throws is handed a RegExp, the expression appears to get transformed into a function instead of an instance of RegExp, this causes the assert.throws method to explode unexpectedly. #693

Closed
lukebayes opened this Issue · 3 comments

2 participants

@lukebayes

Thanks for all your awesome work on such an amazing platform!

I've spent a week or so trying to isolate and figure out the root cause of this problem, but I really can't seem to get any further than this...

The following tests should help explain a little bit.

First, these are the requires that they need:

var vm = require('vm');
var assert = require('assert');

This passes as expected:
common.debug('test RegExp exists in new context');
script = vm.createScript('var expr = /hello/; assert.ok(expr instanceof RegExp)', 'some.js');
script.runInNewContext({ assert : assert });

This expr also passes as expected:
common.debug('test RegExp as argument to function');
script = vm.createScript('function checkExpr(expr) { assert.ok(expr instanceof RegExp); }; checkExpr(/hello/);', 'some.js');
script.runInNewContext({ assert : assert });

Using assert.throws is the only place I can see the problem being reproduced. This expression becomes a string for some reason, adding traces to the assert.js class shows the problem. I first encountered this problem while trying to load and run test content from external files.
common.debug('test RegExp as argument to assert.throws');
script = vm.createScript('assert.throws(function() { throw new Error("hello world"); }, /hello/);', 'some.js');
script.runInNewContext({ assert : assert });

Running these tests results in:

node test/simple/test-script-context.js

DEBUG: test RegExp exists in new context
DEBUG: test RegExp as argument to function
DEBUG: test RegExp as argument to assert.throws

node.js:116
throw e; // process.nextTick error, or 'error' event on first tick
^
TypeError: Expecting a function in instanceof check, but got Error: hello world
at Error.INSTANCE_OF (native)
at expectedException (assert.js:250:10)
at Object._throws (assert.js:285:8)
at Object.throws (assert.js:294:11)
at some.js:1:8
at Object.<anonymous> (/Users/lbayes/Projects/FOSS/NodeJs/test/simple/test-script-context.js:38:8)
at Module._compile (module.js:373:26)
at Object..js (module.js:379:10)
at Module.load (module.js:305:31)
at Function._load (module.js:271:10)

I edited assert.js by adding some log statements to the expectedException function, so that it looks like this:

function expectedException(actual, expected) {
  console.log('------------------------');
  console.log(">> expectedException with actual: " + actual + " and expected: " + expected);
  console.log(">> actual: " + actual + " type: " + typeof(actual));
  console.log(">> expected: " + expected + " type: " + typeof(expected));
  console.log(">> expected is regex?: " + (expected instanceof RegExp));

  // everything else unchanged...

These trace out the following surprising values:

------------------------
>> expectedException with actual: hello world and expected: /hello/
>> actual: hello world type: string
>> expected: /hello/ type: function
>> expected is regex?: false

This is the line in assert.throws that should be returning true:

if (expected instanceof RegExp) {
   return expected.test(actual);

But it can't because expected is a Function, not an object, which causes instanceof to explode.

Please let me know if there's anything else I can try to help figure this out. I was going to dig deeper, but I couldn't figure out where runInNewContext is defined, I assume this is some hook directly into V8?

@herby

The problem is with 'expected instanceof RegExp' which is the known problem when using different contexts - each has its own Object, Array, String, Number, RegExp etc.
In other words - not good inter-context regexp test in assert.throws.

So you're not true when you say 'expected is a Function', it is not, it is a RegExp, but another one, not the one tested in assert.throws (and do not get confused by typeof, typeof any RegExp is 'function', it is correct behaviour, exactly as is said in ES5 specification).

@lukebayes

Hey Herby,

Thanks for the response!

I was hoping this would turn out to be a misunderstanding on my part.

I'll dig a little deeper and see if I can come up with a workaround with this new understanding.

@lukebayes

Based on how contexts work, I went ahead and forked the node sources and created a patch here:

https://github.com/lukebayes/node/tree/assert_in_new_context

I'm unfortunately unable to get the test harness passing locally, but this fix does work for me.

@trevnorris trevnorris added a commit that referenced this issue
@lukebayes lukebayes assert: improve support for new execution contexts
More detailed information in GH-693
bf12399
@isaacs isaacs added a commit that referenced this issue
@lukebayes lukebayes assert: improve support for new execution contexts
More detailed information in GH-693
ae1b0ca
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.