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

test: eval a strict function #5250

Closed
wants to merge 1 commit into from

Conversation

kthelgason
Copy link
Contributor

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.

@mscdex mscdex added vm Issues and PRs related to the vm subsystem. test Issues and PRs related to the tests. labels Feb 15, 2016
@Trott
Copy link
Member

Trott commented Feb 16, 2016

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
Copy link
Contributor

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

@Trott
Copy link
Member

Trott commented Feb 16, 2016

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

@kthelgason
Copy link
Contributor Author

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

@thefourtheye
Copy link
Contributor

@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
Copy link
Contributor Author

@thefourtheye agreed. Fixed and pushed.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this is necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see, my bad.

@Trott
Copy link
Member

Trott commented Feb 16, 2016

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
Copy link
Contributor Author

@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
Copy link
Member

Trott commented Feb 16, 2016

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

@kthelgason
Copy link
Contributor Author

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

@jasnell
Copy link
Member

jasnell commented Mar 12, 2016

LGTM

@thefourtheye
Copy link
Contributor

jasnell pushed a commit that referenced this pull request Mar 14, 2016
PR-URL: #5250
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@jasnell
Copy link
Member

jasnell commented Mar 14, 2016

Landed in 25eedf0

@jasnell jasnell closed this Mar 14, 2016
@kthelgason 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
PR-URL: #5250
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@evanlucas evanlucas mentioned this pull request Mar 14, 2016
rvagg pushed a commit that referenced this pull request Mar 16, 2016
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
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
PR-URL: #5250
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants