1313141 - Change the linkage of GeckoCppUnitTests to compile them inside of xul-gtest

Status: RESOLVED WORKSFORME

(Firefox Build System :: General, defect)

Description

[Benjamin Smedberg]

Currently GeckoCppUnitTests compile against the dependent XPCOM glue, each test as an indepdendent binary. Each test is then run separately.

But the XPCOM glue is going away: so what I think we should do here is compile each of these tests into xul-gtest. But unlike gtests, which may not start have global state or start XPCOM, the GeckoCppUnitTests will still run as indendent processes and may start XPCOM if required.

I'd like help with this if there is help available! This is a blocker for bug 1299187.

Comment 1

[Nicholas Nethercote [inactive]]

I'm happy to attempt this, though froydnj and erahm likely know more about this than I do, so any additional info on how to get started would be welcome...

Comment 2

[Nathan Froyd [:froydnj]]

gtests start XPCOM:

http://dxr.mozilla.org/mozilla-central/source/testing/gtest/mozilla/GTestRunner.cpp#90

I wouldn't be at all surprised if they start threads, too:

http://dxr.mozilla.org/mozilla-central/source/xpcom/tests/gtest/TestThreads.cpp#44
http://dxr.mozilla.org/mozilla-central/source/xpcom/tests/gtest/TestThreadPool.cpp#50

I'm not exactly sure how you want them to start as independent processes, but be gtests too...You want to link them against xul-gtest and then...what?

Comment 3

[Benjamin Smedberg]

I don't want them to *be* gtests. I just want them to be built *within* the gtest-xul library.

Then instead of each one having a separate main() method, we'd have some way to start xpcshell -xpcomtest <testname> and have it dispatch to each test.

Comment 4

[Eric Rahm [:erahm]]

Seems like we'd be better served just making these gtests and getting rid of GeckoCppUnitTests entirely, there aren't *that* many [1].

[1] http://searchfox.org/mozilla-central/search?q=GeckoCppUnitTests&path=

Comment 5

[Mike Hommey [:glandium]]

(In reply to Eric Rahm [:erahm] from comment #4)
> Seems like we'd be better served just making these gtests and getting rid of
> GeckoCppUnitTests entirely, there aren't *that* many [1].
> 
> [1] http://searchfox.org/mozilla-central/search?q=GeckoCppUnitTests&path=

There are 65, some of which don't link libxul.

Comment 6

[Mike Hommey [:glandium]]

(In reply to Benjamin Smedberg [:bsmedberg] from comment #0)
> Currently GeckoCppUnitTests compile against the dependent XPCOM glue, each
> test as an indepdendent binary. Each test is then run separately.
> 
> But the XPCOM glue is going away

I'm curious here. I can see the XPCOM dependent glue going away, but that also requires switching plugin-container and several other programs to the XPCOM standalone glue. Can't many of the GeckoCppUnitTests receive the same treatment (use the standalone glue)?

Comment 7

[Benjamin Smedberg]

The standalone glue is also going away, except for a little bit of support code.

erahm, as I understand it gtests aren't allowed to change global state/start XPCOM; and so many of these tests aren't gtests because they need start XPCOM/create XPCOM services or use the event loop.

Comment 8

[Eric Rahm [:erahm]]

(In reply to Benjamin Smedberg [:bsmedberg] from comment #7)
> erahm, as I understand it gtests aren't allowed to change global state/start
> XPCOM; and so many of these tests aren't gtests because they need start
> XPCOM/create XPCOM services or use the event loop.

Our mozillafied-gtest thing definitely starts XPCOM [1] and I'm reasonably sure we have tests using the event loop. I'm not sure about the creating services part, but that seems like it would be okay. If they need to *stop* xpcom that might be an issue, but I'm not sure what the use case for that is.

If certain tests need a pristine state it might be possible to specify that they should be run one at a time (as in launch gtest with the specific test path rather than running all).

[1] http://searchfox.org/mozilla-central/rev/4012e61cc38758ffa1f7ce26039173f67b048853/testing/gtest/mozilla/GTestRunner.cpp#90

Comment 9

[Mike Hommey [:glandium]]

(In reply to Benjamin Smedberg [:bsmedberg] from comment #7)
> The standalone glue is also going away, except for a little bit of support
> code.

Is there a bug # for this?

Comment 10

[Benjamin Smedberg]

Not a specific one yet. I've been filing bugs attached to 1299187 as I get closer to removing all of those exports.

Comment 11

[Benjamin Smedberg]

> Our mozillafied-gtest thing definitely starts XPCOM [1] and I'm reasonably
> sure we have tests using the event loop. I'm not sure about the creating
> services part, but that seems like it would be okay.

Are the tests run single-threaded now, or are they still run on a bunch of worker threads? The thought of having a bunch of tests creating services randomly from various threads fills me with dread. And what would NS_IsMainThread report for tests?

Comment 12

[Benjamin Smedberg]

for instance there are things like TestBind.cpp which are currently CppUnitTests, which bind sockets and use the STS.

Comment 13

[Eric Rahm [:erahm]]

(In reply to Benjamin Smedberg [:bsmedberg] from comment #11)
> > Our mozillafied-gtest thing definitely starts XPCOM [1] and I'm reasonably
> > sure we have tests using the event loop. I'm not sure about the creating
> > services part, but that seems like it would be okay.
> 
> Are the tests run single-threaded now, or are they still run on a bunch of
> worker threads? The thought of having a bunch of tests creating services
> randomly from various threads fills me with dread. And what would
> NS_IsMainThread report for tests?

I believe they're run one at a time, but possibly are started on a child thread. The child thread would be the XPCOM main thread in this case. I'll double check of course.

Comment 14

[Eric Rahm [:erahm]]

(In reply to Eric Rahm [:erahm] from comment #13)
> I believe they're run one at a time, but possibly are started on a child
> thread. The child thread would be the XPCOM main thread in this case. I'll
> double check of course.

Confirmed, NS_IsMainThread() in a moz-gtest returns true.

Comment 15

[Benjamin Smedberg]

glandium actually I did file xpcom glue removal already, it's bug 1306329

Comment 16

[Nicholas Nethercote [inactive]]

I just discussed this with glandium and it sounds like converting these tests to gtests is the best solution -- there might be some tricky ones, but gtests should work for most of them. I will start doing that.

Comment 17

[Nicholas Nethercote [inactive]]

bsmedberg: erahm tells me you may have started converting TestStartupCache.cpp, is that right? Have you done any others? erahm is working on the xpcom/ ones in bug 1313463.

Comment 18

[Benjamin Smedberg]

Yes TestStartupCache is bug 1314378. I saw the XPCOM bugs so I didn't touch them. I did a few others, you can check the dependency tree of bug 1299178. Here's the unowned ones that are left that I know about:

TestCSPParser
TestIsCertBuiltInRoot
test_AsXXX_helpers
test_async_callbacks_with_spun_event_loops
TestSTSParser
test_asyncStatementExecution_transaction
TestCertDB
test_binding_params
test_file_perms
test_IHistory

Comment 19

[Benjamin Smedberg]

Attachment: bug1313141-list.txt

Here is a better list of the remaining tests that need to be converted. With existing conversions in tree and this patch, I can get Firefox to build with no xpcomglue_s library.

Comment 20

[Nicholas Nethercote [inactive]]

(In reply to Benjamin Smedberg [:bsmedberg] from comment #18)
> Yes TestStartupCache is bug 1314378. I saw the XPCOM bugs so I didn't touch
> them. I did a few others, you can check the dependency tree of bug 1299178.

That should be bug 1299187. To clarify things, I've updated the dependencies so that the test-related bugs all block this bug.

> Here's the unowned ones that are left that I know about:
> 
> TestCSPParser
> TestIsCertBuiltInRoot
> test_AsXXX_helpers
> test_async_callbacks_with_spun_event_loops
> TestSTSParser
> test_asyncStatementExecution_transaction
> TestCertDB
> test_binding_params
> test_file_perms
> test_IHistory

Thank you for the list. I'll start chipping away at these today.

Comment 21

[Nicholas Nethercote [inactive]]

> Here is a better list of the remaining tests that need to be converted. With
> existing conversions in tree and this patch, I can get Firefox to build with
> no xpcomglue_s library.

Did you do that test build on Windows? There are some more GeckoCppUnitTests in media/webrtc/signaling/test/ that aren't built on Windows. They look a bit tricky.

Most of them are actually already gtests, but they invoke RUN_ALL_TESTS themselves. Those ones look like they won't be too hard. But mediaconduit_unittests.cpp and signaling_unittests.cpp do some threaded gtest stuff that I don't yet understand, so I'm not sure if they'll be difficult to convert.

Comment 22

[Benjamin Smedberg]

Yes, my tree is currently a windows tree.

Comment 23

[Benjamin Smedberg]

I filed bug 1316611 about the webrtc/signaling/test issue.

Comment 24

[Nicholas Nethercote [inactive]]

Comment on attachment 8806764 [details]
bug1313141-list.txt

All the tests identified in the attached file (plus the WebRTC ones) now have an in-progress blocking bug report on file.

Comment 25

[Benjamin Smedberg]

As a tracking bug this is done! Yay. There's one disabled webrtc test that dmajor is working on, but it doesn't block this any more.