Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2775053002:
Add unittests for different transport security state generator components. (Closed)
|
Created:
3 years, 9 months ago by martijnc Modified:
3 years, 8 months ago Reviewers:
Ryan Sleevi CC:
cbentzel+watch_chromium.org, chromium-reviews, lgarron, net-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd unittests for different transport security state generator components.
BUG=595493
Review-Url: https://codereview.chromium.org/2775053002
Cr-Commit-Position: refs/heads/master@{#461191}
Committed: https://chromium.googlesource.com/chromium/src/+/b9aca9d06e2948b32e03c1986ec4b47b66ee50dd
Patch Set 1 : update tests. #
Total comments: 14
Patch Set 2 : comments rsleevi. #Patch Set 3 : fix a comment. #
Messages
Total messages: 29 (21 generated)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by martijn@martijnc.be to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
The CQ bit was checked by martijn@martijnc.be to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Add unittests for the transport security state generator. BUG=595493 ========== to ========== Add unittests for different transport security state generator components. BUG=595493 ==========
martijn@martijnc.be changed reviewers: + rsleevi@chromium.org
This is the next (and second to last) CL in a series of changes combined in [1]. This CL adds unittests for individual classes that are part of the transport security state generator. [1] https://codereview.chromium.org/2660793002 https://codereview.chromium.org/2775053002/diff/60001/net/tools/transport_sec... File net/tools/transport_security_state_generator/BUILD.gn (right): https://codereview.chromium.org/2775053002/diff/60001/net/tools/transport_sec... net/tools/transport_security_state_generator/BUILD.gn:39: "bit_writer_unittest.cc", A follow-up CL will add 2 additional files here. https://codereview.chromium.org/2775053002/diff/60001/net/tools/transport_sec... File net/tools/transport_security_state_generator/trie/trie_bit_buffer_unittest.cc (right): https://codereview.chromium.org/2775053002/diff/60001/net/tools/transport_sec... net/tools/transport_security_state_generator/trie/trie_bit_buffer_unittest.cc:34: EXPECT_THAT(writer.bytes(), testing::ElementsAre(0x55, 0x0)); This looks a little weird (padding with an entire \0 byte) but the behavior is copied from the original Go code. Not sure if this was intentional or not but I left the behavior as it was to avoid introducing unintentional behavior changes. In practice this could add an additional byte to the end of the generated blob if the length is an exact multiple of 8. This extra byte would never be accessed if the rest of the structure is correct.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
So, this is all questions, which I think unless they're highlighting something weird (and I just got lucky), this otherwise LGTM :) https://codereview.chromium.org/2775053002/diff/60001/net/tools/transport_sec... File net/tools/transport_security_state_generator/BUILD.gn (right): https://codereview.chromium.org/2775053002/diff/60001/net/tools/transport_sec... net/tools/transport_security_state_generator/BUILD.gn:7: source_set("transport_security_state_generator_sources") { Curious: Why a source_set (and two executable targets) versus a library (and a shared dependency)? (I'm not sure what the 'right' pattern is, hence trying to learn more here on the design decision) https://codereview.chromium.org/2775053002/diff/60001/net/tools/transport_sec... File net/tools/transport_security_state_generator/bit_writer_unittest.cc (right): https://codereview.chromium.org/2775053002/diff/60001/net/tools/transport_sec... net/tools/transport_security_state_generator/bit_writer_unittest.cc:12: A common idiom is to put these tests in an extra (unnamed) namespace, to avoid any ODR violations. If two tests end up sharing the same name, GTest will barf, but if they're left in the same namespace, the compiler/linker will potentially ODR-out one of them. In general, the pattern in //net is to only leave them in the 'parent' namespace if they're forward-declared fixtures. But that's more of a nit, and if that style has changed out from under me, please do let me know and don't feel you have to make that change :) https://codereview.chromium.org/2775053002/diff/60001/net/tools/transport_sec... net/tools/transport_security_state_generator/bit_writer_unittest.cc:58: writer.WriteBits(170, 1); It took a quick consult of the table to find out 170 was 1010101010. I'm not sure if 0xAA would have been clearer (I suppose only if you spend time reversing), but maybe worth a comment here on the binary value :) If only we had C++14 binary literals :) https://codereview.chromium.org/2775053002/diff/60001/net/tools/transport_sec... File net/tools/transport_security_state_generator/huffman/huffman_builder_unittest.cc (right): https://codereview.chromium.org/2775053002/diff/60001/net/tools/transport_sec... net/tools/transport_security_state_generator/huffman/huffman_builder_unittest.cc:18: // Make sure some values have identical counts. Doesn't this make _most_ values have identical counts? Or am I mentally parsing wrong? https://codereview.chromium.org/2775053002/diff/60001/net/tools/transport_sec... net/tools/transport_security_state_generator/huffman/huffman_builder_unittest.cc:54: for (uint8_t i = 0; i <= 63; i++) { Why 63 (vs 127 above)? The i*2 means you'll skip 127 - desired or not? https://codereview.chromium.org/2775053002/diff/60001/net/tools/transport_sec... net/tools/transport_security_state_generator/huffman/huffman_builder_unittest.cc:126: 0x1, 0x2)); Sleevi stupidity - why 0x1/0x2?
The CQ bit was checked by martijn@martijnc.be to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2775053002/diff/60001/net/tools/transport_sec... File net/tools/transport_security_state_generator/BUILD.gn (right): https://codereview.chromium.org/2775053002/diff/60001/net/tools/transport_sec... net/tools/transport_security_state_generator/BUILD.gn:7: source_set("transport_security_state_generator_sources") { On 2017/03/30 at 15:03:32, Ryan Sleevi wrote: > Curious: Why a source_set (and two executable targets) versus a library (and a shared dependency)? > > (I'm not sure what the 'right' pattern is, hence trying to learn more here on the design decision) The GN documentation says a source_set is identical to a shared_library with the exception that is doesn't get linked separately. This should make the build a little faster. The lack of a linking step means that there is no dead code elimination but I don't think that's an issue here. "transport_security_state_generator_sources" is part of both the generator and "transport_security_state_generator_test_sources". "transport_security_state_generator_test_sources" gets linked into net_unittests directly. https://codereview.chromium.org/2775053002/diff/60001/net/tools/transport_sec... File net/tools/transport_security_state_generator/bit_writer_unittest.cc (right): https://codereview.chromium.org/2775053002/diff/60001/net/tools/transport_sec... net/tools/transport_security_state_generator/bit_writer_unittest.cc:12: On 2017/03/30 at 15:03:32, Ryan Sleevi wrote: > A common idiom is to put these tests in an extra (unnamed) namespace, to avoid any ODR violations. If two tests end up sharing the same name, GTest will barf, but if they're left in the same namespace, the compiler/linker will potentially ODR-out one of them. > > In general, the pattern in //net is to only leave them in the 'parent' namespace if they're forward-declared fixtures. > > But that's more of a nit, and if that style has changed out from under me, please do let me know and don't feel you have to make that change :) I put them in an extra namespace, this still seems the common pattern in //net and I'm not aware of any recent changes :). https://codereview.chromium.org/2775053002/diff/60001/net/tools/transport_sec... net/tools/transport_security_state_generator/bit_writer_unittest.cc:58: writer.WriteBits(170, 1); On 2017/03/30 at 15:03:32, Ryan Sleevi wrote: > It took a quick consult of the table to find out 170 was 1010101010. I'm not sure if 0xAA would have been clearer (I suppose only if you spend time reversing), but maybe worth a comment here on the binary value :) > > If only we had C++14 binary literals :) Added a comment and replaced 170 with 0xAA. https://codereview.chromium.org/2775053002/diff/60001/net/tools/transport_sec... File net/tools/transport_security_state_generator/huffman/huffman_builder_unittest.cc (right): https://codereview.chromium.org/2775053002/diff/60001/net/tools/transport_sec... net/tools/transport_security_state_generator/huffman/huffman_builder_unittest.cc:18: // Make sure some values have identical counts. On 2017/03/30 at 15:03:32, Ryan Sleevi wrote: > Doesn't this make _most_ values have identical counts? > > Or am I mentally parsing wrong? The intention is to make the values have an identical count to other values - but not identical to all other values. But like you said, *all* values will have a count that is identical to at least some other values. Rephrased the comment in an attempt to clarify. https://codereview.chromium.org/2775053002/diff/60001/net/tools/transport_sec... net/tools/transport_security_state_generator/huffman/huffman_builder_unittest.cc:54: for (uint8_t i = 0; i <= 63; i++) { On 2017/03/30 at 15:03:32, Ryan Sleevi wrote: > Why 63 (vs 127 above)? The i*2 means you'll skip 127 - desired or not? Yes, this intentionally skips over every odd value. Restructured this a bit so that it (hopefully) is a little clearer. https://codereview.chromium.org/2775053002/diff/60001/net/tools/transport_sec... net/tools/transport_security_state_generator/huffman/huffman_builder_unittest.cc:126: 0x1, 0x2)); On 2017/03/30 at 15:03:32, Ryan Sleevi wrote: > Sleevi stupidity - why 0x1/0x2? Every pair of uint8_t's represents a node in the tree. There are 4 nodes; Node 0: 0xE1 & 0xE2 Node 1: 0xE3 & 0x0 Node 2: 0xE4 & 0xE5 Node 3: 0x1 & 0x2 The last node (last 2 values in the vector) is always the root. The first value (0x1) is the left child and the second value (0x2) is the right child of the root. Because the MSBs are not set, these values "point" to other nodes. The actual value indicates which node they point to. The left child points to node 1 and the right child points to node 2. The left child of node 1 (0xE3) has its MSB set which means it is a leaf. The right child has its MSB not set which means it is a pointer to another node (node 0). The MSBs for both values in node 2 are set indicating that both children are leafs. The leaf values are obtained by ignoring the MSB (0xE4 & 0x7F = 0x64). Both children of node 0 are leafs. Fixed the labels in the tree, the ascii values for node 2 were incorrect. Also replaced 'elements' with 'nodes' in the comment.
The CQ bit was checked by martijn@martijnc.be
Sending this to the CQ now but if you have more questions or comments I'll follow up in a separate CL (I haven't made any major changes). Thanks!
The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org Link to the patchset: https://codereview.chromium.org/2775053002/#ps100001 (title: "fix a comment.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by martijn@martijnc.be
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1490984941026720,
"parent_rev": "d8b95e895d1b41b921ae255a3099f47245f5003c", "commit_rev":
"b9aca9d06e2948b32e03c1986ec4b47b66ee50dd"}
Message was sent while issue was closed.
Description was changed from ========== Add unittests for different transport security state generator components. BUG=595493 ========== to ========== Add unittests for different transport security state generator components. BUG=595493 Review-Url: https://codereview.chromium.org/2775053002 Cr-Commit-Position: refs/heads/master@{#461191} Committed: https://chromium.googlesource.com/chromium/src/+/b9aca9d06e2948b32e03c1986ec4... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:100001) as https://chromium.googlesource.com/chromium/src/+/b9aca9d06e2948b32e03c1986ec4... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
