Disable SpdyCredentialBuilder tests with OpenSSL.

Disable SpdyCredentialBuilder tests with OpenSSL.

These tests fail on Linux redux, that uses OpenSSL, due to
ec_private_key_openssl.cc which is not implemented yet.

They were already disabled on Android through an entry in
build/android/gtest_filter/net_unittests_disabled.

TEST=net_unittests
BUG=142833, 140991
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=152163

[felipeg, 2012-08-16 16:18:50]

lgtm

[Ryan Sleevi, 2012-08-16 17:34:04]

http://codereview.chromium.org/10830354/diff/1002/net/spdy/spdy_credential_bu...
File net/spdy/spdy_credential_builder_unittest.cc (right):

http://codereview.chromium.org/10830354/diff/1002/net/spdy/spdy_credential_bu...
net/spdy/spdy_credential_builder_unittest.cc:79: // OpenSSL due to the
unimplemented ec_private_key_openssl.cc.
Is there a reason you're using OS_LINUX here? Android also uses OpenSSL, and
also is unimplemented there.

Also, the Chromium style guide recommends MAYBE_ as the prefix here -
http://www.chromium.org/developers/tree-sheriffs/sheriff-details-chromium/han...

Yes, this means duplicating the #ifdef between the code, but this is the
existing pattern in Chromium code.

[Philippe, 2012-08-17 09:10:34]

http://codereview.chromium.org/10830354/diff/1002/net/spdy/spdy_credential_bu...
File net/spdy/spdy_credential_builder_unittest.cc (right):

http://codereview.chromium.org/10830354/diff/1002/net/spdy/spdy_credential_bu...
net/spdy/spdy_credential_builder_unittest.cc:79: // OpenSSL due to the
unimplemented ec_private_key_openssl.cc.
On 2012/08/16 17:34:05, Ryan Sleevi wrote:
> Is there a reason you're using OS_LINUX here? Android also uses OpenSSL, and
> also is unimplemented there.
> 
> Also, the Chromium style guide recommends MAYBE_ as the prefix here -
>
http://www.chromium.org/developers/tree-sheriffs/sheriff-details-chromium/han...
> 
> Yes, this means duplicating the #ifdef between the code, but this is the
> existing pattern in Chromium code.

Android had its own way of disabling this test through an entry in
build/android/gtest_filter/net_unittests_disabled. I removed it from there and
made MAYBE_ depend only on USE_OPENSSL here.

I also updated the code by following the link you pointed out.

Also, would it be reasonable to keep the old approach by renaming
DISABLED_ON_LINUX_REDUX() to MAYBE() (although this could lead to name clashes)
to avoid duplicating all this code?

[Ryan Sleevi, 2012-08-17 17:17:25]

lgtm

http://codereview.chromium.org/10830354/diff/1002/net/spdy/spdy_credential_bu...
File net/spdy/spdy_credential_builder_unittest.cc (right):

http://codereview.chromium.org/10830354/diff/1002/net/spdy/spdy_credential_bu...
net/spdy/spdy_credential_builder_unittest.cc:79: // OpenSSL due to the
unimplemented ec_private_key_openssl.cc.
On 2012/08/17 09:10:34, Philippe wrote:
> On 2012/08/16 17:34:05, Ryan Sleevi wrote:
> > Is there a reason you're using OS_LINUX here? Android also uses OpenSSL, and
> > also is unimplemented there.
> > 
> > Also, the Chromium style guide recommends MAYBE_ as the prefix here -
> >
>
http://www.chromium.org/developers/tree-sheriffs/sheriff-details-chromium/han...
> > 
> > Yes, this means duplicating the #ifdef between the code, but this is the
> > existing pattern in Chromium code.
> 
> Android had its own way of disabling this test through an entry in
> build/android/gtest_filter/net_unittests_disabled. I removed it from there and
> made MAYBE_ depend only on USE_OPENSSL here.
> 
> I also updated the code by following the link you pointed out.
> 
> Also, would it be reasonable to keep the old approach by renaming
> DISABLED_ON_LINUX_REDUX() to MAYBE() (although this could lead to name
clashes)
> to avoid duplicating all this code?

The use of macro functions tends to harm readability - because it adds an extra
layer of indirection that has to be followed by the caller. This is especially
true with stringizing macros, since it can be hard to find the final string.

I think how you've written it is fine, and makes it clear at each test site
which tests are disabled on which platforms. This is also important because it
still leaves the possibility that individual tests can be disabled on other
platforms as necessary.

[commit-bot: I haz the power, 2012-08-17 17:17:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pliard@chromium.org/10830354/1003

[commit-bot: I haz the power, 2012-08-17 22:33:55]

Change committed as 152163