Closed Bug 811118 Opened 7 years ago Closed 7 years ago

Webrtc Unittests should be built by default

Categories

(Core :: WebRTC, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: ehugg, Assigned: ehugg)

References

Details

(Whiteboard: [WebRTC][blocking-webrtc+][qa-])

Attachments

(1 file, 2 obsolete files)

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 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
Attachment #680837 - Flags: review?(rjesup)
Assignee: nobody → ethanhugg
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.
(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 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.
Attachment #680837 - Flags: review?(rjesup) → review+
(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.
(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.
Priority: -- → P1
Whiteboard: [WebRTC][blocking-webrtc+]
(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?
Attachment #680837 - Attachment is obsolete: true
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.
Attachment #682563 - Flags: review?(rjesup)
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
Attachment #682563 - Flags: review?(rjesup) → review+
Try is underway already for all targets:

https://tbpl.mozilla.org/?tree=Try&rev=66df01e149bd
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
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.
Attachment #682563 - Attachment is obsolete: true
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.
Attachment #683241 - Flags: review?(rjesup)
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
Attachment #683241 - Flags: review?(rjesup) → review+
Depends on: 813212
https://hg.mozilla.org/mozilla-central/rev/31d01eb0baf3
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Whiteboard: [WebRTC][blocking-webrtc+] → [WebRTC][blocking-webrtc+][qa-]
Depends on: 815916
You need to log in before you can comment on or make changes to this bug.