Convert signaling unit tests to gecko gtests

RESOLVED DUPLICATE of bug 1316611

Status

()

P1
normal
Rank:
17
RESOLVED DUPLICATE of bug 1316611
3 years ago
2 years ago

People

(Reporter: erahm, Unassigned)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

Similar to what we did in bug 1239870 we should convert the signaling unit tests to gecko gtests. This will allow us to remove various "fake" implementations as well as the few remaining references to |PR_LogModuleInfo|.

We can probably just reuse the MtransportTest [1] base class to handle various bits of initialization.

I think it will also allow us to get rid of the |mtransport_s| lib [2].

[1] https://dxr.mozilla.org/mozilla-central/rev/3e04659fdf6aef792f7cf9840189c6c38d08d1e8/media/mtransport/test/gtest_utils.h#103
[2] https://dxr.mozilla.org/mozilla-central/rev/3e04659fdf6aef792f7cf9840189c6c38d08d1e8/media/mtransport/testlib/moz.build
Assignee: nobody → erahm
Okay, so it looks like mediaconduit_unittests is currently busted, I'll convert it, but I can't verify I didn't break anything:

> [erahm@mozilla21268 mozilla-central]$ MOZ_WEBRTC_MEDIACONDUIT_TESTS=1 ./mach cppunitests obj-x86_64-unknown-linux-gnu-clang/dist/bin/mediaconduit_unittests 
> We're assuming the 'cppunitests' command is 'cppunittest' and we're executing it for you.
> 
> SUITE-START | Running 1 tests
> TEST-START | mediaconduit_unittests
> PROCESS | 30126 | 
> Assertion failure: ((bool)(__builtin_expect(!!(!NS_FAILED_impl(rv)), 1))), at /home/erahm/dev/mozilla-central/media/mtransport/test/mtransport_test_utils.h:31
> TEST-UNEXPECTED-FAIL | mediaconduit_unittests | test failed with return code -139
> TEST-INFO took 117ms
> SUITE-END | took 0s
> Result summary:
> cppunittests INFO | Passed: 0
> cppunittests INFO | Failed: 1
Rank: 25
Priority: -- → P2
mediapipeline_unittest uses "FakeMediaStreams", I can't just swap that out as this seems to have drifted from the current DOM API. Leaving it as-is (using a fake impl) doesn't work either as there are some symbol conflicts for redefining MSG etc.

I can update it to be a gecko gtest, but it won't actually build.
Created attachment 8735010 [details] [diff] [review]
Convert signaling tests to gecko gtests
Created attachment 8735011 [details] [diff] [review]
WIP Pt 1: Convert mediapipline unit test

This is the initial work to get the mediapipline test converted. Some work will
have to be done to figure out what to do with the FakeMediaStream interfaces
that have diverged from the non-fake interfaces.
Created attachment 8735012 [details] [diff] [review]
WIP Pt 2: Convert the signaling unit test

This is the initial work to get the siginaling test converted. Some work will
have to be done to figure out what to do with the FakeMediaStream interfaces
that have diverged from the non-fake interfaces.
Alright I've taken this about as far as I can. The mediapipline and signaling unit tests remain partially converted. They both use some sort of pseudo-fake-mock impls that have diverged from their DOM counterparts. Someone with more media knowledge would need to convert those.

Byron, it looks like you win the touched those files the most award, any chance you could take on the final part or give rather concrete pointers on how to fix the fake impl situation?
Assignee: erahm → nobody
Flags: needinfo?(docfaraday)
Looking into it.
Flags: needinfo?(docfaraday)
So, I know as much as you do about the fake media stream stuff. It might be better to ping someone over on the media side of things to give some pointers. They're more likely to know stuff like how to get a media source without using the mock stuff.
Flags: needinfo?(rjesup)
See Also: → bug 1316611
If these will be linked in with main XUL/etc, then we can just drop the fake impls and use real mediastreams.  We used the fake impls because of (IIRC) linkage issues.  

Using real impls likely will require some fixes to the tests, which may be what erahm ran into.  We should also be able to turn on more-real video in some of the tests.
Flags: needinfo?(rjesup)
Rank: 25 → 17
Priority: P2 → P1
Forward duping this to Bug 1316611. I'm planning to tackle the tests one or two at a time as dependencies of that bug rather than trying to fix everything in one go. The work in progress here will be helpful for that.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1316611
You need to log in before you can comment on or make changes to this bug.