Closed Bug 1314514 Opened 8 years ago Closed 8 years ago

gtestify the cubeb unit tests

Categories

(Core :: Audio/Video: cubeb, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: n.nethercote, Assigned: kinetik)

References

Details

(Whiteboard: [tor 23230])

Attachments

(1 file, 5 obsolete files)

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
Attached patch bug1314514.patch (obsolete) (deleted) — 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
Attached patch bug1314514.patch (obsolete) (deleted) — Splinter Review
Fix OS X build with b46ffc5d from upstream.
Attachment #8809674 - Attachment is obsolete: true
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.
Attached patch bug1314514.patch (obsolete) (deleted) — 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)
Attached patch bug1314514.patch (obsolete) (deleted) — 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)
Attached patch bug1314514.patch (deleted) — Splinter Review
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)
Status: ASSIGNED → RESOLVED
Closed: 8 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.

Attachment

General

Created:
Updated:
Size: