test: eval a strict function by kthelgason · Pull Request #5250 · nodejs/node

kthelgason · 2016-02-15T22:58:46Z

This PR adds a regression test for the bug mentioned in #2245 where when a strict function is evaled it would not capture it's context.

Trott · 2016-02-16T05:47:36Z

Test fails for me in 3.x and passes in 4.0.0 and up, which is consistent with it detecting the bug it is designed to test, so that's good. (EDIT: Have to comment out common.js when running with Node 3.x because Node 3.x doesn't have the necessary ES6-isms. That's not a criticism or anything. Just a note for anyone else trying to do the same verification.)

thefourtheye · 2016-02-16T05:52:39Z

@Trott may be because template strings were not available before 4.x?

Trott · 2016-02-16T06:02:16Z

@thefourtheye Yeah, and maybe other stuff too. I think there's at least one arrow functions in there now, for example.

kthelgason · 2016-02-16T09:29:12Z

@Trott @thefourtheye should I rewrite this without template strings to make tests running this on old versions possible?

thefourtheye · 2016-02-16T09:49:07Z

@kthelgason Ya, since we claim that this ~~fixes~~ is a regression for a bug in 0.10, it would be better to be able to test it in that version. Can you please change that?

kthelgason · 2016-02-16T14:09:27Z

@thefourtheye agreed. Fixed and pushed.

thefourtheye · 2016-02-16T14:14:01Z

@@ -0,0 +1,28 @@
+/* eslint-disable strict */


Why this is necessary?

in strict mode running this will fail as bar is undefined until the code has been evaled. If this line is not present the build will fail linting

it would be better to disable the no-undef rule inline

Sorry, I could have been clearer above, the issue here is that this code is not valid in strict mode, since eval in strict mode does not introduce new variables into the surrounding scope. Therefore this file cannot have strict mode enabled, and hence this linter rule.

Oh I see, my bad.

Trott · 2016-02-16T14:56:32Z

This test ran perfectly fine in 3.x (with common.js removed) and there was no need to remove the template strings. Template strings are supported in 3.x.

The problem (which should not be "fixed") was in common.js and was probably the arrow function in that file.

If you want to restore the template string for better code hygiene, feel free.

kthelgason · 2016-02-16T15:00:47Z

@Trott Ok, I still feel that there might be some value to being able to run the test under 0.10 to see the failure.

Trott · 2016-02-16T15:01:56Z

@kthelgason Fair enough. I'm not opposed to the current format or anything.

kthelgason · 2016-03-12T22:02:01Z

@Trott willing to land this or looking for more eyes on it first?

jasnell · 2016-03-12T22:50:38Z

LGTM

thefourtheye · 2016-03-13T07:28:11Z

LGTM. CI Run: https://ci.nodejs.org/job/node-test-pull-request/1912/

PR-URL: #5250 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

jasnell · 2016-03-14T15:22:43Z

Landed in 25eedf0

PR-URL: #5250 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

mscdex added vm Issues and PRs related to the vm subsystem. test Issues and PRs related to the tests. labels Feb 15, 2016

test: eval a strict function
cd5b3fc

kthelgason force-pushed the regr-test-eval-strict branch from f7ea0f9 to cd5b3fcCompare February 16, 2016 14:09

thefourtheye reviewed Feb 16, 2016
View reviewed changes

jasnell added the lts-watch-v4.x label Mar 14, 2016

jasnell pushed a commit that referenced this pull request Mar 14, 2016
test: eval a strict function
25eedf0
PR-URL: #5250 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

jasnell closed this Mar 14, 2016

kthelgason deleted the regr-test-eval-strict branch March 14, 2016 15:49

evanlucas pushed a commit that referenced this pull request Mar 14, 2016
test: eval a strict function
1235176
PR-URL: #5250 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

evanlucas mentioned this pull request Mar 14, 2016
v5.9.0 proposal #5702
Merged

rvagg pushed a commit that referenced this pull request Mar 16, 2016
test: eval a strict function
cceae5a
PR-URL: #5250 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Mar 21, 2016

MylesBorins pushed a commit that referenced this pull request Mar 21, 2016
test: eval a strict function
8c682a3
PR-URL: #5250 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

MylesBorins pushed a commit that referenced this pull request Mar 21, 2016
test: eval a strict function
666d369
PR-URL: #5250 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

jasnell mentioned this pull request Apr 2, 2016
vm.runInContext: eval a strict function #2245
Closed

Uh oh!

Uh oh!

Conversation

kthelgason commented Feb 15, 2016

Uh oh!

Trott commented Feb 16, 2016

Uh oh!

thefourtheye commented Feb 16, 2016

Uh oh!

Trott commented Feb 16, 2016

Uh oh!

kthelgason commented Feb 16, 2016

Uh oh!

thefourtheye commented Feb 16, 2016

Uh oh!

kthelgason commented Feb 16, 2016

Uh oh!

thefourtheyeFeb 16, 2016

Choose a reason for hiding this comment

Uh oh!

kthelgasonFeb 16, 2016

Choose a reason for hiding this comment

Uh oh!

targosFeb 16, 2016

Choose a reason for hiding this comment

Uh oh!

kthelgasonFeb 16, 2016

Choose a reason for hiding this comment

Uh oh!

targosFeb 16, 2016

Choose a reason for hiding this comment

Uh oh!

Trott commented Feb 16, 2016

Uh oh!

kthelgason commented Feb 16, 2016

Uh oh!

Trott commented Feb 16, 2016

Uh oh!

kthelgason commented Mar 12, 2016

Uh oh!

jasnell commented Mar 12, 2016

Uh oh!

thefourtheye commented Mar 13, 2016

Uh oh!

jasnell commented Mar 14, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants