Closed Bug 1313752 Opened 3 years ago Closed 3 years ago

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


(Core :: Networking, defect)

Not set



Tracking Status
firefox53 --- fixed


(Reporter: benjamin, Assigned: benjamin)


(Depends on 1 open bug)


(Whiteboard: [necko-active])


(2 files)

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.
I have a draft, not ready for review.
Assignee: nobody → benjamin
Whiteboard: [necko-active]
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/ also has a GeckoSimplePrograms list, and that causes libxul to be linked in with everything in the directory. (You can check the file in the objdir to see what libraries are used for each directory.) Thanks to glandium for explaining this to me yesterday.
No longer blocks: 1299187
Attachment #8807151 - Flags: review?(michal.novotny) → review+
When I run TestUDP by itself, it passes. But when I run it after TestBind, then it fails, here: 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.
Flags: needinfo?(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.
Flags: needinfo?(mcmanus)
I can rebuild from the reviewboard cset.. let me do that
hmm. this is wfm. 

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

(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.)
thanks nicholas. it fails now - so that's something :)
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
   virtual ~UDPClientListener();
+  UDPClientListener() : mResult(NS_ERROR_FAILURE) {};
   nsresult mResult;
 NS_IMPL_ISUPPORTS(UDPClientListener, nsIUDPSocketListener)
@@ -131,16 +133,18 @@ UDPClientListener::OnStopListening(nsIUD
  * UDPServerListener: listens for incomming UDP packets
 class UDPServerListener : public nsIUDPSocketListener
   virtual ~UDPServerListener();
+  UDPServerListener() : mResult(NS_ERROR_FAILURE) {};
   nsresult mResult;
 NS_IMPL_ISUPPORTS(UDPServerListener, nsIUDPSocketListener)
@@ -274,16 +278,17 @@ TEST(TestUDPSocket, TestUDPSocketMain)
   EXPECT_EQ(count, sizeof(uint32_t));
   // Wait for server
   // Read response from server
+  PumpEvents();
   mozilla::net::NetAddr clientAddr;
   rv = client->GetAddress(&clientAddr);
   // The client address is, but Windows won't receive packets there, so
   // use explicitly
   clientAddr.inet.ip = PR_htonl(127 << 24 | 1);
Attachment #8815398 - Flags: review?(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
Attachment #8815398 - Flags: review?(mcmanus) → review+
Pushed by
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:
Pushed by
Followup to bug 1313752 - remove all the netwerk/test binaries from the XPCOM glue whitelist
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Blocks: 1322072
Depends on: 1322377
Depends on: 1351669
You need to log in before you can comment on or make changes to this bug.