811118 - Webrtc Unittests should be built by default

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P1)

Description

[Ethan Hugg [:ehugg]]

Currently the webrtc unittests are not being built on M-C unless MOZ_WEBRTC_TESTS is set.  This is because they don't always run correctly on the builders.  Evidently the build setup does not support unittests being built but not run as part of the build.  This had the danger of people breaking the build of the unittests with their checkins.

I propose we make the unittests exit(0) unless MOZ_WEBRTC_TESTS is set.  This way they can be built all the time, but only run when needed.  This can be taken out of the unittests one at a time when they are ready to be run by default.

Comment 1

[Ethan Hugg [:ehugg]]

Attachment: build webrtc unittests by default but run them conditionally

Comment 2

[Ethan Hugg [:ehugg]]

Comment on attachment 680837 [details] [diff] [review]
build webrtc unittests by default but run them conditionally


Here's my suggestion.  Try is running here - 
https://tbpl.mozilla.org/?tree=Try&rev=3f860a8fa7ea

Comment 3

[Eric Rescorla (:ekr)]

Comment on attachment 680837 [details] [diff] [review]
build webrtc unittests by default but run them conditionally

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

::: media/mtransport/test/mtransport_test_utils.h
@@ +95,5 @@
> +  char *test_flag = getenv(envname); \
> +  if (!test_flag || strcmp(test_flag, "1")) { \
> +    printf("To run this test set %s=1 in your environment\n", envname); \
> +    exit(0); \
> +  } \

Why not just have this called in the ctor and then we could avoid modifying the tests.

My goal here is to revert this as soon as all the tests work, so the fewer changes the better.

Comment 4

[Ethan Hugg [:ehugg]]

(In reply to Eric Rescorla from comment #3)
> Comment on attachment 680837 [details] [diff] [review]
> build webrtc unittests by default but run them conditionally
> 
> Review of attachment 680837 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/mtransport/test/mtransport_test_utils.h
> @@ +95,5 @@
> > +  char *test_flag = getenv(envname); \
> > +  if (!test_flag || strcmp(test_flag, "1")) { \
> > +    printf("To run this test set %s=1 in your environment\n", envname); \
> > +    exit(0); \
> > +  } \
> 
> Why not just have this called in the ctor and then we could avoid modifying
> the tests.
> 
> My goal here is to revert this as soon as all the tests work, so the fewer
> changes the better.

I think we need the ability to turn them on to run be default one at a time, otherwise we're waiting for all of them to work solidly before turning any of them on.  Some could be turned on now I suppose, I just don't know yet which ones.

Comment 5

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

Comment on attachment 680837 [details] [diff] [review]
build webrtc unittests by default but run them conditionally

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

r+, though you should consider ekr's comment and perhaps switch if it's not a problem.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Eric Rescorla from comment #3)
> My goal here is to revert this as soon as all the tests work, so the fewer
> changes the better.

Do we have a bug which covers that? It sounds important to me to get this working correctly.

(In reply to Ethan Hugg [:ehugg] from comment #4)
> I think we need the ability to turn them on to run be default one at a time,
> otherwise we're waiting for all of them to work solidly before turning any
> of them on.  Some could be turned on now I suppose, I just don't know yet
> which ones.

I kinda would like to know what the current failure rate is at the moment. About how many failing unit tests are we talking? Again, is there a tracking bug? We should work on getting this fixed soonish. Having tests which are unreliable don't proof the quality of the code but let us introduce even more regressions.

I'm absolutely against hiding test results but for fixing the current brokenness. So please reply to me as soon as possible. Thanks.

Comment 7

[Eric Rescorla (:ekr)]

(In reply to Henrik Skupin (:whimboo) from comment #6)
> (In reply to Eric Rescorla from comment #3)
> > My goal here is to revert this as soon as all the tests work, so the fewer
> > changes the better.
> 
> Do we have a bug which covers that? It sounds important to me to get this
> working correctly.
> 
> (In reply to Ethan Hugg [:ehugg] from comment #4)
> > I think we need the ability to turn them on to run be default one at a time,
> > otherwise we're waiting for all of them to work solidly before turning any
> > of them on.  Some could be turned on now I suppose, I just don't know yet
> > which ones.
> 
> I kinda would like to know what the current failure rate is at the moment.

One test fails reliably on Mac. I've been waiting for a crash dump from Ted which
I now have.

One test fails on Linux but it looks to me like it's just a platform/ALSA issue.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Eric Rescorla from comment #7)
> One test fails reliably on Mac. I've been waiting for a crash dump from Ted
> which
> I now have.
> 
> One test fails on Linux but it looks to me like it's just a platform/ALSA
> issue.

Can you get both failures filed as bugs please?

Comment 9

[Ethan Hugg [:ehugg]]

Attachment: build webrtc unittests by default but run them conditionally

Comment 10

[Ethan Hugg [:ehugg]]

Comment on attachment 682563 [details] [diff] [review]
build webrtc unittests by default but run them conditionally

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

Changed this patch so it turns all unittests by default on with the exception of the mediaconduit_unittest on Linux builds.

Comment 11

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

Comment on attachment 682563 [details] [diff] [review]
build webrtc unittests by default but run them conditionally

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

Looks good.  Have we done a try run with the earlier version of this?  If not, maybe probably just -b do -p linux,linux64,win32,macosx64 -u none -t none

The sheriffs are dealing with a lot of backouts today...  Don't want to be one of them :-)

::: media/webrtc/signaling/test/mediaconduit_unittests.cpp
@@ +749,5 @@
>  
>  int main(int argc, char **argv)
>  {
> +#ifdef LINUX
> +  // On Linux this needs to be conditinal since the builders

conditional

Comment 12

[Ethan Hugg [:ehugg]]

Try is underway already for all targets:

https://tbpl.mozilla.org/?tree=Try&rev=66df01e149bd

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Oddly enough, this originally landed on inbound without any apparent problems, but then started failing. Backed out for now.
https://hg.mozilla.org/integration/mozilla-inbound/rev/84b97c1c0bfa

https://tbpl.mozilla.org/php/getParsedLog.php?id=17122324&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=17122187&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=17122368&tree=Mozilla-Inbound

Comment 14

[Ethan Hugg [:ehugg]]

Orange List - Orange Bs from 12 pushes total.

signaling_unittests:
segfault on linux opt - https://tbpl.mozilla.org/php/getParsedLog.php?id=17124391&tree=Mozilla-Inbound
segfault on linux debug - https://tbpl.mozilla.org/php/getParsedLog.php?id=17124106&tree=Mozilla-Inbound

mediaconduit_unittests:
OS X 10.7 debug - incorrect checksum for freed object - https://tbpl.mozilla.org/php/getParsedLog.php?id=17123895&tree=Mozilla-Inbound
OS X 10.7 opt - EXC_BAD_ACCESS / KERN_PROTECTION_FAILURE - https://tbpl.mozilla.org/php/getParsedLog.php?id=17122324&tree=Mozilla-Inbound
OS X 10.7 opt - EXC_BAD_ACCESS / KERN_PROTECTION_FAILURE - https://tbpl.mozilla.org/php/getParsedLog.php?id=17122187&tree=Mozilla-Inbound

ice_unittest:
segfault on linux debug - https://tbpl.mozilla.org/php/getParsedLog.php?id=17122368&tree=Mozilla-Inbound

Comment 15

[Ethan Hugg [:ehugg]]

I suggest that I change this patch to turn off runtime of mediaconduit and signaling unittests for all platforms so we can get them building.

Not sure about the ice test though, that one was unexpected.

Comment 16

[Ethan Hugg [:ehugg]]

Attachment: build webrtc unittests by default but run them conditionally

Comment 17

[Ethan Hugg [:ehugg]]

Comment on attachment 683241 [details] [diff] [review]
build webrtc unittests by default but run them conditionally


With this patch the builders will not attempt to run mediaconduit or signaling unittests.  This assumes the orange we got in the ice unittests is fixes elsewhere.

Comment 18

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

Comment on attachment 683241 [details] [diff] [review]
build webrtc unittests by default but run them conditionally

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

let's try it

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/31d01eb0baf3