Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2629093002:
Parameterize the CertVerifyProc tests so they can be run with (Closed)
|
Created:
3 years, 11 months ago by eroman Modified:
3 years, 10 months ago Reviewers:
Ryan Sleevi CC:
chromium-reviews, cbentzel+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionParameterize the CertVerifyProc tests so they can be run with
implementations other than CertVerifyProc::CreateDefault().
BUG=649017
Review-Url: https://codereview.chromium.org/2629093002
Cr-Commit-Position: refs/heads/master@{#448086}
Committed: https://chromium.googlesource.com/chromium/src/+/a51779b5e41d1d8e1498e8dc10d4795b5988515f
Patch Set 1 #
Total comments: 39
Patch Set 2 : Switch to TYPED_TEST() #Patch Set 3 : Fix headers #
Total comments: 1
Patch Set 4 : Revert back to patchset 1 (plus merge master) #Patch Set 5 : Re-apply some of the useful changes from patchset 2 #Patch Set 6 : Address comments #
Total comments: 8
Patch Set 7 : Address sleevi comments #Messages
Total messages: 35 (20 generated)
The CQ bit was checked by eroman@chromium.org 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.
eroman@chromium.org changed reviewers: + rsleevi@chromium.org
https://codereview.chromium.org/2629093002/diff/1/net/cert/cert_verify_proc_u... File net/cert/cert_verify_proc_unittest.cc (right): https://codereview.chromium.org/2629093002/diff/1/net/cert/cert_verify_proc_u... net/cert/cert_verify_proc_unittest.cc:113: CERT_VERIFY_PROC_BUILTIN, I will scrub all the "built-in" stuff prior to landing (so the CL remains a pure refactor). I left it in here temporarily to demonstrate how it plugs in.
So I left some initial comments. I'm struggling right now to determine whether the complexity is justified, and if I can think of any simpler solutions to recommend. Perhaps it'd be useful to know if there were design alternatives that you considered but discarded? https://codereview.chromium.org/2629093002/diff/1/net/cert/cert_verify_proc_u... File net/cert/cert_verify_proc_unittest.cc (right): https://codereview.chromium.org/2629093002/diff/1/net/cert/cert_verify_proc_u... net/cert/cert_verify_proc_unittest.cc:104: enum CertVerifyProcName { So I had much more feedback originally written here, but I think two things deserve calling out: 1) We're effectively reimplementing RTTI and type_id() - that is, this is all a bunch of dynamic_cast<>s - and that does feel a little weird - but I can see why and where we need it 2) The naming throws me off here, because I associate 'name' with string and 'id' with identifier. So I expected CertVerifyProcName to be a string, when it's an enum, which then makes ToString weird. https://codereview.chromium.org/2629093002/diff/1/net/cert/cert_verify_proc_u... net/cert/cert_verify_proc_unittest.cc:118: CertVerifyProcName GetDefaultCertVerifyProcName() { I'm torn on the appropriateness of this code duplication (and myself felt both guilt and lack of creativity w/r/t doing something similar with AreSHA1IntermediatesAllowed) The problem is it seems that this design choice itself introduces a lot of secondary complexity that I think we can reduce/eliminate. The problem is I'm trying to find whether my unease at reading this code is simply because I wrote the code you're replacing, or whether it's a legitimate remark about the unnecessary complexity in your CL. I don't have a good answer - so I'd simply ask - do you think the complexity is justified and there's no simpler, clearer solution (ideally without so many helper functions)? https://codereview.chromium.org/2629093002/diff/1/net/cert/cert_verify_proc_u... net/cert/cert_verify_proc_unittest.cc:136: const char* CertVerifyProcNameToString(CertVerifyProcName verify_proc_name) { I don't believe this should be a helper function - you have exactly one call, which is just VerifyProcTypeToName https://codereview.chromium.org/2629093002/diff/1/net/cert/cert_verify_proc_u... net/cert/cert_verify_proc_unittest.cc:157: bool TargetIsIphoneSimulator() { Was it necessary to make this a function? Couldn't this just be a bool variable? https://codereview.chromium.org/2629093002/diff/1/net/cert/cert_verify_proc_u... net/cert/cert_verify_proc_unittest.cc:191: bool WeakKeysAreInvalid(CertVerifyProcName verify_proc_name) { Considering you're introducing a test fixture, I'm not sure why these helpers can't or shouldn't be moved to the base class and the passing of CertVerifyProcName dropped https://codereview.chromium.org/2629093002/diff/1/net/cert/cert_verify_proc_u... net/cert/cert_verify_proc_unittest.cc:241: return CertVerifyProc::CreateDefault(); My instinct is that trying to preserve this is perhaps the root cause of the complexity. Do you feel the same? https://codereview.chromium.org/2629093002/diff/1/net/cert/cert_verify_proc_u... net/cert/cert_verify_proc_unittest.cc:255: CertVerifyProcName* verify_proc_name) = 0; So it's unclear why you have the function return the fully constructed object, and stashing the name of it into the |verify_proc_name|, rather than just having the virtual function that tells you what sort of CertVerifyProc to create That is, 321 and 327 are identical - so why not just have the subclass tell you the name to run? https://codereview.chromium.org/2629093002/diff/1/net/cert/cert_verify_proc_u... net/cert/cert_verify_proc_unittest.cc:282: bool UsingCertVerifyProcAndroid() const { We're in CertVerifyProcUnittest - the use of "CertVerifyProc" here and in the types feels entirely redundant/unnecessary UsingAndroid/UsingNSS/UsingOpenSSL/ are possible simplifications there, but I also question whether more fundamentally, we're just interested in default vs builtin? https://codereview.chromium.org/2629093002/diff/1/net/cert/cert_verify_proc_u... net/cert/cert_verify_proc_unittest.cc:581: // certificates provided by servers. This is probably fixed by mattm's changes. https://codereview.chromium.org/2629093002/diff/1/net/cert/cert_verify_proc_u... net/cert/cert_verify_proc_unittest.cc:905: TEST_P(CertVerifyProcTest, InvalidKeyUsage) { This seems like one of those tests we should be using our own files for, and if so, would eliminate the result divergence. https://codereview.chromium.org/2629093002/diff/1/net/cert/cert_verify_proc_u... net/cert/cert_verify_proc_unittest.cc:1305: verify_proc_name == CERT_VERIFY_PROC_MAC; I wonder whether these should be different tests (like OCSP) https://codereview.chromium.org/2629093002/diff/1/net/cert/cert_verify_proc_u... net/cert/cert_verify_proc_unittest.cc:1853: // a mock CertVerifyProc rather than one of the concrete types. My two cents is I'm not sure that this comment is necessary.
https://codereview.chromium.org/2629093002/diff/1/net/cert/cert_verify_proc_u... File net/cert/cert_verify_proc_unittest.cc (right): https://codereview.chromium.org/2629093002/diff/1/net/cert/cert_verify_proc_u... net/cert/cert_verify_proc_unittest.cc:104: enum CertVerifyProcName { On 2017/01/12 20:01:56, Ryan Sleevi wrote: > So I had much more feedback originally written here, but I think two things > deserve calling out: > > 1) We're effectively reimplementing RTTI and type_id() - that is, this is all a > bunch of dynamic_cast<>s - and that does feel a little weird - but I can see why > and where we need it > 2) The naming throws me off here, because I associate 'name' with string and > 'id' with identifier. So I expected CertVerifyProcName to be a string, when it's > an enum, which then makes ToString weird. > Before I dive into the other comments, let's hash out your design concerns. I was actually satisfied with the resulting organization of the unit-tests, finding it more readable than before (and having gone through several other iterations that I didn't like). Fundamentally the problem we have is that test expectations depend on internal details of the verifier that is used (and to a lesser degree the OS version it is run on). Primarily it depends on the concrete class of CertVerifyProc that is being used (and in some cases the OS version as well). In the currently checked in code, this is expressed by sprinkling #if throughout the code, such as: #if defined(OS_MACOSX) && !defined(OS_IOS) ... #endif or #if defined(USE_OPENSSL_CERTS) && !defined(OS_ANDROID) ... #endif These #ifs replicate logic internal to CertVerifyProc::CreateDefault(), and constitute the same layering violation that you do not like with the CertVerifyProcName. My first step was to isolate these #ifs into functions, by representing the verifier under test as a CertVerifyProcName, which re-organizes how that information is accessed. I found this to simplify readability and compostability of the code, for the usual reasons that #if in code is bad (can get compile failures on all platforms when modifying tests, rather than only when compiling in certain configurations; something which bit me in past iterations when I kept the #ifdefs inline). The second step then was to parameterize CertVerifyProcName so the tests can be instantiated and run for more than one (and behave appropriately). The only real alternatives in GTest is to parameterize on a type (which has problems I will get into later), or instantiate tests manually (with its attendant problems). Brainstorming through other ideas: (1) Remove all verifier-specific differences between tests, by dumbing down the tests. This has the disadvantage of losing test coverage, which I consider undesirable. (2) Instantiate tests separately for each of the CertVerifyProc*, in their own _unittest.cc file. The difference in test expectations would still exist in the generic code, however would be communicated via a delegate when instantiating the CertVerifyProc tests. Testing would be explicit for concrete CertVerifyProc rather than using CertVerifyProc::CreateDefault(). There are other variations on (2), but fundamentally the approach is to use a test policy delegate, rather selecting the behavior by a dispatch on the type (within the test). Structurally I find this makes the tests (and differences in behavior) harder to understand, because it is de-coupled from the type. The other structure problem is disabling tests also needs to be described as such a policy. How to split the tests in this manner is also non-trivial. GTest has some support for type-parameterized tests, but it is quite difficult to use and I gave up on getting it to work the way I wanted. Splitting things in this manner will be a much less iterative change that what I have proposed, and from my early attempts I found it to be quite a bit more complicated and not something I would recommend.
I can't convince myself that the template solution we discussed yesterday is fundamentally better, so let's go with this and see if we can't cut and/or simplify some of this further. A few comments below, plus some of the original comments/questions. https://codereview.chromium.org/2629093002/diff/1/net/cert/cert_verify_proc_u... File net/cert/cert_verify_proc_unittest.cc (right): https://codereview.chromium.org/2629093002/diff/1/net/cert/cert_verify_proc_u... net/cert/cert_verify_proc_unittest.cc:104: enum CertVerifyProcName { VerifyProcType? https://codereview.chromium.org/2629093002/diff/1/net/cert/cert_verify_proc_u... net/cert/cert_verify_proc_unittest.cc:165: bool SupportsReturningVerifiedChain(CertVerifyProcName verify_proc_name) { Despite our discussion, I'm still a little hazy about why you made this a free function versus adding the the test fixture. I think some of my unease might trace to the fact that you tried to keep this CL minimal (yay!), but that means it left it inconsistent for reasons that aren't clear - why are some helpers free functions, why are some on the base class, what rules do you use to distinguish when you pass the CVPName versus when you access verify_proc_name_ , etc. https://codereview.chromium.org/2629093002/diff/1/net/cert/cert_verify_proc_u... net/cert/cert_verify_proc_unittest.cc:229: } Let's definitely ditch this. https://codereview.chromium.org/2629093002/diff/1/net/cert/cert_verify_proc_u... net/cert/cert_verify_proc_unittest.cc:235: } Do you actually need this to be a function? Unittests are allowed to do static initializers, and you could always slap a constexpr on the GetDefaultCertVerifyProcName() and force it to be a compile-time check if you wanted. https://codereview.chromium.org/2629093002/diff/1/net/cert/cert_verify_proc_u... net/cert/cert_verify_proc_unittest.cc:241: return CertVerifyProc::CreateDefault(); So I don't think the EXPECT_EQ does anything to prevent the version shear you're worried about, because you're still calling CreateDefault() here - the only difference is that you might "think" you're testing CertVerifyProcMac, but you're actually testing CertVerifyProcBuiltin (because that's what CreateDefault returned) It seems that the only defense against shear (where the test differs from what CreateDefault returns) is that you're expecting different results on some platforms, but that doesn't seem to be in and of itself a hard guarantee. https://codereview.chromium.org/2629093002/diff/1/net/cert/cert_verify_proc_u... net/cert/cert_verify_proc_unittest.cc:286: bool UsingCertVerifyProcNSS() const { If we're going to go with these helper functions (which, to be honest, I'm not terribly keen on, but I can live with), it does seem some documentation about guarantees - such as how there should only ever be one of them true. https://codereview.chromium.org/2629093002/diff/1/net/cert/cert_verify_proc_u... net/cert/cert_verify_proc_unittest.cc:331: // CertVerifyProc::CreateDefault() implementation. Did you consider making an explicit fixture for each of those tests?
The CQ bit was checked by eroman@chromium.org 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...
Switched to TYPE_TEST(). See if you like that better before I respond to the other comments.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by eroman@chromium.org 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...
This is why I said hold up before going down that route, so I can at least check the advice is sane/reasonable :( I didn't want to waste your time if it wasn't a good line of thinking, and as the last review notes mentioned, I couldn't convince myself that it'd be better / significantly different than what you had. I can offer a lot of feedback on how to simplify these templates and a lot of the boilerplate, but before I do so - do you want to go down the template route? Or would you prefer the original thing? We can greatly simplify this CL, but that just means even more work for you - and unless you feel that the templates afforded more flexibility (even in their more verbose state), we probably should go back to the path you were exploring.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I had already started exploring some of the alternatives we discussed in person yesterday, so figured I would finish it off. If nothing else in going through the tests again I fixed a couple more problems (some tests that don't need to be parameterized by type as all the exercised was a mock). I don't particularly care whether we settle on the template approach or not; they both have their tradeoffs. Go ahead and send me your simplifications, I can apply them to this patchset. Note that I didn't forward declare the proc types since that runs afoul of unused code warnings unless the template specializations are #if-ed out. And rolling up policies into a single traits object rather than per-function specializations centralizes the #ifs better, but is ugly in different ways.
https://codereview.chromium.org/2629093002/diff/40001/net/cert/cert_verify_pr... File net/cert/cert_verify_proc_unittest.cc (right): https://codereview.chromium.org/2629093002/diff/40001/net/cert/cert_verify_pr... net/cert/cert_verify_proc_unittest.cc:113: } As mentioned in the other review, the naming here feels like it could be reduced (to "IsNSS") That said, you can specialize the typetraits and avoid the function call entirely I'm not sure your unused code warnings, but I would have thought something like namespace net { class CertVerifyProcMac; class CertVerifyProcNSS; class CertVerifyProcIOS; ... } #if defined(USE_NSS) #include "net/cert/cert_verify_proc_nss.h" #elif .... #endif template <typename T> struct IsNSS : public std::is_same<CertVerifyProcNSS, T>; template <typename T> struct IsIOS : public std::is_same<CertVerifyProcIOS, T>; ... If we wanted to go to an allknowing-object approach template <typename T> struct CertVerifierTraits { bool is_nss = std::is_same<T, CertVerifyProcNSS>::value; bool is_ios = std::is_same<T, CertVerifyProcIOS>::value; ... bool supports_crls = std::is_same<T, CertVerifyProcNSS>::value || std::is_same<T, CertVerifyProcWin>::value || std::is_same<T, CertVerifyProcMac>::value }; ( I think the || should work, if not, it'd have to be wrapped up in an std::conditional as std::conditional<std::is_same<...> || std::is_same<...>, true, false> or std::conditional<std::is_same<...>, true, std::conditional<std::is_same<...>, true, false>> but I think the above should work as written Of course, that only works for the cases of compile-time detection, and we do have a few of run-time detection (Android & macOS & win versions), so those have to be functions since we can't constexpr them. Unfortunately (and the reason I ended up suggesting it wasn't as much a win), is that we can't escape the #ifdef hell because of something like this: template <typename T> bool DetectsKnownRoots(T* unused) { return true; } // The following must be guarded by #ifdefs, even though the goal of templates was to reduce the ifdefs #if defined(OS_ANDROID) template <> bool DetectsKnownRoots(CertVerifyProcAndroid* android) { return base::android::BuildInfo::GetInstance()->sdk_int() >= 17; } #endif That is, even though we get away with the forward declarations of types, for the runtime detection stuff, we need the platform-specific bits. For everything else, however, beyond those platform-specific APIs, I think it works. But the platform-specific bits were enough to make me think it wasn't as big a savings as I hoped for.
PTAL. I have rolled back to patchset #1, and tried to address the comments using that approach rather than the TYPED_TEST() version. https://codereview.chromium.org/2629093002/diff/1/net/cert/cert_verify_proc_u... File net/cert/cert_verify_proc_unittest.cc (right): https://codereview.chromium.org/2629093002/diff/1/net/cert/cert_verify_proc_u... net/cert/cert_verify_proc_unittest.cc:104: enum CertVerifyProcName { On 2017/01/13 17:22:09, Ryan Sleevi wrote: > VerifyProcType? Done. https://codereview.chromium.org/2629093002/diff/1/net/cert/cert_verify_proc_u... net/cert/cert_verify_proc_unittest.cc:113: CERT_VERIFY_PROC_BUILTIN, On 2017/01/12 07:26:09, eroman (slow) wrote: > I will scrub all the "built-in" stuff prior to landing (so the CL remains a pure > refactor). > > I left it in here temporarily to demonstrate how it plugs in. Done. https://codereview.chromium.org/2629093002/diff/1/net/cert/cert_verify_proc_u... net/cert/cert_verify_proc_unittest.cc:118: CertVerifyProcName GetDefaultCertVerifyProcName() { On 2017/01/12 20:01:56, Ryan Sleevi wrote: > I'm torn on the appropriateness of this code duplication (and myself felt both > guilt and lack of creativity w/r/t doing something similar with > AreSHA1IntermediatesAllowed) > > The problem is it seems that this design choice itself introduces a lot of > secondary complexity that I think we can reduce/eliminate. The problem is I'm > trying to find whether my unease at reading this code is simply because I wrote > the code you're replacing, or whether it's a legitimate remark about the > unnecessary complexity in your CL. I don't have a good answer - so I'd simply > ask - do you think the complexity is justified and there's no simpler, clearer > solution (ideally without so many helper functions)? Acknowledged. (Renamed ProcName --> ProcType) https://codereview.chromium.org/2629093002/diff/1/net/cert/cert_verify_proc_u... net/cert/cert_verify_proc_unittest.cc:136: const char* CertVerifyProcNameToString(CertVerifyProcName verify_proc_name) { On 2017/01/12 20:01:56, Ryan Sleevi wrote: > I don't believe this should be a helper function - you have exactly one call, > which is just VerifyProcTypeToName Done. https://codereview.chromium.org/2629093002/diff/1/net/cert/cert_verify_proc_u... net/cert/cert_verify_proc_unittest.cc:157: bool TargetIsIphoneSimulator() { On 2017/01/12 20:01:56, Ryan Sleevi wrote: > Was it necessary to make this a function? Couldn't this just be a bool variable? Done. https://codereview.chromium.org/2629093002/diff/1/net/cert/cert_verify_proc_u... net/cert/cert_verify_proc_unittest.cc:165: bool SupportsReturningVerifiedChain(CertVerifyProcName verify_proc_name) { On 2017/01/13 17:22:09, Ryan Sleevi wrote: > Despite our discussion, I'm still a little hazy about why you made this a free > function versus adding the the test fixture. > > I think some of my unease might trace to the fact that you tried to keep this CL > minimal (yay!), but that means it left it inconsistent for reasons that aren't > clear - why are some helpers free functions, why are some on the base class, > what rules do you use to distinguish when you pass the CVPName versus when you > access verify_proc_name_ , etc. I went ahead and moved them all into the base fixture (and removed the parameter). I was trying to keep things minimal, but reorganizing now should provide some clarity. https://codereview.chromium.org/2629093002/diff/1/net/cert/cert_verify_proc_u... net/cert/cert_verify_proc_unittest.cc:191: bool WeakKeysAreInvalid(CertVerifyProcName verify_proc_name) { On 2017/01/12 20:01:56, Ryan Sleevi wrote: > Considering you're introducing a test fixture, I'm not sure why these helpers > can't or shouldn't be moved to the base class and the passing of > CertVerifyProcName dropped Done. https://codereview.chromium.org/2629093002/diff/1/net/cert/cert_verify_proc_u... net/cert/cert_verify_proc_unittest.cc:229: } On 2017/01/13 17:22:09, Ryan Sleevi wrote: > Let's definitely ditch this. It is useful to to have the test name be: XXX/CertVerifyProcMac XXX/CertVerifyProcBuiltin Instead of: XXX/1 XXX/2 I have kept this for now, however deleted CertVerifyProcNameToString() per the earlier comment. https://codereview.chromium.org/2629093002/diff/1/net/cert/cert_verify_proc_u... net/cert/cert_verify_proc_unittest.cc:235: } On 2017/01/13 17:22:09, Ryan Sleevi wrote: > Do you actually need this to be a function? Unittests are allowed to do static > initializers, and you could always slap a constexpr on the > GetDefaultCertVerifyProcName() and force it to be a compile-time check if you > wanted. Done. https://codereview.chromium.org/2629093002/diff/1/net/cert/cert_verify_proc_u... net/cert/cert_verify_proc_unittest.cc:241: return CertVerifyProc::CreateDefault(); On 2017/01/13 17:22:09, Ryan Sleevi wrote: > So I don't think the EXPECT_EQ does anything to prevent the version shear you're > worried about, because you're still calling CreateDefault() here - the only > difference is that you might "think" you're testing CertVerifyProcMac, but > you're actually testing CertVerifyProcBuiltin (because that's what CreateDefault > returned) > > It seems that the only defense against shear (where the test differs from what > CreateDefault returns) is that you're expecting different results on some > platforms, but that doesn't seem to be in and of itself a hard guarantee. The EXPECT_EQ() was to guard against me adding CERT_VERIFY_PROC_BUILTIN to the kAllVerifiers, but failing to update this line. The consequence of that mistake is all the tests for BUILTIN would trivially pass as they are testing the default verifier rather than the built-in verifier. I can remove the check if you like, since I will be verifying manually at such time too. https://codereview.chromium.org/2629093002/diff/1/net/cert/cert_verify_proc_u... net/cert/cert_verify_proc_unittest.cc:255: CertVerifyProcName* verify_proc_name) = 0; On 2017/01/12 20:01:56, Ryan Sleevi wrote: > So it's unclear why you have the function return the fully constructed object, > and stashing the name of it into the |verify_proc_name|, rather than just having > the virtual function that tells you what sort of CertVerifyProc to create > > That is, 321 and 327 are identical - so why not just have the subclass tell you > the name to run? Done. https://codereview.chromium.org/2629093002/diff/1/net/cert/cert_verify_proc_u... net/cert/cert_verify_proc_unittest.cc:282: bool UsingCertVerifyProcAndroid() const { On 2017/01/12 20:01:56, Ryan Sleevi wrote: > We're in CertVerifyProcUnittest - the use of "CertVerifyProc" here and in the > types feels entirely redundant/unnecessary > > UsingAndroid/UsingNSS/UsingOpenSSL/ are possible simplifications there, but I > also question whether more fundamentally, we're just interested in default vs > builtin? Removed these functions. https://codereview.chromium.org/2629093002/diff/1/net/cert/cert_verify_proc_u... net/cert/cert_verify_proc_unittest.cc:286: bool UsingCertVerifyProcNSS() const { On 2017/01/13 17:22:09, Ryan Sleevi wrote: > If we're going to go with these helper functions (which, to be honest, I'm not > terribly keen on, but I can live with), it does seem some documentation about > guarantees - such as how there should only ever be one of them true. N/A https://codereview.chromium.org/2629093002/diff/1/net/cert/cert_verify_proc_u... net/cert/cert_verify_proc_unittest.cc:331: // CertVerifyProc::CreateDefault() implementation. On 2017/01/13 17:22:09, Ryan Sleevi wrote: > Did you consider making an explicit fixture for each of those tests? That entails duplicating fixtures in at least 4 places. Do you want me to go ahead with that? https://codereview.chromium.org/2629093002/diff/1/net/cert/cert_verify_proc_u... net/cert/cert_verify_proc_unittest.cc:581: // certificates provided by servers. On 2017/01/12 20:01:56, Ryan Sleevi wrote: > This is probably fixed by mattm's changes. Acknowledged -- left a TODO https://codereview.chromium.org/2629093002/diff/1/net/cert/cert_verify_proc_u... net/cert/cert_verify_proc_unittest.cc:905: TEST_P(CertVerifyProcTest, InvalidKeyUsage) { On 2017/01/12 20:01:56, Ryan Sleevi wrote: > This seems like one of those tests we should be using our own files for, and if > so, would eliminate the result divergence. Acknowledged -- left a TODO https://codereview.chromium.org/2629093002/diff/1/net/cert/cert_verify_proc_u... net/cert/cert_verify_proc_unittest.cc:1305: verify_proc_name == CERT_VERIFY_PROC_MAC; On 2017/01/12 20:01:56, Ryan Sleevi wrote: > I wonder whether these should be different tests (like OCSP) When you say "tests" do you mean unit-tests, or do you mean adding something like CertVerifyProc::SupportsCRLSet ? https://codereview.chromium.org/2629093002/diff/1/net/cert/cert_verify_proc_u... net/cert/cert_verify_proc_unittest.cc:1853: // a mock CertVerifyProc rather than one of the concrete types. On 2017/01/12 20:01:56, Ryan Sleevi wrote: > My two cents is I'm not sure that this comment is necessary. Done (removed) Alternately I could re-write it as I had in the follow-up patchset: // Tests to ensure that CertVerifyProc::Verify() sets the has_* members for // hash algorithms.
Mostly LG with a few nits. And I can see there's a lot of crap I'm on the hook to cleanup that I haven't gotten around to (e.g. you touching old tests reminds me I never fixed/deleted said tests) https://codereview.chromium.org/2629093002/diff/100001/net/cert/cert_verify_p... File net/cert/cert_verify_proc_unittest.cc (right): https://codereview.chromium.org/2629093002/diff/100001/net/cert/cert_verify_p... net/cert/cert_verify_proc_unittest.cc:192: } This use of a static is forbidden - https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables I'm not sure why we wouldn't just CertificateList() ? https://codereview.chromium.org/2629093002/diff/100001/net/cert/cert_verify_p... net/cert/cert_verify_proc_unittest.cc:200: class CertVerifyProcBaseClassTest : public testing::Test {}; Am I wrong for thinking that every instance of this class (that has a verify proc) ends up using a MockCertVerifyProc? Should this class instead just set up a MCV and let the caller override the result for the cert? It's a small point though. Naming wise "BaseClassTest" and "BaseTest" are incredibly similar and somewhat confusing to distinguish. Would "CertVerifyProcTest" (for BaseClass) and "CertVerifyProcInternalBase" make it clearer? Do we actually *need* a fixture for CertVerifyProcBaseClassTest? Is it just to friend the specific tests? Should we do that instead? https://codereview.chromium.org/2629093002/diff/100001/net/cert/cert_verify_p... net/cert/cert_verify_proc_unittest.cc:209: verify_proc_ = CertVerifyProc::CreateDefault(); I'm still confused why the delegate gets to set the type but isn't responsible for constructing it. Why not just have the delegate provide the type-and-value? https://codereview.chromium.org/2629093002/diff/100001/net/cert/cert_verify_p... net/cert/cert_verify_proc_unittest.cc:928: auto verify_proc = make_scoped_refptr(new MockCertVerifyProc(dummy_result)); It's a small pedantry nit, but your use of auto semantically changes this code (from working on a scoped_refptr<CertVerifyProc> to now working on the scoped_refptr<MockCertVerifyProc>) This is part of my general hated for auto-plus-pointer-types (and which there have been many a character spilled over email discussing)
The CQ bit was checked by eroman@chromium.org to run a CQ dry run
https://codereview.chromium.org/2629093002/diff/100001/net/cert/cert_verify_p... File net/cert/cert_verify_proc_unittest.cc (right): https://codereview.chromium.org/2629093002/diff/100001/net/cert/cert_verify_p... net/cert/cert_verify_proc_unittest.cc:192: } On 2017/02/01 23:46:38, Ryan Sleevi wrote: > This use of a static is forbidden - > https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables > > I'm not sure why we wouldn't just CertificateList() ? Done. https://codereview.chromium.org/2629093002/diff/100001/net/cert/cert_verify_p... net/cert/cert_verify_proc_unittest.cc:200: class CertVerifyProcBaseClassTest : public testing::Test {}; On 2017/02/01 23:46:38, Ryan Sleevi wrote: > Am I wrong for thinking that every instance of this class (that has a verify > proc) ends up using a MockCertVerifyProc? > > Should this class instead just set up a MCV and let the caller override the > result for the cert? > > It's a small point though. > > Naming wise "BaseClassTest" and "BaseTest" are incredibly similar and somewhat > confusing to distinguish. Would "CertVerifyProcTest" (for BaseClass) and > "CertVerifyProcInternalBase" make it clearer? > > Do we actually *need* a fixture for CertVerifyProcBaseClassTest? Is it just to > friend the specific tests? Should we do that instead? Done. https://codereview.chromium.org/2629093002/diff/100001/net/cert/cert_verify_p... net/cert/cert_verify_proc_unittest.cc:209: verify_proc_ = CertVerifyProc::CreateDefault(); On 2017/02/01 23:46:38, Ryan Sleevi wrote: > I'm still confused why the delegate gets to set the type but isn't responsible > for constructing it. Why not just have the delegate provide the type-and-value? In my earlier version the derived class worked that way (created the verifier and assigned the type) but your comment suggested having it be responsible for just deciding the type. At any rate, this is no longer applicable as I got rid of the base class. https://codereview.chromium.org/2629093002/diff/100001/net/cert/cert_verify_p... net/cert/cert_verify_proc_unittest.cc:928: auto verify_proc = make_scoped_refptr(new MockCertVerifyProc(dummy_result)); On 2017/02/01 23:46:38, Ryan Sleevi wrote: > It's a small pedantry nit, but your use of auto semantically changes this code > (from working on a scoped_refptr<CertVerifyProc> to now working on the > scoped_refptr<MockCertVerifyProc>) > > This is part of my general hated for auto-plus-pointer-types (and which there > have been many a character spilled over email discussing) Done (although testing via the Mock interface shouldn't be harmful here).
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.
ping
Sorry, should have LGTM'd that, since they were nits
The CQ bit was checked by eroman@chromium.org
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": 120001, "attempt_start_ts": 1486157104931040,
"parent_rev": "d09c38982b3b999ecfa67acfb8018d0b198bc4bd", "commit_rev":
"a51779b5e41d1d8e1498e8dc10d4795b5988515f"}
Message was sent while issue was closed.
Description was changed from ========== Parameterize the CertVerifyProc tests so they can be run with implementations other than CertVerifyProc::CreateDefault(). BUG=649017 ========== to ========== Parameterize the CertVerifyProc tests so they can be run with implementations other than CertVerifyProc::CreateDefault(). BUG=649017 Review-Url: https://codereview.chromium.org/2629093002 Cr-Commit-Position: refs/heads/master@{#448086} Committed: https://chromium.googlesource.com/chromium/src/+/a51779b5e41d1d8e1498e8dc10d4... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/a51779b5e41d1d8e1498e8dc10d4... |
