Closed Bug 1316611 Opened 3 years ago Closed 2 years ago

Port WebRTC signaling tests to xul gtest (or?)

Categories

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

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: benjamin, Assigned: dminor)

References

Details

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.
Flags: needinfo?(mreavy)
Blocks: 1313141
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?
Flags: needinfo?(rjesup)
Flags: needinfo?(mreavy)
Flags: needinfo?(drno)
Flags: needinfo?(docfaraday)
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.
Flags: needinfo?(drno)
See Also: → 1273124, 1271682, 1271681
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?
Flags: needinfo?(rjesup) → needinfo?(benjamin)
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
Flags: needinfo?(benjamin)
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.
(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.
erahm already had a stab at this in bug 1257713 but stopped because it was difficult.
See Also: → 1257713
(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.
Rank: 25
Priority: -- → P2
Depends on: 1316886
Depends on: 1316888
Assignee: nobody → dminor
Status: NEW → ASSIGNED
Rank: 25 → 17
Priority: P2 → P1
Depends on: 1317009
Flags: needinfo?(docfaraday)
There's also bug 862837 in which ehugg looked at converting them to gtests.
Duplicate of this bug: 1257713
Depends on: 1317714
Depends on: 1317726
(transferring the bugs blocked by this from bug 1257713)
Blocks: 1299863, 1219468
Depends on: 1319489
Depends on: 1271681
Depends on: 1271682
Depends on: 1322707
No longer depends on: 1271681
No longer depends on: 1271682
No longer depends on: 1319489
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.