Closed Bug 1313141 Opened 3 years ago Closed 3 years ago

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

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED WORKSFORME

People

(Reporter: benjamin, Assigned: njn)

References

Details

Attachments

(1 obsolete file)

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.
Flags: needinfo?(n.nethercote)
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...
Flags: needinfo?(nfroyd)
Flags: needinfo?(n.nethercote)
Flags: needinfo?(erahm)
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?
Flags: needinfo?(nfroyd) → needinfo?(benjamin)
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.
Flags: needinfo?(benjamin)
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=
Flags: needinfo?(erahm)
(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.
(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)?
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.
(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
(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?
Not a specific one yet. I've been filing bugs attached to 1299187 as I get closer to removing all of those exports.
> 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?
for instance there are things like TestBind.cpp which are currently CppUnitTests, which bind sockets and use the STS.
(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.
(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.
glandium actually I did file xpcom glue removal already, it's bug 1306329
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.
Assignee: nobody → n.nethercote
Depends on: 1313463
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.
Flags: needinfo?(benjamin)
Depends on: 1314514
Depends on: 1314497
Depends on: 1314556
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
Flags: needinfo?(benjamin)
Attached file bug1313141-list.txt (obsolete) —
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.
(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.
Depends on: 1313752, 1314350, 1314378
Depends on: 1314827
Depends on: 1314870
Depends on: 1315138
Depends on: 1315170
Depends on: 1315869
> 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.
Flags: needinfo?(benjamin)
Yes, my tree is currently a windows tree.
Flags: needinfo?(benjamin)
Depends on: 1316611
I filed bug 1316611 about the webrtc/signaling/test issue.
Depends on: 1315561
Depends on: 1316792
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.
Attachment #8806764 - Attachment is obsolete: true
Depends on: 1320861
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.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → WORKSFORME
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.