test: add vm module edge cases#11265
Closed
Uh oh!
There was an error while loading. Please reload this page.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
cjihrig reviewed Feb 9, 2017
Contributor
There was a problem hiding this comment.
We've been moving toward using block scopes in tests to avoid things like code1, code2, etc.
MemberAuthor
There was a problem hiding this comment.
The joy of let and const 👍 . Fixed.
fhinkelforce-pushed the vm-tests branch 2 times, most recently from a4a5bc8 to d10278cComparecjihrig approved these changes Feb 9, 2017
jasnell approved these changes Feb 9, 2017
bnoordhuis approved these changes Feb 10, 2017
Member
bnoordhuis left a comment
bnoordhuis left a comment There was a problem hiding this comment.
LGTM with style suggestions and if you can clarify the desc.value thing.
Member
There was a problem hiding this comment.
Treacherous comma? It reads as if the "if the callbacks do not intercept" part is its own sentence.
Member
There was a problem hiding this comment.
I'd either indent the comment or put the brace after the comment.
MemberAuthor
There was a problem hiding this comment.
Not in block-scope anymore because split over different files.
Member
There was a problem hiding this comment.
You can use a backticks string if you want, probably easier to read.
Member
There was a problem hiding this comment.
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?
MemberAuthor
There was a problem hiding this comment.
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.
MemberAuthor
fhinkel commented Feb 10, 2017
jasnell pushed a commit that referenced this pull request Feb 11, 2017
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
Member
jasnell commented Feb 11, 2017
Landed in 5cd9d76 |
italoacasas pushed a commit that referenced this pull request Feb 13, 2017
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Feb 14, 2017
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
krydos pushed a commit to krydos/node that referenced this pull request Feb 25, 2017
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
jasnell pushed a commit that referenced this pull request Mar 7, 2017
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
jasnell pushed a commit that referenced this pull request Mar 7, 2017
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
Merged
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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())andif (maybe_prop_attr.IsNothing())in the GetterCallbackand 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), orvcbuild test(Windows) passesAffected core subsystem(s)
test