Port WebRTC signaling tests to xul gtest (or?)

RESOLVED FIXED

Status

()

Core
WebRTC: Signaling
P1
normal
Rank:
17
RESOLVED FIXED
2 years ago
10 months ago

People

(Reporter: Benjamin Smedberg, Assigned: dminor)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

2 years ago
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)
(Reporter)

Updated

2 years ago
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)
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)
(Reporter)

Comment 4

2 years ago
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.

Comment 6

2 years ago
(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: → bug 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
(Assignee)

Updated

2 years ago
Depends on: 1316886
(Assignee)

Updated

2 years ago
Depends on: 1316888
(Assignee)

Updated

2 years ago
Assignee: nobody → dminor
Status: NEW → ASSIGNED
Rank: 25 → 17
Priority: P2 → P1
(Assignee)

Updated

2 years ago
Depends on: 1317009

Updated

2 years ago
Flags: needinfo?(docfaraday)
There's also bug 862837 in which ehugg looked at converting them to gtests.
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1257713
(Assignee)

Updated

2 years ago
Depends on: 1317714
(Assignee)

Updated

2 years ago
Depends on: 1317726
(transferring the bugs blocked by this from bug 1257713)
Blocks: 1299863, 1219468
(Assignee)

Updated

a year ago
Depends on: 1319489
(Assignee)

Updated

a year ago
Depends on: 1271681
(Assignee)

Updated

a year ago
Depends on: 1271682
(Assignee)

Updated

a year ago
Depends on: 1322707
(Assignee)

Updated

a year ago
No longer depends on: 1271681
(Assignee)

Updated

a year ago
No longer depends on: 1271682
(Assignee)

Updated

a year ago
No longer depends on: 1319489
(Assignee)

Updated

10 months ago
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.