Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2591233004:
Provide detailed information on the test failure (Closed)
|
Created:
3 years, 12 months ago by hongchan Modified:
3 years, 12 months ago Reviewers:
Raymond Toy CC:
chromium-reviews, blink-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionProvide detailed information on the test failure
Currently Audit's assertion chooses either the actual value or the
description about it. Modify the code so the failure test case can have
both information in the error message.
Now the failed assertion will have 'Got ${actual value}' at the end of
error message.
BUG=676348
TEST=LayoutTests/webaudio/unit-tests/audit-failures.html
Committed: https://crrev.com/15a4b6f89805292c7a2c140cc7ee4a42bc78d26d
Cr-Commit-Position: refs/heads/master@{#440268}
Patch Set 1 : Initial patch #Patch Set 2 : Fixing nits #Patch Set 3 : Update expected result #
Total comments: 13
Patch Set 4 : Addressing feedback #
Total comments: 8
Patch Set 5 : Fixing nits after l-g-t-m #
Messages
Total messages: 14 (6 generated)
hongchan@chromium.org changed reviewers: + rtoy@chromium.org
PTAL.
Excited to see this! Just a few small quibbles. https://codereview.chromium.org/2591233004/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/resources/audit.js (right): https://codereview.chromium.org/2591233004/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/audit.js:138: _throwException('Illegal invocation: the assertion is not finished.'); What does "not finished" really mean here? https://codereview.chromium.org/2591233004/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/audit.js:140: let actualString = _generateDescription(this._actual, this._options); Would actualResult be a better name? https://codereview.chromium.org/2591233004/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/audit.js:178: // If the test is failed, add the actual value at the end. s/test is/test/ https://codereview.chromium.org/2591233004/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/unit-tests/audit-failures-expected.txt (right): https://codereview.chromium.org/2591233004/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/unit-tests/audit-failures-expected.txt:6: FAIL X function () { var foo = 0; } did not throw an exception. Got function () { var foo = 0; }. assert_true: expected true got false For notThrow() and throw(), would it make sense not to print out the actual value? https://codereview.chromium.org/2591233004/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/unit-tests/audit-failures.html (right): https://codereview.chromium.org/2591233004/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/unit-tests/audit-failures.html:51: should(0).beCloseTo(0.1, { threshold: 0 }); Would be useful to have a test just like this but with a description.
https://codereview.chromium.org/2591233004/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/resources/audit.js (right): https://codereview.chromium.org/2591233004/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/audit.js:138: _throwException('Illegal invocation: the assertion is not finished.'); On 2016/12/21 18:58:12, Raymond Toy wrote: > What does "not finished" really mean here? Building result text must be done after the assertion is finished. If any code tries to call this method before the assertion, it will throw. https://codereview.chromium.org/2591233004/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/audit.js:140: let actualString = _generateDescription(this._actual, this._options); On 2016/12/21 18:58:12, Raymond Toy wrote: > Would actualResult be a better name? This is a stringified result. So I would like to keep this name unless you want something like 'stringifiedActualResult'. https://codereview.chromium.org/2591233004/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/audit.js:178: // If the test is failed, add the actual value at the end. On 2016/12/21 18:58:12, Raymond Toy wrote: > s/test is/test/ Done. https://codereview.chromium.org/2591233004/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/unit-tests/audit-failures-expected.txt (right): https://codereview.chromium.org/2591233004/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/unit-tests/audit-failures-expected.txt:6: FAIL X function () { var foo = 0; } did not throw an exception. Got function () { var foo = 0; }. assert_true: expected true got false On 2016/12/21 18:58:12, Raymond Toy wrote: > For notThrow() and throw(), would it make sense not to print out the actual > value? Done. https://codereview.chromium.org/2591233004/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/unit-tests/audit-failures.html (right): https://codereview.chromium.org/2591233004/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/unit-tests/audit-failures.html:51: should(0).beCloseTo(0.1, { threshold: 0 }); On 2016/12/21 18:58:13, Raymond Toy wrote: > Would be useful to have a test just like this but with a description. Done.
lgtm with nits https://codereview.chromium.org/2591233004/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/resources/audit.js (right): https://codereview.chromium.org/2591233004/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/audit.js:138: _throwException('Illegal invocation: the assertion is not finished.'); On 2016/12/21 21:30:02, hongchan wrote: > On 2016/12/21 18:58:12, Raymond Toy wrote: > > What does "not finished" really mean here? > > Building result text must be done after the assertion is finished. If any code > tries to call this method before the assertion, it will throw. So this is really an error in Audit? Not something I can do by accident in a test? If so, an internal _assert method of some sort would be nice to indicate that it's an internal check. https://codereview.chromium.org/2591233004/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/audit.js:140: let actualString = _generateDescription(this._actual, this._options); On 2016/12/21 21:30:02, hongchan wrote: > On 2016/12/21 18:58:12, Raymond Toy wrote: > > Would actualResult be a better name? > > This is a stringified result. So I would like to keep this name unless you want > something like 'stringifiedActualResult'. I find "String" not really relevant since it's the result, but ok to just leave it. https://codereview.chromium.org/2591233004/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/resources/audit.js (right): https://codereview.chromium.org/2591233004/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/audit.js:179: if (this._result === false && this._printActualForFailure === true) { Is it really important for these to be exactly false and exactly true? https://codereview.chromium.org/2591233004/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/unit-tests/audit-failures-expected.txt (right): https://codereview.chromium.org/2591233004/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/unit-tests/audit-failures-expected.txt:16: FAIL X Decoding audio data with no argument rejected incorrectly with TypeError: Failed to execute 'decodeAudioData' on 'BaseAudioContext': 1 argument required, but only 0 present.. Got undefined. assert_true: expected true got false "present.." has two periods. (I know that's not part of the CL, so you can ignore this.) https://codereview.chromium.org/2591233004/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/unit-tests/audit-failures.html (right): https://codereview.chromium.org/2591233004/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/unit-tests/audit-failures.html:30: should(1, 'one').beEqualTo(2); I meant in addition to the case without a description. And I think "Result" or something is better than "one". The descriptions should be more like what you would actually write in a real test. https://codereview.chromium.org/2591233004/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/unit-tests/audit-failures.html:52: should(0, 'zero').beCloseTo(0.1, { threshold: 0 }); Use some better, more typical description. "zero" in the actual output makes it just look weird and confusing. (It's ok for the test in line 51 to output 0, though.)
https://codereview.chromium.org/2591233004/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/resources/audit.js (right): https://codereview.chromium.org/2591233004/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/audit.js:140: let actualString = _generateDescription(this._actual, this._options); On 2016/12/21 21:57:37, Raymond Toy wrote: > On 2016/12/21 21:30:02, hongchan wrote: > > On 2016/12/21 18:58:12, Raymond Toy wrote: > > > Would actualResult be a better name? > > > > This is a stringified result. So I would like to keep this name unless you > want > > something like 'stringifiedActualResult'. > > I find "String" not really relevant since it's the result, but ok to just leave > it. Acknowledged. https://codereview.chromium.org/2591233004/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/resources/audit.js (right): https://codereview.chromium.org/2591233004/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/audit.js:179: if (this._result === false && this._printActualForFailure === true) { On 2016/12/21 21:57:37, Raymond Toy wrote: > Is it really important for these to be exactly false and exactly true? Yes. To display the additional actual value at the end, the test case must be failed and the assertion should explicitly indicate "I need this info to be printed." Otherwise we don't print anything additionally. (e.g. a passed case or a failure case with array log) https://codereview.chromium.org/2591233004/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/unit-tests/audit-failures-expected.txt (right): https://codereview.chromium.org/2591233004/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/unit-tests/audit-failures-expected.txt:16: FAIL X Decoding audio data with no argument rejected incorrectly with TypeError: Failed to execute 'decodeAudioData' on 'BaseAudioContext': 1 argument required, but only 0 present.. Got undefined. assert_true: expected true got false On 2016/12/21 21:57:37, Raymond Toy wrote: > "present.." has two periods. (I know that's not part of the CL, so you can > ignore this.) Acknowledged. https://codereview.chromium.org/2591233004/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/unit-tests/audit-failures.html (right): https://codereview.chromium.org/2591233004/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/unit-tests/audit-failures.html:30: should(1, 'one').beEqualTo(2); On 2016/12/21 21:57:37, Raymond Toy wrote: > I meant in addition to the case without a description. And I think "Result" or > something is better than "one". The descriptions should be more like what you > would actually write in a real test. Done. https://codereview.chromium.org/2591233004/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/unit-tests/audit-failures.html:52: should(0, 'zero').beCloseTo(0.1, { threshold: 0 }); On 2016/12/21 21:57:37, Raymond Toy wrote: > Use some better, more typical description. "zero" in the actual output makes it > just look weird and confusing. (It's ok for the test in line 51 to output 0, > though.) Done.
The CQ bit was checked by hongchan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rtoy@chromium.org Link to the patchset: https://codereview.chromium.org/2591233004/#ps80001 (title: "Fixing nits after l-g-t-m")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1482360241192910,
"parent_rev": "bab0ef82af46fb951b2f5f0113ab463a2bdd76c8", "commit_rev":
"f8e67b90c9ee4ba81ebb8aaecc9cea4deecde49e"}
Message was sent while issue was closed.
Description was changed from
==========
Provide detailed information on the test failure
Currently Audit's assertion chooses either the actual value or the
description about it. Modify the code so the failure test case can have
both information in the error message.
Now the failed assertion will have 'Got ${actual value}' at the end of
error message.
BUG=676348
TEST=LayoutTests/webaudio/unit-tests/audit-failures.html
==========
to
==========
Provide detailed information on the test failure
Currently Audit's assertion chooses either the actual value or the
description about it. Modify the code so the failure test case can have
both information in the error message.
Now the failed assertion will have 'Got ${actual value}' at the end of
error message.
BUG=676348
TEST=LayoutTests/webaudio/unit-tests/audit-failures.html
Review-Url: https://codereview.chromium.org/2591233004
==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from
==========
Provide detailed information on the test failure
Currently Audit's assertion chooses either the actual value or the
description about it. Modify the code so the failure test case can have
both information in the error message.
Now the failed assertion will have 'Got ${actual value}' at the end of
error message.
BUG=676348
TEST=LayoutTests/webaudio/unit-tests/audit-failures.html
Review-Url: https://codereview.chromium.org/2591233004
==========
to
==========
Provide detailed information on the test failure
Currently Audit's assertion chooses either the actual value or the
description about it. Modify the code so the failure test case can have
both information in the error message.
Now the failed assertion will have 'Got ${actual value}' at the end of
error message.
BUG=676348
TEST=LayoutTests/webaudio/unit-tests/audit-failures.html
Committed: https://crrev.com/15a4b6f89805292c7a2c140cc7ee4a42bc78d26d
Cr-Commit-Position: refs/heads/master@{#440268}
==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/15a4b6f89805292c7a2c140cc7ee4a42bc78d26d Cr-Commit-Position: refs/heads/master@{#440268} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
