Improve LayoutTest output for Errors

Improve LayoutTest output for Errors

Improving LayoutTest output for errors for tests of
bad inputs in crypto algorithm.

BUG=441020
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=187236

[Paritosh Kumar, 2014-12-12 14:34:55]

paritosh.in@samsung.com changed reviewers:
+ eroman@chromium.org, jww@chromium.org

[Paritosh Kumar, 2014-12-12 14:34:56]

Please have a look.

-Paritosh

[eroman, 2014-12-12 19:37:00]

eroman@chromium.org changed reviewers:
- jww@chromium.org

[eroman, 2014-12-12 19:37:02]

(Removing jww as reviewer, my review will be sufficient for this.)

Thanks, this is a good improvement!

I have a couple wording nits on the messages, but otherwise looks good.

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/aes-cbc-g...
File LayoutTests/crypto/aes-cbc-generateKey-parseAlgorithm-failures.html
(right):

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/aes-cbc-g...
LayoutTests/crypto/aes-cbc-generateKey-parseAlgorithm-failures.html:19: //
Invalid keyUsages
Can you remove the comments? (They add no value now that there is a debug
message with even more detail).

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/aes-cbc-g...
LayoutTests/crypto/aes-cbc-generateKey-parseAlgorithm-failures.html:32:
debug("\ngenerateKey() with bad keyUsages value that is a string...");
"generateKey() with an invalid key usage of 'boo'"

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/aes-cbc-g...
LayoutTests/crypto/aes-cbc-generateKey-parseAlgorithm-failures.html:42:
debug("\ngenerateKey() with length larger than range...");
"generateKey() with a length of 70000"

I think there is value in specifying the exact numeric value, since part of
looking at text expectations it to verify that the range enforcement is correct.

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/aes-cbc-g...
LayoutTests/crypto/aes-cbc-generateKey-parseAlgorithm-failures.html:47:
debug("\ngenerateKey() with negative length...");
generateKey() with a length of -3

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/aes-cbc-p...
File LayoutTests/crypto/aes-cbc-parseAlgorithm-failures.html (right):

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/aes-cbc-p...
LayoutTests/crypto/aes-cbc-parseAlgorithm-failures.html:49: debug('\nencrypt()
with iv that is of 0 byte...');
"encrypt() with an iv that is empty"
or
"encrypt() with an iv of length 0.
or
"encrypt() with an iv containing 0 bytes"

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/aes-cbc-p...
LayoutTests/crypto/aes-cbc-parseAlgorithm-failures.html:55: debug('\nencrypt()
with iv that is of 17 byte...');
encrypt() with an iv containing 17 bytes

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/aes-ctr-g...
File LayoutTests/crypto/aes-ctr-generateKey-parseAlgorithm-failures.html
(right):

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/aes-ctr-g...
LayoutTests/crypto/aes-ctr-generateKey-parseAlgorithm-failures.html:24:
debug("\ngenerateKey() with length larger than an octet...");
Please mention the exact length used (70000)
(since part of the verification is ensuring that is in fact outside of octet
range)

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/aes-ctr-g...
LayoutTests/crypto/aes-ctr-generateKey-parseAlgorithm-failures.html:29:
debug("\ngenerateKey() with negative length...");
mention exact length

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/aes-ctr-p...
File LayoutTests/crypto/aes-ctr-parseAlgorithm-failures.html (right):

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/aes-ctr-p...
LayoutTests/crypto/aes-ctr-parseAlgorithm-failures.html:46: debug("\nencrypt()
with length larger than an octet...");
Please mention the length used (256)

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/aes-ctr-p...
LayoutTests/crypto/aes-ctr-parseAlgorithm-failures.html:51: debug("\nencrypt()
with negative length...");
Please mention the length used (-3)

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/aes-kw-ge...
File LayoutTests/crypto/aes-kw-generateKey-parseAlgorithm-failures.html (right):

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/aes-kw-ge...
LayoutTests/crypto/aes-kw-generateKey-parseAlgorithm-failures.html:24:
debug("\ngenerateKey() with length larger than an octet...");
Same comment as earlier

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/aes-kw-ge...
LayoutTests/crypto/aes-kw-generateKey-parseAlgorithm-failures.html:29:
debug("\ngenerateKey() with negative length...");
Same comment as earlier

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/digest-fa...
File LayoutTests/crypto/digest-failures.html (right):

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/digest-fa...
LayoutTests/crypto/digest-failures.html:17: // Called with too few parameters.
Throughout this change, please remove these sort of comments (which are now
described more accurately by the debug)

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/ecdh-deri...
File LayoutTests/crypto/ecdh-deriveBits-failures.html (right):

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/ecdh-deri...
LayoutTests/crypto/ecdh-deriveBits-failures.html:54: debug("\nderiveBits() with
algorithm name that is null...");
"with algorithm that is null"

(the name would be part of the algorithm dictionary)

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/ecdh-deri...
LayoutTests/crypto/ecdh-deriveBits-failures.html:60: debug("\nderiveBits() with
algorithm name that is -1...");
replace "algorithm name" --> "algorithm"

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/ecdh-deri...
LayoutTests/crypto/ecdh-deriveBits-failures.html:90: debug("\nderiveBits() with
private key as public key...");
This is a bit hard to read. How about:

deriveBits() with a private key as the "public" parameter

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/hmac-gene...
File LayoutTests/crypto/hmac-generateKey-parseAlgorithm-failures.html (right):

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/hmac-gene...
LayoutTests/crypto/hmac-generateKey-parseAlgorithm-failures.html:19:
debug("\ngenerateKey() with negative length...");
please mention exact length used

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/hmac-gene...
LayoutTests/crypto/hmac-generateKey-parseAlgorithm-failures.html:29:
debug("\ngenerateKey() with length larger than an octet...");
please mention exact length used

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/rsa-ssa-g...
File LayoutTests/crypto/rsa-ssa-generateKey-parseAlgorithm-failures.html
(right):

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/rsa-ssa-g...
LayoutTests/crypto/rsa-ssa-generateKey-parseAlgorithm-failures.html:19:
debug("\ngenerateKey() with negative modulusLength...");
Same comment as earlier (mention exact length)

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/rsa-ssa-g...
LayoutTests/crypto/rsa-ssa-generateKey-parseAlgorithm-failures.html:54:
debug("\ngenerateKey() with publicExponent that is of 0 bytes...");
"that is 0 bytes long"

[jww, 2014-12-12 19:55:24]

jww@chromium.org changed reviewers:
+ jww@chromium.org

[jww, 2014-12-12 19:55:24]

I would just add in these two nits, though...

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/aes-cbc-p...
File LayoutTests/crypto/aes-cbc-parseAlgorithm-failures.html (right):

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/aes-cbc-p...
LayoutTests/crypto/aes-cbc-parseAlgorithm-failures.html:49: debug('\nencrypt()
with iv that is of 0 byte...');
nit: "0 byte" -> "0 bytes"

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/aes-cbc-p...
LayoutTests/crypto/aes-cbc-parseAlgorithm-failures.html:55: debug('\nencrypt()
with iv that is of 17 byte...');
nit: "17 byte" -> "17 bytes"

[Paritosh Kumar, 2014-12-15 08:21:27]

Thanks Eric and jww

Updated as per your suggestions.

Please have a look.

-Paritosh

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/aes-cbc-g...
File LayoutTests/crypto/aes-cbc-generateKey-parseAlgorithm-failures.html
(right):

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/aes-cbc-g...
LayoutTests/crypto/aes-cbc-generateKey-parseAlgorithm-failures.html:19: //
Invalid keyUsages
On 2014/12/12 19:37:01, eroman wrote:
> Can you remove the comments? (They add no value now that there is a debug
> message with even more detail).

Done.

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/aes-cbc-g...
LayoutTests/crypto/aes-cbc-generateKey-parseAlgorithm-failures.html:32:
debug("\ngenerateKey() with bad keyUsages value that is a string...");
On 2014/12/12 19:37:01, eroman wrote:
> "generateKey() with an invalid key usage of 'boo'"

Done.

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/aes-cbc-g...
LayoutTests/crypto/aes-cbc-generateKey-parseAlgorithm-failures.html:42:
debug("\ngenerateKey() with length larger than range...");
On 2014/12/12 19:37:01, eroman wrote:
> "generateKey() with a length of 70000"
> 
> I think there is value in specifying the exact numeric value, since part of
> looking at text expectations it to verify that the range enforcement is
correct.

Done.

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/aes-cbc-g...
LayoutTests/crypto/aes-cbc-generateKey-parseAlgorithm-failures.html:47:
debug("\ngenerateKey() with negative length...");
On 2014/12/12 19:37:01, eroman wrote:
> generateKey() with a length of -3

Done.

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/aes-cbc-p...
File LayoutTests/crypto/aes-cbc-parseAlgorithm-failures.html (right):

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/aes-cbc-p...
LayoutTests/crypto/aes-cbc-parseAlgorithm-failures.html:49: debug('\nencrypt()
with iv that is of 0 byte...');
On 2014/12/12 19:37:01, eroman wrote:
> "encrypt() with an iv that is empty"
> or
> "encrypt() with an iv of length 0.
> or
> "encrypt() with an iv containing 0 bytes"

Done.

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/aes-cbc-p...
LayoutTests/crypto/aes-cbc-parseAlgorithm-failures.html:49: debug('\nencrypt()
with iv that is of 0 byte...');
On 2014/12/12 19:55:24, jww wrote:
> nit: "0 byte" -> "0 bytes"

Done.

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/aes-cbc-p...
LayoutTests/crypto/aes-cbc-parseAlgorithm-failures.html:55: debug('\nencrypt()
with iv that is of 17 byte...');
On 2014/12/12 19:37:01, eroman wrote:
> encrypt() with an iv containing 17 bytes

Done.

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/aes-cbc-p...
LayoutTests/crypto/aes-cbc-parseAlgorithm-failures.html:55: debug('\nencrypt()
with iv that is of 17 byte...');
On 2014/12/12 19:55:24, jww wrote:
> nit: "17 byte" -> "17 bytes"

Done.

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/aes-ctr-g...
File LayoutTests/crypto/aes-ctr-generateKey-parseAlgorithm-failures.html
(right):

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/aes-ctr-g...
LayoutTests/crypto/aes-ctr-generateKey-parseAlgorithm-failures.html:24:
debug("\ngenerateKey() with length larger than an octet...");
On 2014/12/12 19:37:01, eroman wrote:
> Please mention the exact length used (70000)
> (since part of the verification is ensuring that is in fact outside of octet
> range)

Done.

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/aes-ctr-g...
LayoutTests/crypto/aes-ctr-generateKey-parseAlgorithm-failures.html:29:
debug("\ngenerateKey() with negative length...");
On 2014/12/12 19:37:01, eroman wrote:
> mention exact length

Done.

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/aes-ctr-p...
File LayoutTests/crypto/aes-ctr-parseAlgorithm-failures.html (right):

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/aes-ctr-p...
LayoutTests/crypto/aes-ctr-parseAlgorithm-failures.html:46: debug("\nencrypt()
with length larger than an octet...");
On 2014/12/12 19:37:01, eroman wrote:
> Please mention the length used (256)

Done.

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/aes-ctr-p...
LayoutTests/crypto/aes-ctr-parseAlgorithm-failures.html:51: debug("\nencrypt()
with negative length...");
On 2014/12/12 19:37:01, eroman wrote:
> Please mention the length used (-3)

Done.

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/aes-kw-ge...
File LayoutTests/crypto/aes-kw-generateKey-parseAlgorithm-failures.html (right):

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/aes-kw-ge...
LayoutTests/crypto/aes-kw-generateKey-parseAlgorithm-failures.html:24:
debug("\ngenerateKey() with length larger than an octet...");
On 2014/12/12 19:37:02, eroman wrote:
> Same comment as earlier

Done.

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/aes-kw-ge...
LayoutTests/crypto/aes-kw-generateKey-parseAlgorithm-failures.html:29:
debug("\ngenerateKey() with negative length...");
On 2014/12/12 19:37:01, eroman wrote:
> Same comment as earlier

Done.

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/digest-fa...
File LayoutTests/crypto/digest-failures.html (right):

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/digest-fa...
LayoutTests/crypto/digest-failures.html:17: // Called with too few parameters.
On 2014/12/12 19:37:02, eroman wrote:
> Throughout this change, please remove these sort of comments (which are now
> described more accurately by the debug)

Done.

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/ecdh-deri...
File LayoutTests/crypto/ecdh-deriveBits-failures.html (right):

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/ecdh-deri...
LayoutTests/crypto/ecdh-deriveBits-failures.html:54: debug("\nderiveBits() with
algorithm name that is null...");
On 2014/12/12 19:37:02, eroman wrote:
> "with algorithm that is null"
> 
> (the name would be part of the algorithm dictionary)

Done.

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/ecdh-deri...
LayoutTests/crypto/ecdh-deriveBits-failures.html:60: debug("\nderiveBits() with
algorithm name that is -1...");
On 2014/12/12 19:37:02, eroman wrote:
> replace "algorithm name" --> "algorithm"

Done.

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/hmac-gene...
File LayoutTests/crypto/hmac-generateKey-parseAlgorithm-failures.html (right):

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/hmac-gene...
LayoutTests/crypto/hmac-generateKey-parseAlgorithm-failures.html:19:
debug("\ngenerateKey() with negative length...");
On 2014/12/12 19:37:02, eroman wrote:
> please mention exact length used

Done.

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/hmac-gene...
LayoutTests/crypto/hmac-generateKey-parseAlgorithm-failures.html:29:
debug("\ngenerateKey() with length larger than an octet...");
On 2014/12/12 19:37:02, eroman wrote:
> please mention exact length used

Done.

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/rsa-ssa-g...
File LayoutTests/crypto/rsa-ssa-generateKey-parseAlgorithm-failures.html
(right):

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/rsa-ssa-g...
LayoutTests/crypto/rsa-ssa-generateKey-parseAlgorithm-failures.html:19:
debug("\ngenerateKey() with negative modulusLength...");
On 2014/12/12 19:37:02, eroman wrote:
> Same comment as earlier (mention exact length)

Done.

https://codereview.chromium.org/801793003/diff/1/LayoutTests/crypto/rsa-ssa-g...
LayoutTests/crypto/rsa-ssa-generateKey-parseAlgorithm-failures.html:54:
debug("\ngenerateKey() with publicExponent that is of 0 bytes...");
On 2014/12/12 19:37:02, eroman wrote:
> "that is 0 bytes long"

Done.

[eroman, 2014-12-15 21:46:18]

patchset 2 LGTM

[eroman, 2014-12-15 21:46:24]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2014-12-15 21:47:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/801793003/20001

[eroman, 2014-12-15 22:32:21]

The CQ bit was unchecked by eroman@chromium.org

[eroman, 2014-12-15 22:35:38]

https://codereview.chromium.org/801793003/diff/20001/LayoutTests/crypto/ecdh-...
File LayoutTests/crypto/ecdh-deriveBits-failures-expected.txt (right):

https://codereview.chromium.org/801793003/diff/20001/LayoutTests/crypto/ecdh-...
LayoutTests/crypto/ecdh-deriveBits-failures-expected.txt:30: deriveBits() with
private key as public key...
It doesn't look like this expectation has been updated to reflect the changes
you made in the accompanying HTML.

Once this is fixed, LGTM.

If you are building on Linux the steps are somewhat annoying:

  (1) Build the BoringSSL version of WebCrypto:

    # Re-generate the ninja files (i.e. makefiles)
    $ GYP_DEFINES="use_openssl=1" ./build/gyp_chromium
    $ ninja -C out/Debug blink_tests

  (2) Temporarily edit the LayoutTests/TestExpectation file by deleting all of
the crypto/ lines (otherwise failures on Linux will be accepted as working)

  (3) Regenerate baselines with:

     ./Tool/Script/run-webkit-tests --debug --reset-results crypto

Cheers

[Paritosh Kumar, 2014-12-16 07:23:54]

The CQ bit was checked by paritosh.in@samsung.com

[Paritosh Kumar, 2014-12-16 07:24:22]

Thanks

Done.

https://codereview.chromium.org/801793003/diff/20001/LayoutTests/crypto/ecdh-...
File LayoutTests/crypto/ecdh-deriveBits-failures-expected.txt (right):

https://codereview.chromium.org/801793003/diff/20001/LayoutTests/crypto/ecdh-...
LayoutTests/crypto/ecdh-deriveBits-failures-expected.txt:30: deriveBits() with
private key as public key...
On 2014/12/15 22:35:38, eroman wrote:
> It doesn't look like this expectation has been updated to reflect the changes
> you made in the accompanying HTML.
> 
> Once this is fixed, LGTM.
> 
> If you are building on Linux the steps are somewhat annoying:
> 
>   (1) Build the BoringSSL version of WebCrypto:
> 
>     # Re-generate the ninja files (i.e. makefiles)
>     $ GYP_DEFINES="use_openssl=1" ./build/gyp_chromium
>     $ ninja -C out/Debug blink_tests
> 
>   (2) Temporarily edit the LayoutTests/TestExpectation file by deleting all of
> the crypto/ lines (otherwise failures on Linux will be accepted as working)
> 
>   (3) Regenerate baselines with:
> 
>      ./Tool/Script/run-webkit-tests --debug --reset-results crypto
> 
> Cheers
Apologize.

Thanks. Done.

[commit-bot: I haz the power, 2014-12-16 07:24:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/801793003/40001

[eroman, 2014-12-16 07:52:18]

patchset 3 LGTM

[commit-bot: I haz the power, 2014-12-16 08:46:42]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=187236