1316611 - Port WebRTC signaling tests to xul gtest (or?)

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect, P1)

Description

[Benjamin Smedberg]

We are removing the XPCOM glue and XPCOM exported functions. This means that standalone binaries are no longer able to load XPCOM. We are porting most existing GeckoCppUnitTests to use gtest instead.

The tests at media/webrtc/signaling/test/ are not obviously fixable. They are *already* gtests, but they don't compile into xul-gtest.

* The tests modify global state, so they'd need to run separately. It should be possible to do this by marking them as "death tests" even though they aren't.
* These tests appear to mock various classes at compile time. I don't know how to fix that. gmock might have some answers, but I really don't know enough about these tests.

mreavy, can I get some help with this? This currently blocks the xpcomglue build work. If acceptable we could stop building these tests for a while, but I don't know how valuable they are.

Comment 1

[Maire Reavy [:mreavy]]

We do a lot in these tests, especially for signaling and call setup-teardown; these would be hard or impossible to test in mochitests due to limits on what's exposed or inability to set up specific races.  (FYI: there was a lot of pain in getting our cppunit tests created.)  I'm circling in and needinfo'ing Byron, Nils, and Randell who are the real experts and will be most affected by this change.  

Bwc, Drno, Jesup -- Thoughts?

Comment 2

[Nils Ohlmeier [:drno]]

A quick bugzilla search for "signaling unittest" shows the pain.

I think we should think about fixing bugs 1271681 and 1271682 and then re-evaluate what is left.

Comment 3

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

Getting rid of the FakeMediaStream mocks would be a very good thing.  Do we mock anything else significant?  What exactly is blocking conversion, and what impacts would the conversion have?

Turning them off would require a commitment to manual testing of them, and dealing with accumulated breakage periodically. Such manual testing is hard to keep up-to-date in any timely fashion.

Does this bug solely apply to "signaling_unittests", or are sdp and jsep and mediaconduit/etc cppunittests also causing problems?

Comment 4

[Benjamin Smedberg]

I'm talking about all of the GeckoCppUnitTests in webrtc/signaling/test. They won't be able to link against XPCOM any more, and so won't build.

Basically I'd like these to either:

1) not use XPCOM at all and be entirely standalone CppUnitTests
"XPCOM" includes nsCOMPtr.h, logging, NS_WARNING, the component manager, thread loop, etc

2) Use XPCOM and be converted to a more normal xul-gtest style gtest, linked into the big binary

Comment 5

[Nils Ohlmeier [:drno]]

So to properly name things I see the following list of stand alone test getting build in media/webrtc/signaling/test:
 - jsep_session_unittest
 - jsep_track_unittest
 - mediaconduit_unittests
 - mediapipeline_unittest
 - sdp_file_parser
 - sdp_unittests
 - signaling_unittests

So I understand this correctly as these are all listed as GeckoCppUnitTests in the moz.build file you are asking to convert all of these?

If we are talking about all of these I think it is not acceptable to turn all of these off. We would loose a substantial part of our testing coverage.

Some of these test, for example sdp_unittests and sdp_file_parser should be fairly easy to convert. I think the biggest and hardest one in that list is signaling_unittests. And for the signalig_unittest previous discussion happened in bug 1273124.

Comment 6

[Eric Rescorla (:ekr)]

(In reply to Benjamin Smedberg [:bsmedberg] from comment #4)
> I'm talking about all of the GeckoCppUnitTests in webrtc/signaling/test.
> They won't be able to link against XPCOM any more, and so won't build.
> 
> Basically I'd like these to either:
> 
> 1) not use XPCOM at all and be entirely standalone CppUnitTests
> "XPCOM" includes nsCOMPtr.h, logging, NS_WARNING, the component manager,
> thread loop, etc

This doesn't seem practical, because we use lots of XPCOM services.


> 2) Use XPCOM and be converted to a more normal xul-gtest style gtest, linked
> into the big binary

This is probably the way that we will have to go, but frankly it's pretty unfortunate.

Comment 7

[Nicholas Nethercote [inactive]]

erahm already had a stab at this in bug 1257713 but stopped because it was difficult.

Comment 8

[Eric Rahm [:erahm]]

(In reply to Nicholas Nethercote [:njn] from comment #7)
> erahm already had a stab at this in bug 1257713 but stopped because it was
> difficult.

To elaborate:

I didn't finish it because a) one the tests was already broken, and b) the mock stuff. I really think someone from the media team should finish that part or do some sort of pair programming with the person who does it.

Considering it's blocking 3 other bugs at this point it should probably be higher priority.

Comment 9

[(not currently active) Ted Mielczarek]

There's also bug 862837 in which ehugg looked at converting them to gtests.

Comment 11

[Eric Rahm [:erahm]]

(transferring the bugs blocked by this from bug 1257713)