[WebCrypto] Cleanup in LoadTestKeys() in ecdh unit tests

[WebCrypto] Cleanup in LoadTestKeys() in ecdh unit tests

Introduced property in the ecdh.json to track the data to be used for P-521 tests in ecdh_unittest.

In ecdh_unittest.cc, it does not rely on the position of data, but relies on the property value, if the value is true it uses that test for loading test keys.

BUG=443009
R=eroman

Committed: https://crrev.com/1031ecd676e38ba05656aecfb9f8af8903c900fa
Cr-Commit-Position: refs/heads/master@{#309907}

[Habib Virji, 2014-12-22 15:46:37]

Instead of relying on the position, it instead uses the special parameter in
json structure to use for loadTestKey function.

[eroman, 2014-12-22 17:56:07]

Spelling error in the changelist description.

https://codereview.chromium.org/816403002/diff/1/content/child/webcrypto/test...
File content/child/webcrypto/test/ecdh_unittest.cc (right):

https://codereview.chromium.org/816403002/diff/1/content/child/webcrypto/test...
content/child/webcrypto/test/ecdh_unittest.cc:134: const base::DictionaryValue*
test;
This does not handle the case where the property is not found in a predictable
way. Instead of proceeding with "test" as the last value, it should assert
explaining that the required property not found.

https://codereview.chromium.org/816403002/diff/1/content/test/data/webcrypto/...
File content/test/data/webcrypto/ecdh.json (right):

https://codereview.chromium.org/816403002/diff/1/content/test/data/webcrypto/...
content/test/data/webcrypto/ecdh.json:153: "load_p521_test_keys": 1 // Special
variable use by ecdh_unittest.cc::LoadTestKeys
(1) This should be a boolean not an integer.
(2) Call it something more specific, like:
    * valid_p521_keys

Or instead, can ModifyLoadTestKeys to scan through the list for a valid P521 key
(testing that curve is P521 and the expectation is success).

[Habib Virji, 2014-12-23 10:59:05]

https://codereview.chromium.org/816403002/diff/1/content/child/webcrypto/test...
File content/child/webcrypto/test/ecdh_unittest.cc (right):

https://codereview.chromium.org/816403002/diff/1/content/child/webcrypto/test...
content/child/webcrypto/test/ecdh_unittest.cc:134: const base::DictionaryValue*
test;
On 2014/12/22 17:56:07, eroman wrote:
> This does not handle the case where the property is not found in a predictable
> way. Instead of proceeding with "test" as the last value, it should assert
> explaining that the required property not found.

I have added now, assert failure if it fails to find valid_p521_key. Since
LoadTestKeys is a helper function, returning early did not help as it was
failing in the calling test. So added a assert failure as a return type and
doing the assert check where LoadTestKeys is called in individual test. Since it
returns assert had to change assert check to expect check.

https://codereview.chromium.org/816403002/diff/1/content/test/data/webcrypto/...
File content/test/data/webcrypto/ecdh.json (right):

https://codereview.chromium.org/816403002/diff/1/content/test/data/webcrypto/...
content/test/data/webcrypto/ecdh.json:153: "load_p521_test_keys": 1 // Special
variable use by ecdh_unittest.cc::LoadTestKeys
Changed to boolean and change to more specific attribute. 

Only reason why i did not do second suggestion, was there are some error test
too related to P-521 in json file. It would have required me to stop on finding
first p-521, to avoid confusion which p-521 is being used, that's why relying on
existing test.

[eroman, 2014-12-29 18:01:04]

looksgood after addressing these comments.

https://codereview.chromium.org/816403002/diff/20001/content/child/webcrypto/...
File content/child/webcrypto/test/ecdh_unittest.cc (right):

https://codereview.chromium.org/816403002/diff/20001/content/child/webcrypto/...
content/child/webcrypto/test/ecdh_unittest.cc:130: blink::WebCryptoKey*
private_key) {
run "git cl format" to fix these sorts of formatting errors.

(In fact I am surprised it didn't complain on upload, the presubmit tests should
warn about this.)

https://codereview.chromium.org/816403002/diff/20001/content/child/webcrypto/...
content/child/webcrypto/test/ecdh_unittest.cc:132:
EXPECT_TRUE(ReadJsonTestFileToList("ecdh.json", &tests));
Please change to:

if (!ReadJsonTestFileToList(...))
  return ::testing::AssertionFailure() << "Failed loading ecdh.json";

https://codereview.chromium.org/816403002/diff/20001/content/child/webcrypto/...
content/child/webcrypto/test/ecdh_unittest.cc:142: else if (!valid_p521_keys &&
tests->GetSize() == test_index + 1)
use curly braces around this:

else if (...) {
  ...
}

(Curly braces are used when the statement is multi-line.)

[Habib Virji, 2014-12-30 17:22:48]

Thanks updated.

https://codereview.chromium.org/816403002/diff/20001/content/child/webcrypto/...
File content/child/webcrypto/test/ecdh_unittest.cc (right):

https://codereview.chromium.org/816403002/diff/20001/content/child/webcrypto/...
content/child/webcrypto/test/ecdh_unittest.cc:130: blink::WebCryptoKey*
private_key) {
On 2014/12/29 18:01:04, eroman wrote:
> run "git cl format" to fix these sorts of formatting errors.
> 
> (In fact I am surprised it didn't complain on upload, the presubmit tests
should
> warn about this.)

It do not remember, but will keep an eye on running git cl format before other
submissions.

https://codereview.chromium.org/816403002/diff/20001/content/child/webcrypto/...
content/child/webcrypto/test/ecdh_unittest.cc:132:
EXPECT_TRUE(ReadJsonTestFileToList("ecdh.json", &tests));
On 2014/12/29 18:01:04, eroman wrote:
> Please change to:
> 
> if (!ReadJsonTestFileToList(...))
>   return ::testing::AssertionFailure() << "Failed loading ecdh.json";

Done.

https://codereview.chromium.org/816403002/diff/20001/content/child/webcrypto/...
content/child/webcrypto/test/ecdh_unittest.cc:142: else if (!valid_p521_keys &&
tests->GetSize() == test_index + 1)
On 2014/12/29 18:01:04, eroman wrote:
> use curly braces around this:
> 
> else if (...) {
>   ...
> }
> 
> (Curly braces are used when the statement is multi-line.)

Done.

[eroman, 2014-12-30 17:41:48]

https://codereview.chromium.org/816403002/diff/40001/content/child/webcrypto/...
File content/child/webcrypto/test/ecdh_unittest.cc (right):

https://codereview.chromium.org/816403002/diff/40001/content/child/webcrypto/...
content/child/webcrypto/test/ecdh_unittest.cc:143: else if (!valid_p521_keys &&
tests->GetSize() == test_index + 1) {
This is not a good way to do the test. (For instance it will not return a
failure if tests->GetSize() == 0)

Instead move this outside of the "for" loop:

if (!valid_p521_keys) {
  return ::testing::AssertionFailure() << ....;
}

[Habib Virji, 2014-12-31 09:38:24]

https://codereview.chromium.org/816403002/diff/40001/content/child/webcrypto/...
File content/child/webcrypto/test/ecdh_unittest.cc (right):

https://codereview.chromium.org/816403002/diff/40001/content/child/webcrypto/...
content/child/webcrypto/test/ecdh_unittest.cc:143: else if (!valid_p521_keys &&
tests->GetSize() == test_index + 1) {
On 2014/12/30 17:41:48, eroman wrote:
> This is not a good way to do the test. (For instance it will not return a
> failure if tests->GetSize() == 0)
> 
> Instead move this outside of the "for" loop:
> 
> if (!valid_p521_keys) {
>   return ::testing::AssertionFailure() << ....;
> }

Done.

[eroman, 2014-12-31 19:46:26]

The CQ bit was checked by eroman@chromium.org

[eroman, 2014-12-31 19:46:27]

lgtm

[commit-bot: I haz the power, 2014-12-31 19:46:47]

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

[commit-bot: I haz the power, 2014-12-31 20:24:44]

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

[commit-bot: I haz the power, 2014-12-31 20:24:45]

Try jobs failed on following builders:
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/10...)

[Habib Virji, 2015-01-05 10:19:26]

The CQ bit was checked by habib.virji@samsung.com

[commit-bot: I haz the power, 2015-01-05 10:20:01]

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

[commit-bot: I haz the power, 2015-01-05 11:18:08]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-01-05 11:19:15]

Patchset 5 (id:??) landed as
https://crrev.com/1031ecd676e38ba05656aecfb9f8af8903c900fa
Cr-Commit-Position: refs/heads/master@{#309907}