global test case variables in feature tests

[johnjbarton]

I've reworked the feature test harness into es6 but one of the tests won't pass. It transcodes to:

0 'use strict';
1 var $__1,
2     $__2,
3     $__3,
4     $__4;
5 console.log('t a=', typeof a, ' t b=', typeof b, ' t c=', typeof c);
6 console.log('a=', a, ' b=', b, ' c=', c);
7 var a,
8     b,
9     c,
10     x,
11     y,
12     z;
13 console.log('a=', a, ' b=', b, ' c=', c);
14 function checkA() {
15   console.log('a=', a, ' b=', b, ' c=', c);
16   assert.equal(a, 1);
17   assert.isUndefined(b);
18   assert.isUndefined(c);
19   a = b = c = undefined;
20 }
21 var a = ($__3 = (42 === 42 ? [1] : [42])[$traceurRuntime.toProperty(Symbol.iterator)](), ($__4 = $__3.next()).done ? void 0 : $__4.value);
22 checkA();
23 

Here is the output

t a= number  t b= number  t c= number
a= 0  b= 1  c= 3
a= 0  b= 1  c= 3
a= 1  b= 1  c= 3

The new version uses V8 runInTheContext which purports to resemble (0,eval)('code'), essentially the call used in the older version. It seems this isn't quite true.

I'm not surprised that we somehow polluted the global space with values a, b,c, but I am surprised that combination of "use strict" and var a; did not save us.

Anyway any suggestions? Of course I could force the tests to run again with eval() but I was hoping to use the same code for all the node evaluations.

[arv]

Did you mean runInContext, runInThisContext or runInNewContext?

Whichever one you are using it looks like you are reusing a context since that is the only way that typeof a can be number on line 5.

I'm not surprised that we somehow polluted the global space with values a, b,c, but I am surprised that combination of "use strict" and var a; did not save us.

'use strict' only reports error if you try to read or write to a non declared binding and there is no property on the global object.

'use strict';
global.x = 1;
x = 2;  // OK
y = 3;  // ReferenceError

[johnjbarton]

Sorry, runInThisContext. For module loading I think that is correct. It's actually the current behavior that confuses me. If some other test says var a = 1; (and that seems likely and what I observe now), then how can a be undefined in Simplify.js?

Shouldn't we be running these tests in IIFE or loading them as modules anyway?

[domenic]

It's possible vm and/or V8 has bugs here :-/. Minimal repros appreciated. These seem potentially related. nodejs/node#769 nodejs/node-v0.x-archive#9084 nodejs/node#548 Especially nodejs/node-v0.x-archive#9084 (comment)

[arv]

Shouldn't we be running these tests in IIFE or loading them as modules anyway?

Definitely not. Script code has different semantics than Module and FunctionBody.

[arv]

Simplify.js: Global vars creates properties on the global (in Script).

What we probably should do is to create a new context for every test but I am concerned about the performance of doing that.

[domenic]

Context creation is much faster in io.js 2.0.2 onward now that snapshots have been re-enabled.

[arv]

Sure. But nothing is going to beat global eval. Still, if the performance regression of the tests is acceptable it would be a lot better to use new contexts to remove interdependencies.

[johnjbarton]

I'm still unclear. Consider this Script:

"use strict"
var b = 'b';

Followed by this one:

"use strict"
var a, b;
a = 5;
assert.isUndefined(b);

It's my understanding that after the second script completes we should assert. If we execute these in the other order, we should not assert. Thus I believe the test is incorrect: we can't assert b is undefined because we don't know the status of the global b.

[arv]

I agree with you. The tests sometimes break due to changes in other files. When that happens a more unique name is usually used.

[johnjbarton]

In the case of the Simplify test, the assert.isUndefined(b); is really trying to verify that we did not assign to b in the test. So I think we can initialize the test variables to a unique value and check that instead.