1314514 - gtestify the cubeb unit tests

Status: RESOLVED FIXED

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

Description

[Nicholas Nethercote [inactive]]

The cubeb unit tests use ScopedXPCOM (conditionally, if CUBEB_GECKO_BUILD is
set) but they don't need it.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Remove unnecessary XPCOM dependency from cubeb unit tests

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.

Comment 2

[Matthew Gregan [:kinetik]]

Comment on attachment 8806593 [details] [diff] [review]
Remove unnecessary XPCOM dependency from cubeb unit tests

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

r+

Comment 3

[Matthew Gregan [:kinetik]]

Upstreamed in https://github.com/kinetiknz/cubeb/commit/70497380eb5351f967c3fa52f3eb432969c2e65d.

Comment 4

[Nicholas Nethercote [inactive]]

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.

Comment 5

[Matthew Gregan [:kinetik]]

> 70497380eb5351f967c3fa52f3eb432969c2e65d

I backed this out for now.  Will look at what's needed upstream to for GTest.

Comment 6

[Matthew Gregan [:kinetik]]

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

Comment 7

[Matthew Gregan [:kinetik]]

Attachment: bug1314514.patch

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

Comment 8

[Matthew Gregan [:kinetik]]

Attachment: bug1314514.patch

Fix OS X build with b46ffc5d from upstream.

Comment 9

[Matthew Gregan [:kinetik]]

Above fix, plus more tests enabled (including gtest, whoops!):

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

Comment 10

[Nicholas Nethercote [inactive]]

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.

Comment 11

[Matthew Gregan [:kinetik]]

Attachment: bug1314514.patch

Rename media/libcubeb/tests to gtest.

Comment 12

[Matthew Gregan [:kinetik]]

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?

Comment 13

[Nicholas Nethercote [inactive]]

(Paul, see comment 12)

Comment 14

[Paul Adenot (: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

Comment 15

[Matthew Gregan [:kinetik]]

Attachment: bug1314514.patch

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.

Comment 16

[Matthew Gregan [:kinetik]]

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

Comment 17

[Matthew Gregan [:kinetik]]

Attachment: bug1314514.patch

And with the actual patch rather than the empty try commit.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/359999c77a46
Update libcubeb to 8bab182c.  r=padenot

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This caused build failures like https://treeherder.mozilla.org/logviewer.html#?job_id=39237720&repo=mozilla-inbound

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/ea07c491499006c6650fbd75431a393c4f4d124d

Comment 20

[Matthew Gregan [:kinetik]]

Ah, ALLOWED_XPCOM_GLUE was added after the base cset I was pushing to try.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e66c6309387c
Update libcubeb to 8bab182c.  r=padenot

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e66c6309387c