Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2005613002:
[test] add debugger scopes testing for async functions (Closed)
|
Created:
4 years, 7 months ago by caitp (gmail) Modified:
4 years, 7 months ago CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[test] add debugger scopes testing for async functions
BUG=v8:4483
R=littledan@chromium.org, yangguo@chromium.org
Committed: https://crrev.com/38c6fb82f35b08a0e9d0afef644d74bc6d9d2ab1
Cr-Commit-Position: refs/heads/master@{#36489}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Fixups #Patch Set 3 : Exit early when exception occurs #Messages
Total messages: 12 (3 generated)
PTAL --- based on `generators-debug-scopes.js`. I had originally made different versions of this for all the various syntactic forms, but it is a fairly long running test (especially in debug builds), so it may not be worth doing. But, we probably do at least need tests for eval via AsyncFunction constructor, since it has different test results from the other ones. And something similar for async arrows (with the various lexical bindings) would be good too.
PTAL --- based on `generators-debug-scopes.js`. I had originally made different versions of this for all the various syntactic forms, but it is a fairly long running test (especially in debug builds), so it may not be worth doing. But, we probably do at least need tests for eval via AsyncFunction constructor, since it has different test results from the other ones. And something similar for async arrows (with the various lexical bindings) would be good too.
LGTM. This is awesome. Thanks! I have some small nits. https://codereview.chromium.org/2005613002/diff/1/test/mjsunit/harmony/async-... File test/mjsunit/harmony/async-function-debug-scopes.js (right): https://codereview.chromium.org/2005613002/diff/1/test/mjsunit/harmony/async-... test/mjsunit/harmony/async-function-debug-scopes.js:43: throw exception; Can we use %Abort here? My experience with Promises at least suggest that throwing exceptions runs the risk of it getting swallowed, and the test passes even though it must not. https://codereview.chromium.org/2005613002/diff/1/test/mjsunit/harmony/async-... test/mjsunit/harmony/async-function-debug-scopes.js:175: CheckScopeContent({a:1,b:2,x:3,y:4}, 0, exec_state); Please format the object literal for easier readability. (And below) https://codereview.chromium.org/2005613002/diff/1/test/mjsunit/harmony/async-... test/mjsunit/harmony/async-function-debug-scopes.js:348: CheckScopeChain([debug.ScopeType.Block, // Is this correct? I think it is. Using let within a with-scope should create a new block-scope, right? https://codereview.chromium.org/2005613002/diff/1/test/mjsunit/harmony/async-... test/mjsunit/harmony/async-function-debug-scopes.js:485: assertPropertiesEqual(scope1.scopeObject().value(), scope2.scopeObject().value()); Can we add a line break to make sure we stay within 80 characters per line? (And below as well). I know you probably just copied these over from another test, but let's not repeat these small mistakes :)
Mostly fixed up (haven't added the %AbortJS() yet, explained why inline) https://codereview.chromium.org/2005613002/diff/1/test/mjsunit/harmony/async-... File test/mjsunit/harmony/async-function-debug-scopes.js (right): https://codereview.chromium.org/2005613002/diff/1/test/mjsunit/harmony/async-... test/mjsunit/harmony/async-function-debug-scopes.js:43: throw exception; On 2016/05/23 06:55:20, Yang wrote: > Can we use %Abort here? My experience with Promises at least suggest that > throwing exceptions runs the risk of it getting swallowed, and the test passes > even though it must not. This thrown exception gets caught in a catch handler down below ``` 475 runTests().catch(error => { 476 print(error.stack); 477 quit(1); 478 }); ``` I think this is okay, and it's a bit easier to read with quit() vs %AbortJS(), due to not filling the screen up with debug output. But, if you need it to change, let me know https://codereview.chromium.org/2005613002/diff/1/test/mjsunit/harmony/async-... test/mjsunit/harmony/async-function-debug-scopes.js:175: CheckScopeContent({a:1,b:2,x:3,y:4}, 0, exec_state); On 2016/05/23 06:55:20, Yang wrote: > Please format the object literal for easier readability. (And below) Acknowledged. https://codereview.chromium.org/2005613002/diff/1/test/mjsunit/harmony/async-... test/mjsunit/harmony/async-function-debug-scopes.js:348: CheckScopeChain([debug.ScopeType.Block, // Is this correct? On 2016/05/23 06:55:20, Yang wrote: > I think it is. Using let within a with-scope should create a new block-scope, > right? I think it makes sense for the implementation (AFAIK if let/const is not used, FinalizeBlockScope kills the block scope?) --- But it's still confusing and not clear that it's really doing anything helpful. Anyways, will remove the comment https://codereview.chromium.org/2005613002/diff/1/test/mjsunit/harmony/async-... test/mjsunit/harmony/async-function-debug-scopes.js:485: assertPropertiesEqual(scope1.scopeObject().value(), scope2.scopeObject().value()); On 2016/05/23 06:55:20, Yang wrote: > Can we add a line break to make sure we stay within 80 characters per line? (And > below as well). I know you probably just copied these over from another test, > but let's not repeat these small mistakes :) Acknowledged.
lgtm with comment https://codereview.chromium.org/2005613002/diff/1/test/mjsunit/harmony/async-... File test/mjsunit/harmony/async-function-debug-scopes.js (right): https://codereview.chromium.org/2005613002/diff/1/test/mjsunit/harmony/async-... test/mjsunit/harmony/async-function-debug-scopes.js:43: throw exception; On 2016/05/24 15:39:37, caitp wrote: > On 2016/05/23 06:55:20, Yang wrote: > > Can we use %Abort here? My experience with Promises at least suggest that > > throwing exceptions runs the risk of it getting swallowed, and the test passes > > even though it must not. > > This thrown exception gets caught in a catch handler down below > > ``` > 475 runTests().catch(error => { > 476 print(error.stack); > 477 quit(1); > 478 }); > ``` > > I think this is okay, and it's a bit easier to read with quit() vs %AbortJS(), > due to not filling the screen up with debug output. But, if you need it to > change, let me know Can we print the stack trace and quit here, instead of relying on catch to work correctly down the line?
https://codereview.chromium.org/2005613002/diff/1/test/mjsunit/harmony/async-... File test/mjsunit/harmony/async-function-debug-scopes.js (right): https://codereview.chromium.org/2005613002/diff/1/test/mjsunit/harmony/async-... test/mjsunit/harmony/async-function-debug-scopes.js:43: throw exception; On 2016/05/24 16:22:15, Yang wrote: > On 2016/05/24 15:39:37, caitp wrote: > > On 2016/05/23 06:55:20, Yang wrote: > > > Can we use %Abort here? My experience with Promises at least suggest that > > > throwing exceptions runs the risk of it getting swallowed, and the test > passes > > > even though it must not. > > > > This thrown exception gets caught in a catch handler down below > > > > ``` > > 475 runTests().catch(error => { > > 476 print(error.stack); > > 477 quit(1); > > 478 }); > > ``` > > > > I think this is okay, and it's a bit easier to read with quit() vs %AbortJS(), > > due to not filling the screen up with debug output. But, if you need it to > > change, let me know > > Can we print the stack trace and quit here, instead of relying on catch to work > correctly down the line? Done
The CQ bit was checked by caitpotter88@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from yangguo@chromium.org Link to the patchset: https://codereview.chromium.org/2005613002/#ps40001 (title: "Exit early when exception occurs")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2005613002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2005613002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [test] add debugger scopes testing for async functions BUG=v8:4483 R=littledan@chromium.org, yangguo@chromium.org ========== to ========== [test] add debugger scopes testing for async functions BUG=v8:4483 R=littledan@chromium.org, yangguo@chromium.org Committed: https://crrev.com/38c6fb82f35b08a0e9d0afef644d74bc6d9d2ab1 Cr-Commit-Position: refs/heads/master@{#36489} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/38c6fb82f35b08a0e9d0afef644d74bc6d9d2ab1 Cr-Commit-Position: refs/heads/master@{#36489} |
