Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
207063003:
Add missing call to TestRootCerts::Clear() after usage. (Closed)
|
Created:
6 years, 9 months ago by haavardm Modified:
6 years, 9 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdd missing calls to TestRootCerts::Clear() after usage.
The next unit test that uses TestRootCerts may fail if
TestRootCerts instance is not empty. So every unit test
that uses TestRootCerts needs to clear it at the end.
BUG=none
Test=none
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=259558
Patch Set 1 : Add missing calls to TestRootCerts::Clear() #
Total comments: 3
Messages
Total messages: 16 (0 generated)
I rather small patch to fix a test failure on linux using OpenSSL.
Patch set 1 LGTM. Thanks! General comments: I really like that you always describe the bug and your fix in detail. I just wanted to note that the CL's description is not always the best place for that info. Sometimes that info should be in the source code, so that someone who reads the code can get the info without having to look it up. Sometimes the info should be in a code review comment or in the bug report if it contains a lot of details. https://codereview.chromium.org/207063003/diff/1/net/cert/cert_verify_proc_un... File net/cert/cert_verify_proc_unittest.cc (right): https://codereview.chromium.org/207063003/diff/1/net/cert/cert_verify_proc_un... net/cert/cert_verify_proc_unittest.cc:1017: root_certs->Clear(); Please add a comment to note why it is necessary to clear the test root certs store. At first I thought you just wanted to test the Clear() method.
I wonder if we should add a ScopedTestRootCerts helper class to ensure that a unit test always clears the test root certs store at the end.
We have ScopedTestRoot. It is used elsewhere, to avoid this problem. I believe we did not use it here because this is the implementation under test, and wanted to avoid coupling as much of the test as possible. We could always use a TestFixture to clear it. On Mar 25, 2014 9:02 AM, <wtc@chromium.org> wrote: > I wonder if we should add a ScopedTestRootCerts helper class to ensure > that a > unit test always clears the test root certs store at the end. > > https://codereview.chromium.org/207063003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/03/25 16:00:19, wtc wrote: > Patch set 1 LGTM. Thanks! > > General comments: > > I really like that you always describe the bug and your fix in detail. I just > wanted to note that the CL's description is not always the best place for that > info. Sometimes that info should be in the source code, so that someone who > reads the code can get the info without having to look it up. Sometimes the info > should be in a code review comment or in the bug report if it contains a lot of > details. Right. So info needed for reviewers in a comment, info needed to understand the code in the code, and general info about the patch in the CL description. Does that sound correct? > > https://codereview.chromium.org/207063003/diff/1/net/cert/cert_verify_proc_un... > File net/cert/cert_verify_proc_unittest.cc (right): > > https://codereview.chromium.org/207063003/diff/1/net/cert/cert_verify_proc_un... > net/cert/cert_verify_proc_unittest.cc:1017: root_certs->Clear(); > > Please add a comment to note why it is necessary to clear the test root certs > store. At first I thought you just wanted to test the Clear() method.
https://codereview.chromium.org/207063003/diff/1/net/cert/cert_verify_proc_un... File net/cert/cert_verify_proc_unittest.cc (right): https://codereview.chromium.org/207063003/diff/1/net/cert/cert_verify_proc_un... net/cert/cert_verify_proc_unittest.cc:1017: root_certs->Clear(); On 2014/03/25 16:00:19, wtc wrote: > > Please add a comment to note why it is necessary to clear the test root certs > store. At first I thought you just wanted to test the Clear() method. This call is done like this many places (basically everytime TestRootCerts::GetInstance() is used without ScopedTestRoot). So in that case I should make a comment for all of these.
On 2014/03/25 17:17:31, Håvard Molland wrote: > > Right. So info needed for reviewers in a comment, info needed to > understand the code in the code, and general info about the patch in the CL > description. > > Does that sound correct? Yes. The most important point is: - Info needed to understand the code should be in the code. Everything else is a matter of personal preference. You can take a look at what other Chromium developers are doing to get a feeling of the level of details appropriate for CL descriptions: http://src.chromium.org/viewvc/chrome/trunk/src/net/cert/cert_verify_proc_uni... As a concrete example, for this CL, I think the following two paragraphs would be enough: Add missing calls to TestRootCerts::Clear() after usage. The next unit test that uses TestRootCerts may fail if TestRootCerts instance is not empty. So every unit test that uses TestRootCerts needs to clear it at the end. Details such as the actual unit test that failed and the build configuration just need to be in the code review or bug report. Thanks.
https://codereview.chromium.org/207063003/diff/1/net/cert/cert_verify_proc_un... File net/cert/cert_verify_proc_unittest.cc (right): https://codereview.chromium.org/207063003/diff/1/net/cert/cert_verify_proc_un... net/cert/cert_verify_proc_unittest.cc:1017: root_certs->Clear(); On 2014/03/25 17:18:03, Håvard Molland wrote: > > This call is done like this many places (basically everytime > TestRootCerts::GetInstance() is used without ScopedTestRoot). So in that case I > should make a comment for all of these. I see. If you think adding a comment for all of these would be excessive, then we don't need to do that. I didn't remember how TestRootCerts works, so I was confused by this.
The CQ bit was checked by haavardm@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/haavardm@opera.com/207063003/1
On 2014/03/25 22:24:13, wtc wrote: > https://codereview.chromium.org/207063003/diff/1/net/cert/cert_verify_proc_un... > File net/cert/cert_verify_proc_unittest.cc (right): > > https://codereview.chromium.org/207063003/diff/1/net/cert/cert_verify_proc_un... > net/cert/cert_verify_proc_unittest.cc:1017: root_certs->Clear(); > > On 2014/03/25 17:18:03, Håvard Molland wrote: > > > > This call is done like this many places (basically everytime > > TestRootCerts::GetInstance() is used without ScopedTestRoot). So in that case > I > > should make a comment for all of these. > > I see. If you think adding a comment for all of these would be excessive, then > we don't need to do that. I didn't remember how TestRootCerts works, so I was > confused by this. Thanks, I leave it as is then.
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) content_browsertests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by haavardm@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/haavardm@opera.com/207063003/1
Message was sent while issue was closed.
Change committed as 259558 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
