test: add vm module edge cases by fhinkel · Pull Request #11265 · nodejs/node · GitHub

This PR adds two, admittedly contrived, examples that test edge cases of the vm module.

They demonstrate that the if statements if (maybe_rv.IsEmpty()) and
if (maybe_prop_attr.IsNothing()) in the GetterCallback
and the QueryCallback in src/node_contextify.cc
are observable. So far we have no tests that break when these
statements are removed (neither does Jest).

Both GetterCallback and QueryCallback
explicitly check the global_proxy() if a property is
not found on the sandbox. In these tests, the explicit check
inside the callback yields different results than deferring the
check until after the callback. The check is deferred, if the
callbacks do not intercept, i.e., if args.GetReturnValue().Set() is
not called.

Since we're working on fixing several of the vm issues, I'd like to have these
tests so we're at least aware if we cause any breaking changes.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

test

We've been moving toward using block scopes in tests to avoid things like code1, code2, etc.

The joy of let and const 👍 . Fixed.

LGTM with style suggestions and if you can clarify the desc.value thing.

Treacherous comma? It reads as if the "if the callbacks do not intercept" part is its own sentence.

Comma deleted.

I'd either indent the comment or put the brace after the comment.

Not in block-scope anymore because split over different files.

You can use a backticks string if you want, probably easier to read.

Done.

I don't follow why this would be the expected result instead of typeof desc.value === 'undefined' (and typeof desc.get === 'function'.) Is this a test that better belongs in test/known_issues?

Expected because it is the current behavior - not necessarily correct behavior. I wanted to make sure we have a test that breaks if somebody touches these 5 lines of code. But I'm getting more and more convinced, that these lines shouldn't be there. Moving this to known issues.

Add two, admittedly contrived, examples that test edge cases of the vm module. They demonstrate that the if statements `if (maybe_rv.IsEmpty())` and `if (maybe_prop_attr.IsNothing())` in the GetterCallback and the QueryCallback are observable. Both GetterCallback and QueryCallback explicitly check the global_proxy() if a property is not found on the sandbox. In these tests, the explicit check inside the callback yields different results than deferring the check until after the callback. The check is deferred, if the callbacks do not intercept, i.e., if args.GetReturnValue().Set() is not called.

CI https://ci.nodejs.org/job/node-test-pull-request/6335/

Add two, admittedly contrived, examples that test edge cases of the vm module. They demonstrate that the if statements `if (maybe_rv.IsEmpty())` and `if (maybe_prop_attr.IsNothing())` in the GetterCallback and the QueryCallback are observable. Both GetterCallback and QueryCallback explicitly check the global_proxy() if a property is not found on the sandbox. In these tests, the explicit check inside the callback yields different results than deferring the check until after the callback. The check is deferred, if the callbacks do not intercept, i.e., if args.GetReturnValue().Set() is not called. PR-URL: #11265 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Landed in 5cd9d76

Add two, admittedly contrived, examples that test edge cases of the vm module. They demonstrate that the if statements `if (maybe_rv.IsEmpty())` and `if (maybe_prop_attr.IsNothing())` in the GetterCallback and the QueryCallback are observable. Both GetterCallback and QueryCallback explicitly check the global_proxy() if a property is not found on the sandbox. In these tests, the explicit check inside the callback yields different results than deferring the check until after the callback. The check is deferred, if the callbacks do not intercept, i.e., if args.GetReturnValue().Set() is not called. PR-URL: #11265 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Add two, admittedly contrived, examples that test edge cases of the vm module. They demonstrate that the if statements `if (maybe_rv.IsEmpty())` and `if (maybe_prop_attr.IsNothing())` in the GetterCallback and the QueryCallback are observable. Both GetterCallback and QueryCallback explicitly check the global_proxy() if a property is not found on the sandbox. In these tests, the explicit check inside the callback yields different results than deferring the check until after the callback. The check is deferred, if the callbacks do not intercept, i.e., if args.GetReturnValue().Set() is not called. PR-URL: nodejs#11265 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Add two, admittedly contrived, examples that test edge cases of the vm module. They demonstrate that the if statements `if (maybe_rv.IsEmpty())` and `if (maybe_prop_attr.IsNothing())` in the GetterCallback and the QueryCallback are observable. Both GetterCallback and QueryCallback explicitly check the global_proxy() if a property is not found on the sandbox. In these tests, the explicit check inside the callback yields different results than deferring the check until after the callback. The check is deferred, if the callbacks do not intercept, i.e., if args.GetReturnValue().Set() is not called. PR-URL: #11265 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

nodejs-github-bot added the test Issues and PRs related to the tests. label Feb 9, 2017

fhinkel added the vm Issues and PRs related to the vm subsystem. label Feb 9, 2017

cjihrig reviewed Feb 9, 2017

View reviewed changes

fhinkel force-pushed the vm-tests branch 2 times, most recently from a4a5bc8 to d10278c Compare February 9, 2017 17:43

cjihrig approved these changes Feb 9, 2017

View reviewed changes

jasnell approved these changes Feb 9, 2017

View reviewed changes

bnoordhuis approved these changes Feb 10, 2017

View reviewed changes

fhinkel force-pushed the vm-tests branch from d10278c to e4c3c59 Compare February 10, 2017 16:19

fhinkel force-pushed the vm-tests branch from e4c3c59 to 13f05fa Compare February 10, 2017 16:26

jasnell closed this Feb 11, 2017

italoacasas mentioned this pull request Feb 13, 2017

Proposal - 7.6.0 (Current) #11185

Merged

MylesBorins mentioned this pull request Mar 9, 2017

v6.10.1 proposal #11759

Merged

MylesBorins mentioned this pull request Mar 9, 2017

v4.8.1 proposal #11760

Merged

test: add vm module edge cases by fhinkel · Pull Request #11265 · nodejs/node · GitHub

Uh oh!

Uh oh!

test: add vm module edge cases - #11265

test: add vm module edge cases#11265fhinkel wants to merge 1 commit intonodejs:masternodejs/node:masterfrom fhinkel:vm-testsfhinkel/node:vm-testsCopy head branch name to clipboard

Conversation

fhinkel commented Feb 9, 2017

Checklist

Affected core subsystem(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fhinkel commented Feb 10, 2017

Uh oh!

jasnell commented Feb 11, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

test: add vm module edge cases#11265
fhinkel wants to merge 1 commit into
nodejs:masterfrom
fhinkel:vm-tests