1313752 - Remove necko binary tests that aren't being run, and port the rest to gtest

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Benjamin Smedberg]

There are a bunch of binaries in necko/tests. As part of removing the XPCOM glue, I'm trying to port the ones we care about to gtest and stop building the rest.

As it stands, it appears that the only three that are currently running in any kind of automation are TestBind, TestCookie, and TestUDPSocket. I've ported those to gtest (I hope) and removed the rest.

Comment 1

[Benjamin Smedberg]

I have a draft, not ready for review.

Comment 2

[Nicholas Nethercote [inactive]]

FYI: some of these tests are marked as CppUnitTests, not GeckoCppUnitTests, even though they use XPCOM. (E.g. TestBind.cpp, TestUDPSocket.cpp). The reason this works is that netwerk/tests/moz.build also has a GeckoSimplePrograms list, and that causes libxul to be linked in with everything in the directory. (You can check the backend.mk file in the objdir to see what libraries are used for each directory.) Thanks to glandium for explaining this to me yesterday.

Comment 3

[Benjamin Smedberg]

Attachment: Bug 1313752 - Port binary tests in netwerk/test to gtest, or remove the ones that we currently aren't running. .novotny

Review commit: https://reviewboard.mozilla.org/r/90410/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/90410/

Comment 4

[Benjamin Smedberg]

When I run TestUDP by itself, it passes. But when I run it after TestBind, then it fails, here:

https://hg.mozilla.org/try/file/01740174065bb1b0e1f5a88c04d6a7a9e61a14b8/netwerk/test/TestUDPSocket.cpp#l61 because `input` is byte-reversed compared to aExpectedContent.

(This is called from line 177, called from line 291.)

mcmanus you reviewed this file at various points. Can you help me with clues as to why/how TestBind would be interfering with this test? I can't figure out whether the first test has leftover data (seems unlikely), or whether there's some sort of global byte-ordering switch that exists, or something else.

Comment 5

[Patrick McManus [:mcmanus]]

I don't have any good ideas here - but I'm happy to try and debug it for you.

unfortunately I waited so long the patch has significantly bitrotted. If you rebase I'll take a look. sorry again.

Comment 6

[Patrick McManus [:mcmanus]]

I can rebuild from the reviewboard cset.. let me do that

Comment 7

[Patrick McManus [:mcmanus]]

hmm. this is wfm. 

is the repro as simple as:
"TestBind; TestUDP "

?

Comment 8

[Nicholas Nethercote [inactive]]

(In reply to Patrick McManus [:mcmanus] from comment #7)
> hmm. this is wfm. 
> 
> is the repro as simple as:
> "TestBind; TestUDP "

The patch converts a number of standalone C++ unit tests to gtests. All gtests are combined into a single executable, which means it's now possible for different tests to interfere with each other.

The command line you've given makes sense in the old standalone world, but doesn't make sense after conversion to gtests. I suspect you'll need to invoke gtests, with the patch applied, something like this:

> mach gtest '*TestBind*:*TestUDP*'

The final argument is a colon-separated list of globs matching test case names.

(BTW, it might be worth adding a common prefix such as "network_" to all the test case names, so that the '*network*' pattern can be used to run all the network gtests at once.)

Comment 9

[Patrick McManus [:mcmanus]]

thanks nicholas. it fails now - so that's something :)

Comment 10

[Patrick McManus [:mcmanus]]

nick - thanks for reminding me about gtest

benjamin - I think that test has just been racy. This patch WFM

diff --git a/netwerk/test/TestUDPSocket.cpp b/netwerk/test/TestUDPSocket.cpp
--- a/netwerk/test/TestUDPSocket.cpp
+++ b/netwerk/test/TestUDPSocket.cpp
@@ -72,16 +72,18 @@ static bool CheckMessageContent(nsIUDPMe
  * UDPClientListener: listens for incomming UDP packets
  */
 class UDPClientListener : public nsIUDPSocketListener
 {
 protected:
   virtual ~UDPClientListener();
 
 public:
+  UDPClientListener() : mResult(NS_ERROR_FAILURE) {};
+
   NS_DECL_THREADSAFE_ISUPPORTS
   NS_DECL_NSIUDPSOCKETLISTENER
   nsresult mResult;
 };
 
 NS_IMPL_ISUPPORTS(UDPClientListener, nsIUDPSocketListener)
 
 UDPClientListener::~UDPClientListener()
@@ -131,16 +133,18 @@ UDPClientListener::OnStopListening(nsIUD
  * UDPServerListener: listens for incomming UDP packets
  */
 class UDPServerListener : public nsIUDPSocketListener
 {
 protected:
   virtual ~UDPServerListener();
 
 public:
+  UDPServerListener() : mResult(NS_ERROR_FAILURE) {};
+  
   NS_DECL_THREADSAFE_ISUPPORTS
   NS_DECL_NSIUDPSOCKETLISTENER
 
   nsresult mResult;
 };
 
 NS_IMPL_ISUPPORTS(UDPServerListener, nsIUDPSocketListener)
 
@@ -274,16 +278,17 @@ TEST(TestUDPSocket, TestUDPSocketMain)
   ASSERT_TRUE(NS_SUCCEEDED(rv));
   EXPECT_EQ(count, sizeof(uint32_t));
 
   // Wait for server
   PumpEvents();
   ASSERT_TRUE(NS_SUCCEEDED(serverListener->mResult));
 
   // Read response from server
+  PumpEvents();
   ASSERT_TRUE(NS_SUCCEEDED(clientListener->mResult));
 
   mozilla::net::NetAddr clientAddr;
   rv = client->GetAddress(&clientAddr);
   ASSERT_TRUE(NS_SUCCEEDED(rv));
   // The client address is 0.0.0.0, but Windows won't receive packets there, so
   // use 127.0.0.1 explicitly
   clientAddr.inet.ip = PR_htonl(127 << 24 | 1);

Comment 11

[Benjamin Smedberg]

Attachment: Followup to - Scope the netwerk TestCommon waiting to be non-global, and add assertions so that waiting is deterministic

MozReview-Commit-ID: 7jLgNIujrbu

Comment 12

[Patrick McManus [:mcmanus]]

Comment on attachment 8815398 [details] [diff] [review]
Followup to  - Scope the netwerk TestCommon waiting to be non-global, and add assertions so that waiting is deterministic

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

lgtm. thanks

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c72d9641ee84
Port binary tests in netwerk/test to gtest, or remove the ones that we currently aren't running. r=michal.novotny. Includes the following followups:

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3eb899eaa85
Followup to bug 1313752 - remove all the netwerk/test binaries from the XPCOM glue whitelist

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/c72d9641ee84
https://hg.mozilla.org/mozilla-central/rev/a3eb899eaa85

Comment 16

[Jorg K (CEST = GMT+2)]

test_common_init() was removed here:
https://hg.mozilla.org/mozilla-central/rev/c72d9641ee84#l10.12

We fixed Thunderbird here:
https://hg.mozilla.org/comm-central/rev/8cba5e96786d45cea65c17fe3aee91eeba150753