|
|
Created:
7 years, 3 months ago by eroman Modified:
7 years, 2 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRefactor webcrypto unittests.
Extracts some helper functions which will be re-used for AES-CBC tests. Notably, importing of secret keys and comparison of arraybuffer against hex string.
BUG=245025
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=225365
Patch Set 1 #
Total comments: 3
Patch Set 2 : switch back to ARRAYSIZE_UNSAFE #
Total comments: 2
Patch Set 3 : Remove multiple inheritance and "using namespace WebKit" #
Total comments: 12
Patch Set 4 : address sleevi comment #
Total comments: 4
Patch Set 5 : Use forwarding #
Total comments: 1
Patch Set 6 : remove unneeded header #Patch Set 7 : remove comment #Patch Set 8 : moar ASSERT #Patch Set 9 : fix forwarder #
Total comments: 1
Patch Set 10 : change to preincrement #Patch Set 11 : Rebase onto master #
Messages
Total messages: 17 (0 generated)
(Split this out from my upcoming AES-CBC changelist for simplicity) https://codereview.chromium.org/24237013/diff/1/content/renderer/webcrypto_im... File content/renderer/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/24237013/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl_unittest.cc:19: using namespace WebKit; Not sure if this is OK; the style guide says no "using namespace", but not sure if that applies to test files... https://codereview.chromium.org/24237013/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl_unittest.cc:25: WebCryptoKey ImportSecretKeyFromRawHexString( Extracted from HMACSampleSets() https://codereview.chromium.org/24237013/diff/1/content/renderer/webcrypto_im... content/renderer/webcrypto_impl_unittest.cc:87: struct TestCase { Changed the format a bit: * flattened the test expectations so algorithm is explicit rather than array index. * Made the input a hex string so input_length isn't needed
lgtm https://codereview.chromium.org/24237013/diff/9001/content/renderer/webcrypto... File content/renderer/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/24237013/diff/9001/content/renderer/webcrypto... content/renderer/webcrypto_impl_unittest.cc:43: EXPECT_TRUE(handle.get()); Does the WebCryptoKey interface handle a null value passed to it? If not, this should be assert.
https://codereview.chromium.org/24237013/diff/9001/content/renderer/webcrypto... File content/renderer/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/24237013/diff/9001/content/renderer/webcrypto... content/renderer/webcrypto_impl_unittest.cc:43: EXPECT_TRUE(handle.get()); On 2013/09/23 20:04:51, Bryan Eyler wrote: > Does the WebCryptoKey interface handle a null value passed to it? If not, this > should be assert. I had to make this an EXPECT_ rather than ASSERT_, because of how the ASSERT_ macro is defined. Under the hood, ASSERT_ expands to conceptually: if (failed-assertion) { FAIL(); return; } Since this appears in a helper function with a non-void return type, that doesn't compile.
I have updated this changelist after conversation with rsleevi: * No longer using multiple inheritance to workaround the need for friend_testing. I have left the existing friend_tests in place and added a new one for the test fixture. * I removed the "using namespace WebKit" and added back all of the "WebKit::" prefixes, since this is also disallowed by the style guide.
https://codereview.chromium.org/24237013/diff/15001/content/renderer/webcrypt... File content/renderer/webcrypto_impl.h (right): https://codereview.chromium.org/24237013/diff/15001/content/renderer/webcrypt... content/renderer/webcrypto_impl.h:44: FRIEND_TEST_ALL_PREFIXES(WebCryptoImplTest, HMACSampleSets); nit: these two are no longer needed, are they? https://codereview.chromium.org/24237013/diff/15001/content/renderer/webcrypt... File content/renderer/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/24237013/diff/15001/content/renderer/webcrypt... content/renderer/webcrypto_impl_unittest.cc:22: protected: why protected? Visibility rules are meaningless here in the context of a testing::test https://codereview.chromium.org/24237013/diff/15001/content/renderer/webcrypt... content/renderer/webcrypto_impl_unittest.cc:46: /*extracatable=*/false, This is fairly uncommon style in Chromium code to comment like this. Would rather drop it... It's also spelled wrong. https://codereview.chromium.org/24237013/diff/15001/content/renderer/webcrypt... content/renderer/webcrypto_impl_unittest.cc:76: } Seems like these should all be free functions in an unnamed namespace right? That is, there's nothing here that relies on the special protected access. https://codereview.chromium.org/24237013/diff/15001/content/renderer/webcrypt... content/renderer/webcrypto_impl_unittest.cc:78: WebCryptoImpl crypto_; You still have this protected, and it works because of the FRIEND_TEST. That is, I'm not sure it cleans anything up? It seems like this member should be private, and the protected / public forwarders https://codereview.chromium.org/24237013/diff/15001/content/renderer/webcrypt... content/renderer/webcrypto_impl_unittest.cc:259: test.key, algorithm, WebKit::WebCryptoKeyUsageSign); No Verify tests yet?
https://codereview.chromium.org/24237013/diff/15001/content/renderer/webcrypt... File content/renderer/webcrypto_impl.h (right): https://codereview.chromium.org/24237013/diff/15001/content/renderer/webcrypt... content/renderer/webcrypto_impl.h:44: FRIEND_TEST_ALL_PREFIXES(WebCryptoImplTest, HMACSampleSets); On 2013/09/24 01:17:11, Ryan Sleevi wrote: > nit: these two are no longer needed, are they? They are still needed: DigestSampleSets: calls DigestInternal HMACSampleSets: calls SignInternal I considered adding forwarding methods in the fixture base class, but I couldn't stomach it so left the friend tests. https://codereview.chromium.org/24237013/diff/15001/content/renderer/webcrypt... File content/renderer/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/24237013/diff/15001/content/renderer/webcrypt... content/renderer/webcrypto_impl_unittest.cc:22: protected: On 2013/09/24 01:17:11, Ryan Sleevi wrote: > why protected? Visibility rules are meaningless here in the context of a > testing::test > These are helper methods used by the test classes. https://codereview.chromium.org/24237013/diff/15001/content/renderer/webcrypt... content/renderer/webcrypto_impl_unittest.cc:46: /*extracatable=*/false, On 2013/09/24 01:17:11, Ryan Sleevi wrote: > This is fairly uncommon style in Chromium code to comment like this. Would > rather drop it... > > It's also spelled wrong. Removed. https://codereview.chromium.org/24237013/diff/15001/content/renderer/webcrypt... content/renderer/webcrypto_impl_unittest.cc:76: } On 2013/09/24 01:17:11, Ryan Sleevi wrote: > Seems like these should all be free functions in an unnamed namespace right? > > That is, there's nothing here that relies on the special protected access. Done. https://codereview.chromium.org/24237013/diff/15001/content/renderer/webcrypt... content/renderer/webcrypto_impl_unittest.cc:78: WebCryptoImpl crypto_; On 2013/09/24 01:17:11, Ryan Sleevi wrote: > You still have this protected, and it works because of the FRIEND_TEST. > > That is, I'm not sure it cleans anything up? > > It seems like this member should be private, and the protected / public > forwarders It is protected so that the individual tests can access it and use the same instance of WebCryptoImpl. The test fixture is a friend so that ImportSecretKeyFromRawHexString() can call ImportKeyInternal. I could remove this member and make it an input to ImportSecretKeyFromRawHexString(), however since every test must intantiate a WebCryptoImpl, why not extract the commonality? https://codereview.chromium.org/24237013/diff/15001/content/renderer/webcrypt... content/renderer/webcrypto_impl_unittest.cc:259: test.key, algorithm, WebKit::WebCryptoKeyUsageSign); On 2013/09/24 01:17:11, Ryan Sleevi wrote: > No Verify tests yet? Correct. I believe Bryan is working on verify() next.
rsleevi: I added forwarding implementations in the test fixture per our discussion. Let me know if you prefer this approach to friend testing.
https://codereview.chromium.org/24237013/diff/2/content/renderer/webcrypto_im... File content/renderer/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/24237013/diff/2/content/renderer/webcrypto_im... content/renderer/webcrypto_impl_unittest.cc:32: array_buffer.data(), array_buffer.byteLength()).c_str()); if the operation fails, and |array_buffer| is not initialized to a size, will base::HexEncode handle it properly? Is it valid to call array_buffer.data() on a WebArrayBuffer with 0 byteLength()? https://codereview.chromium.org/24237013/diff/2/content/renderer/webcrypto_im... content/renderer/webcrypto_impl_unittest.cc:70: EXPECT_TRUE(handle.get()); This should be ASSERT_TRUE, correct? Things will crash if this fails https://codereview.chromium.org/24237013/diff/22001/content/renderer/webcrypt... File content/renderer/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/24237013/diff/22001/content/renderer/webcrypt... content/renderer/webcrypto_impl_unittest.cc:84: // access to the methods more simply, but is forbidden by style guide. *cough* unnecessary comment? *cough*
https://codereview.chromium.org/24237013/diff/2/content/renderer/webcrypto_im... File content/renderer/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/24237013/diff/2/content/renderer/webcrypto_im... content/renderer/webcrypto_impl_unittest.cc:70: EXPECT_TRUE(handle.get()); On 2013/09/24 01:47:13, Ryan Sleevi wrote: > This should be ASSERT_TRUE, correct? Things will crash if this fails See the response I gave to Bryan on this issue.
Anything else you want addressed? https://codereview.chromium.org/24237013/diff/2/content/renderer/webcrypto_im... File content/renderer/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/24237013/diff/2/content/renderer/webcrypto_im... content/renderer/webcrypto_impl_unittest.cc:32: array_buffer.data(), array_buffer.byteLength()).c_str()); On 2013/09/24 01:47:13, Ryan Sleevi wrote: > if the operation fails, and |array_buffer| is not initialized to a size, will > base::HexEncode handle it properly? Is it valid to call array_buffer.data() on a > WebArrayBuffer with 0 byteLength()? Yes, default WebArrayBuffer will have 0 length, and HexEncode will return an empty string. I also changed some EXPECT_EQ to ASSERT_EQ to prevent this situation from happening.
lgtm https://codereview.chromium.org/24237013/diff/36001/content/renderer/webcrypt... File content/renderer/webcrypto_impl_unittest.cc (right): https://codereview.chromium.org/24237013/diff/36001/content/renderer/webcrypt... content/renderer/webcrypto_impl_unittest.cc:282: test_index++) { nit: prefer pre-increment (more of a style consistency thing, since it has no perf implications here)
jamesr: Please review for content/renderer approval
jamesr or jochen for content/renderer approval.
Stamp lgtm, although please fix "unnittests" -> "unittests" typo in description
Thanks! Fixed the description.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eroman@chromium.org/24237013/44001
Message was sent while issue was closed.
Change committed as 225365 |