Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 8 months ago by Clemens Hammacher Modified:
4 years, 8 months ago Reviewers:
titzer, JF, Michael Achenbach CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@wasm-throw-error Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[wasm] Also test structured stack trace
This extends the wasm test case which only checks the "simple"
string-variant of the stack trace.
It checks the return values of the getFunctionName, getLineNumber,
getFileName and toString methods.
R=machenbach@chromium.org, jfb@chromium.org, titzer@chromium.org
Committed: https://crrev.com/449af6f2295e0bfc7db751e32bfe42f7a3110179
Cr-Commit-Position: refs/heads/master@{#35687}
Patch Set 1 #
Total comments: 1
Patch Set 2 : fix assertContains helper function #
Total comments: 2
Patch Set 3 : address JS's comments #Patch Set 4 : change order of assertContains arguments #Patch Set 5 : remove line from mjsunit.status #
Created: 4 years, 8 months ago
Messages
Total messages: 23 (7 generated)
Tests are sad. https://codereview.chromium.org/1875153002/diff/1/test/mjsunit/wasm/stack.js File test/mjsunit/wasm/stack.js (right): https://codereview.chromium.org/1875153002/diff/1/test/mjsunit/wasm/stack.js#... test/mjsunit/wasm/stack.js:5: // Flags: --expose-wasm Start the file with: // clang-format off I should have done it a while ago, it's a PITA to format the file and have to change line / column in the expected backtrace.
On 2016/04/11 at 18:18:15, jfb wrote: > Tests are sad. should be better now. > https://codereview.chromium.org/1875153002/diff/1/test/mjsunit/wasm/stack.js#... > test/mjsunit/wasm/stack.js:5: // Flags: --expose-wasm > Start the file with: > // clang-format off > > I should have done it a while ago, it's a PITA to format the file and have to change line / column in the expected backtrace. How is this related to clang-format?
> https://codereview.chromium.org/1875153002/diff/1/test/mjsunit/wasm/stack.js#... > > test/mjsunit/wasm/stack.js:5: // Flags: --expose-wasm > > Start the file with: > > // clang-format off > > > > I should have done it a while ago, it's a PITA to format the file and have to > change line / column in the expected backtrace. > > How is this related to clang-format? I found the presubmit check on clang-format annoying because doing it forces you to also change the expected output. When a new version of clang-format decides that things are formatted differently than previously it's causing needless churn. https://codereview.chromium.org/1875153002/diff/20001/test/mjsunit/wasm/stack.js File test/mjsunit/wasm/stack.js (right): https://codereview.chromium.org/1875153002/diff/20001/test/mjsunit/wasm/stack... test/mjsunit/wasm/stack.js:98: })(); I find that structure hard to read, IMO it would be better to have something like: [ // func name line file name toString ["STACK", 54, "stack.js", "stack.js:54:11"], ["<WASM>", null, null, "<WASM>"], ["testStackFrames", 87, "stack.js", "stack.js:87:18"], [null, 98, "stack.js", "stack.js:98:3"], ].verifyContains(stack, ...);
https://codereview.chromium.org/1875153002/diff/20001/test/mjsunit/wasm/stack.js File test/mjsunit/wasm/stack.js (right): https://codereview.chromium.org/1875153002/diff/20001/test/mjsunit/wasm/stack... test/mjsunit/wasm/stack.js:98: })(); On 2016/04/11 at 19:35:09, JF wrote: > I find that structure hard to read, IMO it would be better to have something like: > > [ > // func name line file name toString > ["STACK", 54, "stack.js", "stack.js:54:11"], > ["<WASM>", null, null, "<WASM>"], > ["testStackFrames", 87, "stack.js", "stack.js:87:18"], > [null, 98, "stack.js", "stack.js:98:3"], > ].verifyContains(stack, ...); Sure, that's nicer. Changed it accordingly.
lgtm
On 2016/04/12 18:20:53, JF wrote: > lgtm lgtm
The CQ bit was checked by clemensh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1875153002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1875153002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/1903293002/ by machenbach@chromium.org. The reason for reverting is: Breaks: https://build.chromium.org/p/client.v8/builders/V8%20Linux64%20-%20custom%20s....
On 2016/04/20 at 16:23:00, machenbach wrote: > A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/1903293002/ by machenbach@chromium.org. > > The reason for reverting is: Breaks: > https://build.chromium.org/p/client.v8/builders/V8%20Linux64%20-%20custom%20s.... This CL now also removes the line in mjsunit.status for ignoring test failures. @machenbach, can I have your LGTM please? ;)
clemensh@chromium.org changed reviewers: + machenbach@chromium.org - ahaas@chromium.org
lgtm
Description was changed from ========== [wasm] Also test structured stack trace This extends the wasm test case which only checks the "simple" string-variant of the stack trace. It checks the return values of the getFunctionName, getLineNumber, getFileName and toString methods. R=ahaas@chromium.org, jfb@chromium.org, titzer@chromium.org BUG= ========== to ========== [wasm] Also test structured stack trace This extends the wasm test case which only checks the "simple" string-variant of the stack trace. It checks the return values of the getFunctionName, getLineNumber, getFileName and toString methods. R=machenbach@chromium.org, jfb@chromium.org, titzer@chromium.org ==========
The CQ bit was checked by clemensh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from titzer@chromium.org, jfb@chromium.org Link to the patchset: https://codereview.chromium.org/1875153002/#ps80001 (title: "remove line from mjsunit.status")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1875153002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1875153002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [wasm] Also test structured stack trace This extends the wasm test case which only checks the "simple" string-variant of the stack trace. It checks the return values of the getFunctionName, getLineNumber, getFileName and toString methods. R=machenbach@chromium.org, jfb@chromium.org, titzer@chromium.org ========== to ========== [wasm] Also test structured stack trace This extends the wasm test case which only checks the "simple" string-variant of the stack trace. It checks the return values of the getFunctionName, getLineNumber, getFileName and toString methods. R=ahaas@chromium.org, jfb@chromium.org, titzer@chromium.org BUG= Committed: https://crrev.com/782c204c80b56b1bae2e8e2a937b4e6258630d61 Cr-Commit-Position: refs/heads/master@{#35666} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/782c204c80b56b1bae2e8e2a937b4e6258630d61 Cr-Commit-Position: refs/heads/master@{#35666}
Message was sent while issue was closed.
Description was changed from ========== [wasm] Also test structured stack trace This extends the wasm test case which only checks the "simple" string-variant of the stack trace. It checks the return values of the getFunctionName, getLineNumber, getFileName and toString methods. R=ahaas@chromium.org, jfb@chromium.org, titzer@chromium.org BUG= Committed: https://crrev.com/782c204c80b56b1bae2e8e2a937b4e6258630d61 Cr-Commit-Position: refs/heads/master@{#35666} ========== to ========== [wasm] Also test structured stack trace This extends the wasm test case which only checks the "simple" string-variant of the stack trace. It checks the return values of the getFunctionName, getLineNumber, getFileName and toString methods. R=machenbach@chromium.org, jfb@chromium.org, titzer@chromium.org Committed: https://crrev.com/449af6f2295e0bfc7db751e32bfe42f7a3110179 Cr-Commit-Position: refs/heads/master@{#35687} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/449af6f2295e0bfc7db751e32bfe42f7a3110179 Cr-Commit-Position: refs/heads/master@{#35687} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
