Additional unit test for audit.js: failure cases and logging examples.

Additional unit test for audit.js: failure cases and logging examples.

The unit test for audit.js only covers basic/successful cases.
This CL is to add another unit test that tests failure cases with the
expected text result.

In the course of refining the failure output message, some editorial
edits also have been made in audit.js.

BUG=672988
TEST=LayoutTests/webaudio/unit-tests/audit-failures.html

Committed: https://crrev.com/e373ad9f97f06bf8e44ead4ad51c0500bb98afee
Cr-Commit-Position: refs/heads/master@{#439681}

[hongchan, 2016-12-09 21:30:41]

hongchan@chromium.org changed reviewers:
+ rtoy@chromium.org

[hongchan, 2016-12-09 21:30:42]

PTAL.

[hongchan, 2016-12-09 21:30:56]

The CQ bit was checked by hongchan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-09 21:32:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Raymond Toy, 2016-12-09 22:10:26]

While you're at it, can you add the expected file for the tests that pass in
unit-test/audit.html?  This will help ensure that the pass messages make sense
too.

https://codereview.chromium.org/2568573002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/webaudio/resources/audit.js (right):

https://codereview.chromium.org/2568573002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/webaudio/resources/audit.js:305: failDetail =
'${actual} *INCORRECTLY* threw ' + error.name + ': "'
Do these really need to be highlighted?  The "FAIL" at the beginning of the line
is enough of a hint that something failed. (Plus the gross assert_foo failed at
the end is a pretty big indicator of failure.)

https://codereview.chromium.org/2568573002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/webaudio/unit-tests/audit-failures-expected.txt
(right):

https://codereview.chromium.org/2568573002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/webaudio/unit-tests/audit-failures-expected.txt:16:
FAIL X undefined resolved *INCORRECTLY*. assert_true: expected true got false
This test needs a better description because "undefined" has no meaning in the
context of this message.  How does "undefined" resolve?

https://codereview.chromium.org/2568573002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/webaudio/unit-tests/audit-failures-expected.txt:19:
FAIL X 0 is *NOT* 0.1 within an error of 0: 0 with error of 0. assert_true:
expected true got false
This message looks wrong: "0 with error of 0".  It ought to be rephrased in some
way.

https://codereview.chromium.org/2568573002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/webaudio/unit-tests/audit-failures-expected.txt:20:
FAIL X [1,8,11,18,6,5,5,5,123,49] contains 7 values that are *NOT* equal to 5:
This error message isn't the best because it's hard to tell that you're really
expecting 5 for all values.  Can we rephrase that?

https://codereview.chromium.org/2568573002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/webaudio/unit-tests/audit-failures-expected.txt:27:
FAIL X [1,8,11,18,6,5,5,5,123,49] contains 3 values that are *NOT* equal to
[1,8,10,18,6,5,6,5,123,48]:
This is badly phrased as well.  Perhaps it's better to say:

[...] expected to be equal to [...] but differs in 3 places:

https://codereview.chromium.org/2568573002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/webaudio/unit-tests/audit-failures-expected.txt:32:
FAIL X [1,1,1,1,2,2,3,3,3] contains an *UNEXPECTED* value of 3 at index 6.
assert_true: expected true got false
This could be better phrased too because you can't tell what the actual output
was supposed to be. Maybe:

[...] expected to be [1..., 3..., 2...] but got 3 at index 6.

And shouldn't the error message say that 2 was unexpected at index 4?

https://codereview.chromium.org/2568573002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/webaudio/unit-tests/audit-failures.html
(right):

https://codereview.chromium.org/2568573002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/webaudio/unit-tests/audit-failures.html:29:
should(typeof 3.141592).beEqualTo('string');
Add description: "typeof 3.14"

https://codereview.chromium.org/2568573002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/webaudio/unit-tests/audit-failures.html:37:
should(oac.decodeAudioData(), 'Start OAC rendering').beResolved(),
'Start OAC rendering' doesn't actually describe this test.  One because you're
not starting rendering and two because this test is, I think, for testing that a
promise didn't resolve.

https://codereview.chromium.org/2568573002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/webaudio/unit-tests/audit-failures.html:56:
relativeThreshold: 0.1
Add a test using only relativeThreshold and one using only absoluteThreshold.

[commit-bot: I haz the power, 2016-12-09 22:45:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-09 22:45:33]

Dry run: This issue passed the CQ dry run.

[hongchan, 2016-12-19 22:14:06]

Can you take a look at this again?

https://codereview.chromium.org/2568573002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/webaudio/resources/audit.js (right):

https://codereview.chromium.org/2568573002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/webaudio/resources/audit.js:305: failDetail =
'${actual} *INCORRECTLY* threw ' + error.name + ': "'
On 2016/12/09 22:10:26, Raymond Toy wrote:
> Do these really need to be highlighted?  The "FAIL" at the beginning of the
line
> is enough of a hint that something failed. (Plus the gross assert_foo failed
at
> the end is a pretty big indicator of failure.)

Then do we remove all the *HIGHLIGHT*? I felt PASS/FAIL at the end is not really
visible.

https://codereview.chromium.org/2568573002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/webaudio/unit-tests/audit-failures-expected.txt
(right):

https://codereview.chromium.org/2568573002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/webaudio/unit-tests/audit-failures-expected.txt:16:
FAIL X undefined resolved *INCORRECTLY*. assert_true: expected true got false
On 2016/12/09 22:10:26, Raymond Toy wrote:
> This test needs a better description because "undefined" has no meaning in the
> context of this message.  How does "undefined" resolve?

In this case, the author needs to provide the description. I missed it. We
decided not to generate the description automatically so it simply prints the
evaluated value here.

https://codereview.chromium.org/2568573002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/webaudio/unit-tests/audit-failures-expected.txt:19:
FAIL X 0 is *NOT* 0.1 within an error of 0: 0 with error of 0. assert_true:
expected true got false
On 2016/12/09 22:10:26, Raymond Toy wrote:
> This message looks wrong: "0 with error of 0".  It ought to be rephrased in
some
> way.

Can you suggest something? This error message was written by you a while ago.

https://codereview.chromium.org/2568573002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/webaudio/unit-tests/audit-failures-expected.txt:20:
FAIL X [1,8,11,18,6,5,5,5,123,49] contains 7 values that are *NOT* equal to 5:
On 2016/12/09 22:10:26, Raymond Toy wrote:
> This error message isn't the best because it's hard to tell that you're really
> expecting 5 for all values.  Can we rephrase that?

Done.

https://codereview.chromium.org/2568573002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/webaudio/unit-tests/audit-failures-expected.txt:27:
FAIL X [1,8,11,18,6,5,5,5,123,49] contains 3 values that are *NOT* equal to
[1,8,10,18,6,5,6,5,123,48]:
On 2016/12/09 22:10:26, Raymond Toy wrote:
> This is badly phrased as well.  Perhaps it's better to say:
> 
> [...] expected to be equal to [...] but differs in 3 places:

Done.

https://codereview.chromium.org/2568573002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/webaudio/unit-tests/audit-failures-expected.txt:32:
FAIL X [1,1,1,1,2,2,3,3,3] contains an *UNEXPECTED* value of 3 at index 6.
assert_true: expected true got false
On 2016/12/09 22:10:26, Raymond Toy wrote:
> This could be better phrased too because you can't tell what the actual output
> was supposed to be. Maybe:
> 
> [...] expected to be [1..., 3..., 2...] but got 3 at index 6.
> 
> And shouldn't the error message say that 2 was unexpected at index 4?

So we want the first instance of error. Right? I need to think about the
algorithm for that.

https://codereview.chromium.org/2568573002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/webaudio/unit-tests/audit-failures.html
(right):

https://codereview.chromium.org/2568573002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/webaudio/unit-tests/audit-failures.html:29:
should(typeof 3.141592).beEqualTo('string');
On 2016/12/09 22:10:26, Raymond Toy wrote:
> Add description: "typeof 3.14"

Done.

https://codereview.chromium.org/2568573002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/webaudio/unit-tests/audit-failures.html:37:
should(oac.decodeAudioData(), 'Start OAC rendering').beResolved(),
On 2016/12/09 22:10:26, Raymond Toy wrote:
> 'Start OAC rendering' doesn't actually describe this test.  One because you're
> not starting rendering and two because this test is, I think, for testing that
a
> promise didn't resolve.

Sorry this was a typo.

https://codereview.chromium.org/2568573002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/webaudio/unit-tests/audit-failures.html:56:
relativeThreshold: 0.1
On 2016/12/09 22:10:26, Raymond Toy wrote:
> Add a test using only relativeThreshold and one using only absoluteThreshold.

Done.

[hongchan, 2016-12-20 00:15:00]

PTAL at PS6.

Now containValues() prints out the wrong value/index properly.

[Raymond Toy, 2016-12-20 00:18:18]

lgtm

[hongchan, 2016-12-20 01:42:58]

The CQ bit was checked by hongchan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-20 01:43:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-20 02:47:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-20 02:47:09]

Dry run: This issue passed the CQ dry run.

[hongchan, 2016-12-20 02:49:01]

The CQ bit was checked by hongchan@chromium.org

[commit-bot: I haz the power, 2016-12-20 02:49:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-20 02:54:12]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1482202141110610,
"parent_rev": "4ab649d680f13a8c8fbc70406837fafcce946270", "commit_rev":
"d66ec7b9a8a0e1c177e449d87032c8dc9026a62e"}

[commit-bot: I haz the power, 2016-12-20 02:54:43]

Description was changed from

==========
Additional unit test for audit.js: failure cases and logging examples.

The unit test for audit.js only covers basic/successful cases.
This CL is to add another unit test that tests failure cases with the
expected text result.

In the course of refining the failure output message, some editorial
edits also have been made in audit.js.

BUG=672988
TEST=LayoutTests/webaudio/unit-tests/audit-failures.html
==========

to

==========
Additional unit test for audit.js: failure cases and logging examples.

The unit test for audit.js only covers basic/successful cases.
This CL is to add another unit test that tests failure cases with the
expected text result.

In the course of refining the failure output message, some editorial
edits also have been made in audit.js.

BUG=672988
TEST=LayoutTests/webaudio/unit-tests/audit-failures.html

Review-Url: https://codereview.chromium.org/2568573002
==========

[commit-bot: I haz the power, 2016-12-20 02:54:45]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-12-20 02:57:04]

Description was changed from

==========
Additional unit test for audit.js: failure cases and logging examples.

The unit test for audit.js only covers basic/successful cases.
This CL is to add another unit test that tests failure cases with the
expected text result.

In the course of refining the failure output message, some editorial
edits also have been made in audit.js.

BUG=672988
TEST=LayoutTests/webaudio/unit-tests/audit-failures.html

Review-Url: https://codereview.chromium.org/2568573002
==========

to

==========
Additional unit test for audit.js: failure cases and logging examples.

The unit test for audit.js only covers basic/successful cases.
This CL is to add another unit test that tests failure cases with the
expected text result.

In the course of refining the failure output message, some editorial
edits also have been made in audit.js.

BUG=672988
TEST=LayoutTests/webaudio/unit-tests/audit-failures.html

Committed: https://crrev.com/e373ad9f97f06bf8e44ead4ad51c0500bb98afee
Cr-Commit-Position: refs/heads/master@{#439681}
==========

[commit-bot: I haz the power, 2016-12-20 02:57:05]

Patchset 6 (id:??) landed as
https://crrev.com/e373ad9f97f06bf8e44ead4ad51c0500bb98afee
Cr-Commit-Position: refs/heads/master@{#439681}