Rework how webcrypto tests are disabled for inprogress OpenSSL implementation.

Rework how webcrypto tests are disabled for inprogress OpenSSL implementation.

Rather than preventing compilation of the test code, mark the test as disabled.

BUG=245025
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=241132

[eroman, 2013-12-06 00:21:43]

Minor change for readabilty.
I can abandon if you disagree.

[Ryan Sleevi, 2013-12-06 19:33:41]

NACK.

If #ifdef approach keeps the code compiled out entirely. Much easier to
reason and refactor that way than the gtest disabled way.
On Dec 5, 2013 4:21 PM, <eroman@chromium.org> wrote:

> Reviewers: Ryan Sleevi,
>
> Message:
> Minor change for readabilty.
> I can abandon if you disagree.
>
> Description:
> Rework how webcrypto tests are disabled for inprogress OpenSSL
> implementation.
>
> Rather than preventing compilation of the test code, mark the test as
> disabled.
>
> BUG=245025
>
> Please review this at https://codereview.chromium.org/106873003/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files (+29, -36 lines):
>   M content/renderer/webcrypto/webcrypto_impl_unittest.cc
>
>
> Index: content/renderer/webcrypto/webcrypto_impl_unittest.cc
> diff --git a/content/renderer/webcrypto/webcrypto_impl_unittest.cc
> b/content/renderer/webcrypto/webcrypto_impl_unittest.cc
> index 48552b76e62bb32a1fe5d0f2b6609a5f9fc13d5f..
> bd003bd8db605086bc3155bcca61eb3b5d651814 100644
> --- a/content/renderer/webcrypto/webcrypto_impl_unittest.cc
> +++ b/content/renderer/webcrypto/webcrypto_impl_unittest.cc
> @@ -22,6 +22,27 @@
>  #include "third_party/WebKit/public/platform/WebCryptoAlgorithmParams.h"
>  #include "third_party/WebKit/public/platform/WebCryptoKey.h"
>
> +
> +// The OpenSSL implementation of WebCrypto is less complete, so don't run
> all of
> +// the tests: http://crbug.com/267888
> +#if defined(USE_OPENSSL)
> +#define AesCbcSampleSets DISABLED_AesCbcSampleSets
> +#define GenerateKeyAes DISABLED_GenerateKeyAes
> +#define GenerateKeyAesBadLength DISABLED_GenerateKeyAesBadLength
> +#define GenerateKeyHmac DISABLED_GenerateKeyHmac
> +#define GenerateKeyHmacNoLength DISABLED_GenerateKeyHmacNoLength
> +#define ImportSecretKeyNoAlgorithm DISABLED_ImportSecretKeyNoAlgorithm
> +#define ImportJwkInputConsistency DISABLED_ImportJwkInputConsistency
> +#define ImportJwkHappy DISABLED_ImportJwkHappy
> +#define ImportExportSpki DISABLED_ImportExportSpki
> +#define ImportPkcs8 DISABLED_ImportPkcs8
> +#define GenerateKeyPairRsa DISABLED_GenerateKeyPairRsa
> +#define RsaEsRoundTrip DISABLED_RsaEsRoundTrip
> +#define RsaEsKnownAnswer DISABLED_RsaEsKnownAnswer
> +#define RsaEsFailures DISABLED_RsaEsFailures
> +#define ImportJwkRsaFailures DISABLED_ImportJwkRsaFailures
> +#endif  // #if defined(USE_OPENSSL)
> +
>  namespace content {
>
>  namespace {
> @@ -60,8 +81,6 @@ void RestoreJwkOctDictionary(base::DictionaryValue*
> dict) {
>    dict->SetString("k", "GADWrMRHwQfoNaXU5fZvTg==");
>  }
>
> -#if !defined(USE_OPENSSL)
> -
>  // Helper for ImportJwkRsaFailures. Restores the JWK JSON
>  // dictionary to a good state
>  void RestoreJwkRsaDictionary(base::DictionaryValue* dict) {
> @@ -77,23 +96,6 @@ void RestoreJwkRsaDictionary(base::DictionaryValue*
> dict) {
>    dict->SetString("e", "AQAB");
>  }
>
> -blink::WebCryptoAlgorithm CreateRsaKeyGenAlgorithm(
> -    blink::WebCryptoAlgorithmId algorithm_id,
> -    unsigned modulus_length,
> -    const std::vector<uint8>& public_exponent) {
> -  DCHECK(algorithm_id == blink::WebCryptoAlgorithmIdRsaEsPkcs1v1_5 ||
> -         algorithm_id == blink::WebCryptoAlgorithmIdRsaSsaPkcs1v1_5 ||
> -         algorithm_id == blink::WebCryptoAlgorithmIdRsaOaep);
> -  return blink::WebCryptoAlgorithm::adoptParamsAndCreate(
> -      algorithm_id,
> -      new blink::WebCryptoRsaKeyGenParams(
> -          modulus_length,
> -          webcrypto::Uint8VectorStart(public_exponent),
> -          public_exponent.size()));
> -}
> -
> -#endif  // #if !defined(USE_OPENSSL)
> -
>  }  // namespace
>
>  class WebCryptoImplTest : public testing::Test {
> @@ -473,8 +475,6 @@ TEST_F(WebCryptoImplTest, HMACSampleSets) {
>    }
>  }
>
> -#if !defined(USE_OPENSSL)
> -
>  TEST_F(WebCryptoImplTest, AesCbcFailures) {
>    blink::WebCryptoKey key = ImportSecretKeyFromRawHexString(
>        "2b7e151628aed2a6abf7158809cf4f3c",
> @@ -722,7 +722,6 @@ TEST_F(WebCryptoImplTest, ImportSecretKeyNoAlgorithm) {
>        &key));
>  }
>
> -#endif  //#if !defined(USE_OPENSSL)
>
>  TEST_F(WebCryptoImplTest, ImportJwkFailures) {
>
> @@ -832,8 +831,6 @@ TEST_F(WebCryptoImplTest, ImportJwkOctFailures) {
>    RestoreJwkOctDictionary(&dict);
>  }
>
> -#if !defined(USE_OPENSSL)
> -
>  TEST_F(WebCryptoImplTest, ImportJwkRsaFailures) {
>
>    base::DictionaryValue dict;
> @@ -890,8 +887,6 @@ TEST_F(WebCryptoImplTest, ImportJwkRsaFailures) {
>    RestoreJwkRsaDictionary(&dict);
>  }
>
> -#endif  // #if !defined(USE_OPENSSL)
> -
>  TEST_F(WebCryptoImplTest, ImportJwkInputConsistency) {
>    // The Web Crypto spec says that if a JWK value is present, but is
>    // inconsistent with the input value, the operation must fail.
> @@ -1043,8 +1038,6 @@ TEST_F(WebCryptoImplTest, ImportJwkHappy) {
>    // TODO(padolph): Import an RSA public key JWK and use it
>  }
>
> -#if !defined(USE_OPENSSL)
> -
>  TEST_F(WebCryptoImplTest, ImportExportSpki) {
>    // openssl genrsa -out pair.pem 2048
>    // openssl rsa -in pair.pem -out pubkey.der -outform DER -pubout
> @@ -1330,9 +1323,10 @@ TEST_F(WebCryptoImplTest, RsaEsRoundTrip) {
>    // Create a key pair.
>    const unsigned kModulusLength = 256;
>    blink::WebCryptoAlgorithm algorithm =
> -      CreateRsaKeyGenAlgorithm(blink::WebCryptoAlgorithmIdRsaEsPkcs1v1_5,
> -                               kModulusLength,
> -                               HexStringToBytes("010001"));
> +      webcrypto::CreateRsaKeyGenAlgorithm(
> +          blink::WebCryptoAlgorithmIdRsaEsPkcs1v1_5,
> +          kModulusLength,
> +          HexStringToBytes("010001"));
>    const blink::WebCryptoKeyUsageMask usage_mask =
>        blink::WebCryptoKeyUsageEncrypt | blink::WebCryptoKeyUsageDecrypt;
>    blink::WebCryptoKey public_key = blink::WebCryptoKey::createNull();
> @@ -1512,9 +1506,10 @@ TEST_F(WebCryptoImplTest, RsaEsFailures) {
>    // Create a key pair.
>    const unsigned kModulusLength = 256;
>    blink::WebCryptoAlgorithm algorithm =
> -      CreateRsaKeyGenAlgorithm(blink::WebCryptoAlgorithmIdRsaEsPkcs1v1_5,
> -                               kModulusLength,
> -                               HexStringToBytes("010001"));
> +      webcrypto::CreateRsaKeyGenAlgorithm(
> +          blink::WebCryptoAlgorithmIdRsaEsPkcs1v1_5,
> +          kModulusLength,
> +          HexStringToBytes("010001"));
>    const blink::WebCryptoKeyUsageMask usage_mask =
>        blink::WebCryptoKeyUsageEncrypt | blink::WebCryptoKeyUsageDecrypt;
>    blink::WebCryptoKey public_key = blink::WebCryptoKey::createNull();
> @@ -1579,6 +1574,4 @@ TEST_F(WebCryptoImplTest, RsaEsFailures) {
>    ExpectArrayBufferMatchesHex(message_hex_str, decrypted_data);
>  }
>
> -#endif  // #if !defined(USE_OPENSSL)
> -
>  }  // namespace content
>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[eroman, 2013-12-06 19:50:52]

I disagree.

The interface being tested is platform-neutral, so compilation is not an issue.

It is currently difficult to shuffle around code or understand which are
disabled on openssl precisely because there are ifdefs spread around the place.

[eroman, 2013-12-06 20:14:36]

Spoke with Ryan about this, and he would tolerate an approach like:

#if defined(USE_OPENSSL)
#define MAYBE_AesCbcSampleSets DISABLED_AesCbcSampleSets
#else
#define MAYBE_AesCbcSampleSets AesCbcSampleSets
#endif

TEST_F(WebCryptoImplTest, MAYBE_AesCbcSampleSets) {
  ...
}

Where the disabling code is beside each test.

Since that doesn't seem like an improvement over what we currently have I'll
abandon this minor refactor.

Paul, if you have an opinion let me know.

[padolph, 2013-12-06 20:14:44]

As a data point, I keep running into problems passing the Android
build because of errors for unused functions called from code that is
#ifdef'ed out for OpenSSL. This requires for example corresponding
#ifdef's around TU-local utility functions, which is getting messy.
Eric's change solves this problem nicely at the expense of compiling
everything.


On Fri, Dec 6, 2013 at 11:50 AM,  <eroman@chromium.org> wrote:
> I disagree.
>
> The interface being tested is platform-neutral, so compilation is not an
> issue.
>
> It is currently difficult to shuffle around code or understand which are
> disabled on openssl precisely because there are ifdefs spread around the
> place.
>
> https://codereview.chromium.org/106873003/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[padolph, 2013-12-06 20:22:14]

Streams crossed.

If we can't do Eric's original proposal, I agree keeping what we have
now is best. We might have to revisit this if the scattered #ifdefs
start to become a bigger problem for refactoring / rebasing.


On Fri, Dec 6, 2013 at 12:14 PM, Paul Adolph <padolph@netflix.com> wrote:
> As a data point, I keep running into problems passing the Android
> build because of errors for unused functions called from code that is
> #ifdef'ed out for OpenSSL. This requires for example corresponding
> #ifdef's around TU-local utility functions, which is getting messy.
> Eric's change solves this problem nicely at the expense of compiling
> everything.
>
>
> On Fri, Dec 6, 2013 at 11:50 AM,  <eroman@chromium.org> wrote:
>> I disagree.
>>
>> The interface being tested is platform-neutral, so compilation is not an
>> issue.
>>
>> It is currently difficult to shuffle around code or understand which are
>> disabled on openssl precisely because there are ifdefs spread around the
>> place.
>>
>> https://codereview.chromium.org/106873003/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[eroman, 2013-12-06 21:43:14]

PTAL.

I have updated the changelist to move disabling knowledge to where the test is
defined. I believe this addresses the initial concern, and is also simpler.

[padolph, 2013-12-06 21:49:32]

On 2013/12/06 21:43:14, eroman wrote:
> PTAL.
> 
> I have updated the changelist to move disabling knowledge to where the test is
> defined. I believe this addresses the initial concern, and is also simpler.

That works for me. Thanks.

[padolph, 2013-12-06 21:50:10]

(manual email)

On 2013/12/06 21:43:14, eroman wrote:
> PTAL.
>
> I have updated the changelist to move disabling knowledge to where the test
is
> defined. I believe this addresses the initial concern, and is also simpler.

That works for me. Thanks.

On Fri, Dec 6, 2013 at 1:43 PM,  <eroman@chromium.org> wrote:
> PTAL.
>
> I have updated the changelist to move disabling knowledge to where the test
> is
> defined. I believe this addresses the initial concern, and is also simpler.
>
> https://codereview.chromium.org/106873003/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[padolph, 2013-12-07 00:48:38]

lgtm

[Ryan Sleevi, 2013-12-08 05:20:47]

https://codereview.chromium.org/106873003/diff/100001/content/renderer/webcry...
File content/renderer/webcrypto/webcrypto_impl_unittest.cc (right):

https://codereview.chromium.org/106873003/diff/100001/content/renderer/webcry...
content/renderer/webcrypto/webcrypto_impl_unittest.cc:31: #endif
Please do not use macros like this.

As we discussed on IM, we talked about the following pattern:

#if defined(USE_NSS)
#define MAYBE_Foo Foo
#else
#define MAYBE_Foo DISABLED_Foo
#endif

This is a pattern that Chrome uses extensively. I don't think it's warranted to
deviate from this, personal opinions on prettiness aside.

[eroman, 2013-12-09 18:11:30]

I don't understand the argument for repeating the test name 4 times...

[eroman, 2013-12-09 18:38:40]

If the concern is that this isn't idiomatic chromium code, I disagree. There are
plenty of places which use a macro to disable tests in exactly this fashion.

Following are some (non-exhaustive) samples.

-----------------------------
security_unittest.cc
-----------------------------

#else
  #define TCMALLOC_TEST(function) DISABLED_##function
#endif

-----------------------------
unit_tests.h
-----------------------------

#if defined(THREAD_SANITIZER)
#define DISABLE_ON_TSAN(test_name) DISABLED_##test_name
#else

-----------------------------
backend_unittest.cc
-----------------------------

#if defined(OS_MACOSX)
#define SIMPLE_MAYBE_MACOS(TestName) DISABLED_ ## TestName
#else

-----------------------------
plugin_browsertest.cc
-----------------------------

#if defined(OS_WIN) && defined(ARCH_CPU_X86_64)
#define MAYBE(x) DISABLED_##x
#else

-----------------------------
ppapi_browsertest.cc
-----------------------------

#if defined(ARCH_CPU_ARM_FAMILY)
#define MAYBE_GLIBC(test_name) DISABLED_##test_name
#else

-----------------------------
nacl_browsertest_util.h
-----------------------------

#if (defined(OS_WIN) && !defined(NDEBUG))
#define MAYBE_PNACL(test_name) DISABLED_##test_name
#else

-----------------------------
media_browsertest.cc
-----------------------------

#if defined(OS_WIN) && defined(ARCH_CPU_X86_64) && _MSC_VER == 1700
#define MAYBE(x) DISABLED_##x
#else

-----------------------------
broker_process_unittest.cc
-----------------------------

#if defined(OS_ANDROID)
  #define DISABLE_ON_ANDROID(function) DISABLED_##function
#else

-----------------------------
firefox_importer_unittest.cc
-----------------------------

#if defined(OS_WIN) && defined(ARCH_CPU_X86_64)
#define MAYBE_NSS(x) DISABLED_##x
#else

-----------------------------
firefox_importer_browsertest.cc
-----------------------------

#if defined(OS_MACOSX) || (defined(OS_WIN) && defined(ARCH_CPU_X86_64))
#define MAYBE_IMPORTER(x) DISABLED_##x
#else

-----------------------------
debugger_apitest.cc
-----------------------------

#if defined(OS_WIN)
#define MAYBE(x) DISABLED_##x
#else

-----------------------------
bookmark_bar_view_test.cc
-----------------------------

#if (defined(OS_WIN) && defined(ARCH_CPU_X86_64)) || \
  (defined(OS_LINUX) && defined(USE_AURA))
#define MAYBE(x) DISABLED_##x
#else

-----------------------------
accessibility_win_browsertest.cc
-----------------------------

#if defined(ARCH_CPU_X86_64)
#define MAYBE(x) DISABLED_##x
#else

-----------------------------
sockets_udp_apitest.cc
-----------------------------

#if defined(OS_WIN) && defined(ARCH_CPU_X86_64)
#define MAYBE(x) DISABLED_##x
#else

-----------------------------
sockets_tcp_apitest.cc
-----------------------------

#if defined(OS_WIN) && defined(ARCH_CPU_X86_64)
#define MAYBE(x) DISABLED_##x
#else

-----------------------------
dump_accessibility_tree_browsertest.cc
-----------------------------

#if defined(OS_WIN) && defined(ARCH_CPU_X86_64)
#define MAYBE(x) DISABLED_##x
#else

-----------------------------
mouse_cursor_monitor_unittest.cc
-----------------------------

#else
#define MAYBE(x) DISABLED_##x
#endif

-----------------------------
sockets_tcp_server_apitest.cc
-----------------------------

#if defined(OS_WIN) && defined(ARCH_CPU_X86_64)
#define MAYBE(x) DISABLED_##x
#else

[eroman, 2013-12-16 21:01:00]

ping

[Ryan Sleevi, 2013-12-16 21:13:24]

On 2013/12/16 21:01:00, eroman wrote:
> ping

Rather than calling the macro NSSONLY, let's go with MAYBE(x) or MAYBE_NSS(x),
on the consistency argument you raised above.

Other than that, LGTM.

[eroman, 2013-12-16 21:31:00]

Done -- renamed to MAYBE()

[commit-bot: I haz the power, 2013-12-16 21:32:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/eroman@chromium.org/106873003/140001

[commit-bot: I haz the power, 2013-12-17 00:07:59]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-12-17 00:18:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/eroman@chromium.org/106873003/140001

[commit-bot: I haz the power, 2013-12-17 03:27:54]

Change committed as 241132