Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1907443005:
Disable CertVerifyProcTest.PaypalNullCertParsing (Closed)
DescriptionDisable CertVerifyProcTest.PaypalNullCertParsing
The test expectation is incorrect on some configurations, so disable the
test until it is fixed (better to have a bug to track a failing test than
a false sense of security due to false positive).
BUG=605457
Committed: https://crrev.com/743c8e4de5881808684cf30e58f83f441010d2c4
Cr-Commit-Position: refs/heads/master@{#388770}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Change condition & remove code #Patch Set 3 : Unconditionally disable test #Messages
Total messages: 15 (6 generated)
sdefresne@chromium.org changed reviewers: + davidben@chromium.org, rsleevi@chromium.org
Can one of you take a look?
https://codereview.chromium.org/1907443005/diff/1/net/cert/cert_verify_proc_u... File net/cert/cert_verify_proc_unittest.cc (right): https://codereview.chromium.org/1907443005/diff/1/net/cert/cert_verify_proc_u... net/cert/cert_verify_proc_unittest.cc:209: #if !defined(OS_IOS) As explained in the previous CL (but perhaps unclearly), this should be disabled across all the platforms that aren't rejecting it as invalid #if defined(USE_NSS_CERTS) || defined(OS_ANDROID) || (defined(OS_IOS) && TARGET_IPHONE_SIMULATOR && version < 9,3,0)) #define MAYBE_... PayPal #else #define MAYBE_... DISABLED_ #endif Then remove line 242-246 (which is where the bug was introduced), remove the #ifdefs around 250/253, and that should do the 'proper' test.
Description was changed from ========== Disable CertVerifyProcTest.PaypalNullCertParsing on iOS. The test expectation are incorrect on iOS, so disable the test until it is fixed (better to have a bug to track a failing test than a false sense of security due to false positive). BUG=605457 ========== to ========== Disable CertVerifyProcTest.PaypalNullCertParsing on some configurations. The test expectation is incorrect on some configurations, so disable the test until it is fixed (better to have a bug to track a failing test than a false sense of security due to false positive). BUG=605457 ==========
Please take another look. https://codereview.chromium.org/1907443005/diff/1/net/cert/cert_verify_proc_u... File net/cert/cert_verify_proc_unittest.cc (right): https://codereview.chromium.org/1907443005/diff/1/net/cert/cert_verify_proc_u... net/cert/cert_verify_proc_unittest.cc:209: #if !defined(OS_IOS) On 2016/04/21 at 11:29:21, Ryan Sleevi wrote: > As explained in the previous CL (but perhaps unclearly), this should be disabled across all the platforms that aren't rejecting it as invalid > > #if defined(USE_NSS_CERTS) || defined(OS_ANDROID) || (defined(OS_IOS) && TARGET_IPHONE_SIMULATOR && version < 9,3,0)) > #define MAYBE_... PayPal > #else > #define MAYBE_... DISABLED_ > #endif > > Then remove line 242-246 (which is where the bug was introduced), remove the #ifdefs around 250/253, and that should do the 'proper' test. I've removed line 242-246 and changed the conditional to "defined(USE_NSS_CERTS) || !defined(OS_IOS)". This is because I cannot test the version of the OS that the device is running, only the min deployement target, with macros. So I either have to disable it completely on iOS or have to do runtime check.
OK, if you can't gate on version, we should just disable the test entirely. I originally had more comments here about how you could clean it up, but rather than hold you up with that, let's just mark the test DISABLED for all platforms (i.e. no MAYBE_, no conditionals), and I'll take on the bug and fix up the test to ensure that, on all platforms, we reject names with embedded NULLs, and fix it up so that our test case avoids any "trusted CA" issues. SG?
On 2016/04/21 at 13:12:57, rsleevi wrote: > OK, if you can't gate on version, we should just disable the test entirely. I originally had more comments here about how you could clean it up, but rather than hold you up with that, let's just mark the test DISABLED for all platforms (i.e. no MAYBE_, no conditionals), and I'll take on the bug and fix up the test to ensure that, on all platforms, we reject names with embedded NULLs, and fix it up so that our test case avoids any "trusted CA" issues. SG? Ack. I've reverted the changes in the test itself, since I was not confident I deleted the "bad" lines. I've left the additional include of build/build_config.h as there are other uses of OS_* macros and this include was missing.
The CQ bit was checked by rsleevi@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1907443005/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1907443005/40001
Description was changed from ========== Disable CertVerifyProcTest.PaypalNullCertParsing on some configurations. The test expectation is incorrect on some configurations, so disable the test until it is fixed (better to have a bug to track a failing test than a false sense of security due to false positive). BUG=605457 ========== to ========== Disable CertVerifyProcTest.PaypalNullCertParsing The test expectation is incorrect on some configurations, so disable the test until it is fixed (better to have a bug to track a failing test than a false sense of security due to false positive). BUG=605457 ==========
Message was sent while issue was closed.
Description was changed from ========== Disable CertVerifyProcTest.PaypalNullCertParsing The test expectation is incorrect on some configurations, so disable the test until it is fixed (better to have a bug to track a failing test than a false sense of security due to false positive). BUG=605457 ========== to ========== Disable CertVerifyProcTest.PaypalNullCertParsing The test expectation is incorrect on some configurations, so disable the test until it is fixed (better to have a bug to track a failing test than a false sense of security due to false positive). BUG=605457 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Disable CertVerifyProcTest.PaypalNullCertParsing The test expectation is incorrect on some configurations, so disable the test until it is fixed (better to have a bug to track a failing test than a false sense of security due to false positive). BUG=605457 ========== to ========== Disable CertVerifyProcTest.PaypalNullCertParsing The test expectation is incorrect on some configurations, so disable the test until it is fixed (better to have a bug to track a failing test than a false sense of security due to false positive). BUG=605457 Committed: https://crrev.com/743c8e4de5881808684cf30e58f83f441010d2c4 Cr-Commit-Position: refs/heads/master@{#388770} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/743c8e4de5881808684cf30e58f83f441010d2c4 Cr-Commit-Position: refs/heads/master@{#388770} |
