Implement should().notBeConstantValueOf(x)

Implement should().notBeConstantValueOf(x)

This CL adds the new assertion to audit; it checks if an array has
values other than the expected value. This is useful when checking
the non-zero array.

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

Review-Url: https://codereview.chromium.org/2618053002
Cr-Commit-Position: refs/heads/master@{#442095}
Committed: https://chromium.googlesource.com/chromium/src/+/48de3222c0b6a9cf23aa2e18184ef72cf736023a

[hongchan, 2017-01-06 18:34:00]

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

[hongchan, 2017-01-06 18:34:01]

PTAL.

[Raymond Toy, 2017-01-06 18:50:12]

https://codereview.chromium.org/2618053002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/webaudio/resources/audit.js (right):

https://codereview.chromium.org/2618053002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/webaudio/resources/audit.js:551: *   "PASS  
[1,0,1] contains values that differ from the constant 1."
This doesn't seem to match what the code is producing.  Make it match.

https://codereview.chromium.org/2618053002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/webaudio/resources/audit.js:559: let passDetail,
failDetail;
Should this be two separate lets?  Couldn't find anything in the Javascript
style guide, but the C++ style says separate declarations.

https://codereview.chromium.org/2618053002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/webaudio/resources/audit.js:560: let differences
= {};
Would it be better if differences were an (empty) array and you just pushed the
index onto it (in line 564 below)?

https://codereview.chromium.org/2618053002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/webaudio/resources/audit.js:579: failDetail =
'${actual} contains only the constant ${expected}.';
It might be better, maybe, to say "${actual} should have contain at least one
value different from ${expected}".  When reading the output, we know it fails,
but it's not so easy to determine why from this sentence.  I think the
rephrasing makes it clear that the actual result should not be a constant. 
(Feel free to phrase my suggestion differently to make it clearer.)

https://codereview.chromium.org/2618053002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/webaudio/unit-tests/audit-expected.txt
(right):

https://codereview.chromium.org/2618053002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/webaudio/unit-tests/audit-expected.txt:7: PASS  
[1,0,1] contains 1 value that differs from the constant 1.
While the output shows that it is true, the text doesn't really say why this is
passing.  You could even say "[1,0,1] is not constantly 1" or something to
better reflect what is expected.

[hongchan, 2017-01-06 19:14:42]

https://codereview.chromium.org/2618053002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/webaudio/resources/audit.js (right):

https://codereview.chromium.org/2618053002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/webaudio/resources/audit.js:551: *   "PASS  
[1,0,1] contains values that differ from the constant 1."
On 2017/01/06 18:50:12, Raymond Toy wrote:
> This doesn't seem to match what the code is producing.  Make it match.

Done.

https://codereview.chromium.org/2618053002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/webaudio/resources/audit.js:559: let passDetail,
failDetail;
On 2017/01/06 18:50:12, Raymond Toy wrote:
> Should this be two separate lets?  Couldn't find anything in the Javascript
> style guide, but the C++ style says separate declarations.

You are correct. Should I fix this instance only?

https://codereview.chromium.org/2618053002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/webaudio/resources/audit.js:560: let differences
= {};
On 2017/01/06 18:50:12, Raymond Toy wrote:
> Would it be better if differences were an (empty) array and you just pushed
the
> index onto it (in line 564 below)?

Not sure. I thought you might want to print out the index and the value that
differ from the expected value. With an array, we will lose the record of actual
values. I am open, but not sure if the change gives us any benefit.

https://codereview.chromium.org/2618053002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/webaudio/resources/audit.js:579: failDetail =
'${actual} contains only the constant ${expected}.';
On 2017/01/06 18:50:12, Raymond Toy wrote:
> It might be better, maybe, to say "${actual} should have contain at least one
> value different from ${expected}".  When reading the output, we know it fails,
> but it's not so easy to determine why from this sentence.  I think the
> rephrasing makes it clear that the actual result should not be a constant. 
> (Feel free to phrase my suggestion differently to make it clearer.)

Thanks, your phrasing is much better. Done.

https://codereview.chromium.org/2618053002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/webaudio/unit-tests/audit-expected.txt
(right):

https://codereview.chromium.org/2618053002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/webaudio/unit-tests/audit-expected.txt:7: PASS  
[1,0,1] contains 1 value that differs from the constant 1.
On 2017/01/06 18:50:12, Raymond Toy wrote:
> While the output shows that it is true, the text doesn't really say why this
is
> passing.  You could even say "[1,0,1] is not constantly 1" or something to
> better reflect what is expected.

I am adding the number of differences as well. Done.

[Raymond Toy, 2017-01-06 22:03:43]

lgtm with nit

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

https://codereview.chromium.org/2618053002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/webaudio/resources/audit.js:574: noun:
numberOfDifferences > 1 ? 'values' : 'value',
Nit: Since you have "values" and "differs", you probably want to add a test to
show that.

[hongchan, 2017-01-06 22:08:45]

The CQ bit was checked by hongchan@chromium.org

[hongchan, 2017-01-06 22:08:45]

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/2618053002/#ps40001
(title: "Addressing nit after l-g-t-m")

[hongchan, 2017-01-06 22:08:55]

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

https://codereview.chromium.org/2618053002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/webaudio/resources/audit.js:574: noun:
numberOfDifferences > 1 ? 'values' : 'value',
On 2017/01/06 22:03:43, Raymond Toy wrote:
> Nit: Since you have "values" and "differs", you probably want to add a test to
> show that.

Done. Also removed the 'differs' part since it's not needed any more.

[commit-bot: I haz the power, 2017-01-06 22:09:08]

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, 2017-01-06 23:30:15]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1483740525022380,
"parent_rev": "a439a069a35766fd06c5c91b72cd22f42d2787bc", "commit_rev":
"48de3222c0b6a9cf23aa2e18184ef72cf736023a"}

[commit-bot: I haz the power, 2017-01-06 23:30:44]

Description was changed from

==========
Implement should().notBeConstantValueOf(x)

This CL adds the new assertion to audit; it checks if an array has
values other than the expected value. This is useful when checking
the non-zero array.

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

to

==========
Implement should().notBeConstantValueOf(x)

This CL adds the new assertion to audit; it checks if an array has
values other than the expected value. This is useful when checking
the non-zero array.

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

Review-Url: https://codereview.chromium.org/2618053002
Cr-Commit-Position: refs/heads/master@{#442095}
Committed:
https://chromium.googlesource.com/chromium/src/+/48de3222c0b6a9cf23aa2e18184e...
==========

[commit-bot: I haz the power, 2017-01-06 23:30:45]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/48de3222c0b6a9cf23aa2e18184e...