Closed Bug 799246 Opened 7 years ago Closed 7 years ago

Conditionally enable webrtc unit tests

Categories

(Core :: WebRTC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: ekr, Assigned: ekr)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 1 obsolete file)

No description provided.
Attachment #669290 - Attachment description: Conditionally enable webrtc unit tests → 1. Conditionally enable WebRTC tests. 2. Fix Chromium IPC so that logging doesn't get pulled into the test framework.
Attachment #669290 - Flags: review?(rjesup)
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.
Attachment #669290 - Flags: review?(jones.chris.g)
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)?
Attachment #669290 - Flags: review?(ted.mielczarek)
Attachment #669290 - Flags: review?(rjesup)
Attachment #669290 - Flags: review+
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.
Attachment #669290 - Flags: review?(jones.chris.g) → review+
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.
Attachment #669290 - Flags: review?(ted.mielczarek) → review+
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.
Attachment #669290 - Attachment description: 1. Conditionally enable WebRTC tests. 2. Fix Chromium IPC so that logging doesn't get pulled into the test framework. → 1. Conditionally enable WebRTC tests. 2. Fix Chromium IPC so that logging doesn't get pulled into the test framework.
Attachment #669290 - Attachment is obsolete: true
Attachment #669368 - Flags: checkin?(rjesup)
(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)
https://hg.mozilla.org/mozilla-central/rev/ec34a79837f6
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Whiteboard: [qa-]
Attachment #669368 - Flags: checkin?(rjesup)
You need to log in before you can comment on or make changes to this bug.