gtestify the cubeb unit tests

RESOLVED FIXED in Firefox 53

Status

()

defect
P1
normal
Rank:
15
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: njn, Assigned: kinetik)

Tracking

unspecified
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

(Whiteboard: [tor 23230])

Attachments

(1 attachment, 5 obsolete attachments)

The cubeb unit tests use ScopedXPCOM (conditionally, if CUBEB_GECKO_BUILD is
set) but they don't need it.
This patch removes it the ScopedXPCOM usage, which allows the tests to be
changed from GeckoCppUnitTests to CppUnitTests.

With this change, the only remaining use of CUBEB_GECKO_BUILD is in
media/libcubeb/src/cubeb_resampler_internal.h.
Attachment #8806593 - Flags: review?(kinetik)
Comment on attachment 8806593 [details] [diff] [review]
Remove unnecessary XPCOM dependency from cubeb unit tests

Review of attachment 8806593 [details] [diff] [review]:
-----------------------------------------------------------------

r+
Attachment #8806593 - Flags: review?(kinetik) → review+
Rank: 15
Priority: -- → P1
This is harder than it first looked. The attached patch works on Linux, but fails to build on Mac, Windows and Android:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=de81eb898308

Adding |USE_LIBS += ['xul']| fixed the problem on Mac but caused the tests to segfault on Linux. Converting these to gtests might be necessary.
> 70497380eb5351f967c3fa52f3eb432969c2e65d

I backed this out for now.  Will look at what's needed upstream to for GTest.
Pull request #189 upstream converts the tests to gtests and includes njn's removal of the XPCOM deps.

https://github.com/kinetiknz/cubeb/pull/189
Assignee: n.nethercote → kinetik
Posted patch bug1314514.patch (obsolete) — Splinter Review
Updates in-tree libcubeb to e1da8042.  This picks up the test conversions to gtests and subsequent minor tweaks to integrate correctly with Gecko, plus:

7ac08e0 Make API visible cubeb_device_info members and cubeb_devid const.
5abde56 Avoid using cubeb_devid internally.
37ce70d Bail out safely from the rendering loop if we could not join the rendering thread in time (#187)

The last item is bug 1315194.

Waiting on try results before requesting review:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bca00ab2bb1a0d438eb3a2315574bb4cb6a4dcae
Attachment #8806593 - Attachment is obsolete: true
Blocks: 1315194
Posted patch bug1314514.patch (obsolete) — Splinter Review
Fix OS X build with b46ffc5d from upstream.
Attachment #8809674 - Attachment is obsolete: true
Above fix, plus more tests enabled (including gtest, whoops!):

https://treeherder.mozilla.org/#/jobs?repo=try&revision=52fc9aab55249af421309baba93a35c6a2e543a7
Thank you for doing this.

One small nit: gtests are usually put in a directory called "gtest". I suggest renaming media/libcubeb/tests as media/libcubeb/gtest.
Posted patch bug1314514.patch (obsolete) — Splinter Review
Rename media/libcubeb/tests to gtest.
Attachment #8809692 - Attachment is obsolete: true
cubeb.duplex and cubeb.record are failing because they expect to capture sound from a device.

I guess these were previously running in a test context where a dummy capture device was available?  Paul, do you know anything about this?
(Paul, see comment 12)
Flags: needinfo?(padenot)
(In reply to Matthew Gregan [:kinetik] from comment #12)
> cubeb.duplex and cubeb.record are failing because they expect to capture
> sound from a device.
> 
> I guess these were previously running in a test context where a dummy
> capture device was available?  Paul, do you know anything about this?

I learned very recently that some time ago, changes were made to the way one should indicate that a `cppunittest` should be ran during the `cpp` chunk. It is now necessary to list it in [0]. Since we haven't been informed, and because the list has been made in October 2014, we're not running the unit-tests that use input (cubeb was not capable of doing input at the time).

When I discovered this (randomly by trying to enable more cubeb tests on try and not seeing them in the log), I've been pushing to try with more tests enabled [2]. However because of a wrong base revision and a bug in trychooser (that prevents executing cppunittests on their own), it errored and went back in my queue.

Sad state of affair. However there is some light, I've been researching ways to test cubeb better and I found ways to do a lots of things on OSX and Windows. I'll open issues upstream.

[1]: http://searchfox.org/mozilla-central/source/testing/cppunittest.ini
[2]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0dfdfd5da55fa4bb626194c4bf872f0a01594de9
Flags: needinfo?(padenot)
Posted patch bug1314514.patch (obsolete) — Splinter Review
Thanks! I wasn't aware of cppunittest.ini.  I've removed the old tests from that now.

So the easy solution for now is to disable those tests in the gtests as well.

Also updated the libcubeb import to latest master to pick up the PulseAudio crash fix and AudioUnit race fix.

And renamed tests to gtest (and remembered to update the commit) this time.

This should be ready to land, assuming Try looks good.
Attachment #8809699 - Attachment is obsolete: true
Attachment #8810669 - Flags: review?(padenot)
And with the actual patch rather than the empty try commit.
Attachment #8810669 - Attachment is obsolete: true
Attachment #8810669 - Flags: review?(padenot)
Attachment #8810673 - Flags: review?(padenot)
Attachment #8810673 - Flags: review?(padenot) → review+
Ah, ALLOWED_XPCOM_GLUE was added after the base cset I was pushing to try.
Flags: needinfo?(kinetik)
https://hg.mozilla.org/mozilla-central/rev/e66c6309387c
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Summary: Remove unnecessary XPCOM dependency from cubeb unit tests → gtestify the cubeb unit tests
Whiteboard: [tor 23230]
You need to log in before you can comment on or make changes to this bug.