799246 - Conditionally enable webrtc unit tests

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Comment 1

[Eric Rescorla (:ekr)]

Attachment: 1. Conditionally enable WebRTC tests. 2. Fix Chromium IPC so that logging doesn't get pulled into the test framework.

Comment 2

[Eric Rescorla (:ekr)]

Comment on attachment 669290 [details] [diff] [review]
1. Conditionally enable WebRTC tests.
2. Fix Chromium IPC so that logging doesn't get pulled into the test framework.

Review of attachment 669290 [details] [diff] [review]:
-----------------------------------------------------------------

Cjones: you just need to review logging.h.
Jesup: please review all.

Comment 3

[Eric Rescorla (:ekr)]

https://tbpl.mozilla.org/?tree=Try&rev=5d16149a9fd4

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 669290 [details] [diff] [review]
1. Conditionally enable WebRTC tests.
2. Fix Chromium IPC so that logging doesn't get pulled into the test framework.

Review of attachment 669290 [details] [diff] [review]:
-----------------------------------------------------------------

::: configure.in
@@ +5267,2 @@
>  AC_SUBST(MOZ_WEBRTC)
> +AC_SUBST(MOZ_WEBRTC_TESTS)

Do we need AC_DEFINE(MOZ_WEBRTC_TESTS)?

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 669290 [details] [diff] [review]
1. Conditionally enable WebRTC tests.
2. Fix Chromium IPC so that logging doesn't get pulled into the test framework.

>diff --git a/ipc/chromium/src/base/logging.h b/ipc/chromium/src/base/logging.h

>+#else
>+#include <sstream>
>+

This include need to be moved with other includes at head of file.

Otherwise looks ok.  r=me with that.

Comment 6

[(not currently active) Ted Mielczarek]

Comment on attachment 669290 [details] [diff] [review]
1. Conditionally enable WebRTC tests.
2. Fix Chromium IPC so that logging doesn't get pulled into the test framework.

Review of attachment 669290 [details] [diff] [review]:
-----------------------------------------------------------------

::: configure.in
@@ +5261,5 @@
>  
> +MOZ_ARG_ENABLE_BOOL(webrtc-tests,
> +[  --enable-webrtc-tests   Enable WebRTC unit tests],
> +    MOZ_WEBRTC_TESTS=1,
> +    MOZ_WEBRTC_TESTS=)

Don't put this configure argument in. This isn't something we need people fiddling. You can just stick
MOZ_WEBRTC_TESTS=1
in your mozconfig and it will work fine with just the AC_SUBST.

@@ +5267,2 @@
>  AC_SUBST(MOZ_WEBRTC)
> +AC_SUBST(MOZ_WEBRTC_TESTS)

Don't add the define unless we need it.

::: ipc/chromium/src/base/logging.h
@@ +83,5 @@
>  {
>    return log;
>  }
>  
> +#ifndef NO_CHROMIUM_LOGGING

This is a little confusing "if not defined no chromium logging".

::: toolkit/toolkit-tiers.mk
@@ +293,5 @@
>  tier_platform_dirs += testing/mozbase
>  ifdef MOZ_WEBRTC
> +ifdef MOZ_WEBRTC_TESTS
> +tier_platform_dirs += media/webrtc/signaling/test
> +tier_platform_dirs += media/mtransport/test

I'm not sure why this test does a single assignment per line, you could stick these both in the same assignment if you wanted to.

Comment 7

[Eric Rescorla (:ekr)]

Comment on attachment 669290 [details] [diff] [review]
1. Conditionally enable WebRTC tests.
2. Fix Chromium IPC so that logging doesn't get pulled into the test framework.

Review of attachment 669290 [details] [diff] [review]:
-----------------------------------------------------------------

Cjones: you just need to review logging.h.
Jesup: please review all.

::: configure.in
@@ +5261,5 @@
>  
> +MOZ_ARG_ENABLE_BOOL(webrtc-tests,
> +[  --enable-webrtc-tests   Enable WebRTC unit tests],
> +    MOZ_WEBRTC_TESTS=1,
> +    MOZ_WEBRTC_TESTS=)

Done.

@@ +5267,2 @@
>  AC_SUBST(MOZ_WEBRTC)
> +AC_SUBST(MOZ_WEBRTC_TESTS)

OK.

::: ipc/chromium/src/base/logging.h
@@ +83,5 @@
>  {
>    return log;
>  }
>  
> +#ifndef NO_CHROMIUM_LOGGING

Swapped the order.

@@ +89,5 @@
>  #define LOG_IF(info, condition) \
>    if (!(condition)) mozilla::LogWrapper(mozilla::LOG_ ## info, __FILE__, __LINE__)
> +#else
> +#include <sstream>
> +

Moved this to the top.

::: toolkit/toolkit-tiers.mk
@@ +293,5 @@
>  tier_platform_dirs += testing/mozbase
>  ifdef MOZ_WEBRTC
> +ifdef MOZ_WEBRTC_TESTS
> +tier_platform_dirs += media/webrtc/signaling/test
> +tier_platform_dirs += media/mtransport/test

I am imitating the style of the lines directly above it.

Comment 8

[Eric Rescorla (:ekr)]

Attachment: Conditionally enable webrtc unit tests

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Ted Mielczarek [:ted] from comment #6)
> ::: toolkit/toolkit-tiers.mk
> @@ +293,5 @@
> >  tier_platform_dirs += testing/mozbase
> >  ifdef MOZ_WEBRTC
> > +ifdef MOZ_WEBRTC_TESTS
> > +tier_platform_dirs += media/webrtc/signaling/test
> > +tier_platform_dirs += media/mtransport/test
> 
> I'm not sure why this test does a single assignment per line, you could
> stick these both in the same assignment if you wanted to.

That was largely because they landed in independent patches initially (the big landing queue)

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ec34a79837f6

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ec34a79837f6