Closed Bug 1239870 Opened 4 years ago Closed 4 years ago

Convert mtransport CppUnitTests to mozilla gtests

Categories

(Core :: WebRTC: Networking, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox46 --- affected
firefox47 --- fixed
Blocking Flags:

People

(Reporter: erahm, Assigned: erahm)

References

Details

Attachments

(6 files, 8 obsolete files)

247.25 KB, text/x-log
Details
1.21 KB, patch
bwc
: review+
Details | Diff | Splinter Review
3.79 KB, patch
bwc
: review+
Details | Diff | Splinter Review
3.41 KB, patch
bwc
: review+
Details | Diff | Splinter Review
4.48 KB, patch
bwc
: review+
Details | Diff | Splinter Review
123.75 KB, patch
bwc
: review+
Details | Diff | Splinter Review
In order to remove libxpcomrt we need to convert the mtransport unit tests over to GeckoCppUnitTests.
Alright, more likely we'll want to convert these to mozilla gtests (so they get built in with XUL and get access to all the private things they need). That's a bit more involved:

- Remove each main function, move functionality elsewhere (probably a base fixture)
- Move various global test helpers to a fixture (not 100% sure we need this)
- Remove duplicate symbols (again possibly moving to a fixture)
- Update build file to something like xpcom/tests/gtest/moz.build
Summary: Convert mtransport CppUnitTests to GeckoCppUnitTests → Convert mtransport CppUnitTests to mozilla gtests
backlog: --- → webrtc/webaudio+
Rank: 25
Priority: -- → P2
Once files are all compiled into the same gtest this will cause duplicate
symbol errors. It's also unused.
Assignee: nobody → erahm
Status: NEW → ASSIGNED
This will fix duplicate symbol errors when switching over to a single gtest
executable.
Currently this is a WIP, more cleanup and testing to come.

This converts the individual cppunit gtests into the combined mozilla gtest
which has access to xpcom internals. The build file is simplified to reflect
this change, individual main functions are removed, and duplicate symbols are
removed.
Comment on attachment 8721016 [details] [diff] [review]
WIP: Part 3: Switch over mtransport tests to mozilla gtests

Nils, can you check if the TURN test is still working with the env vars set? I've confirmed it still compiles and runs on Linux, but that was without a TURN server configured.

To run you would execute:
> ./mach gtest TurnClient.*
Attachment #8721016 - Flags: feedback?(drno)
Looks like your patch is breaking something... looking into it
Comment on attachment 8721016 [details] [diff] [review]
WIP: Part 3: Switch over mtransport tests to mozilla gtests

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

Don't mean to discourage with f-, just pointing out that it does not work (yet).

I'll send you credentials to my own test TURN server so you continue with less dependencies.

::: media/mtransport/test/turn_unittest.cpp
@@ +101,5 @@
>          net_fd_(nullptr),
>          turn_ctx_(nullptr),
>          allocated_(false),
>          received_(0),
>          protocol_(IPPROTO_UDP) {

How much I dislike splinter :-)

You moved the code from line 134-137 into your gtest_utils.h. Now you need to remove these three lines, because they currently fail.

@@ +497,5 @@
> +//  RUN_ON_THREAD(test_utils->sts_target(),
> +//                WrapRunnableNM(&NrIceCtx::Create,
> +//                               dummy, false, false, false, false, false,
> +//                               NrIceCtx::ICE_POLICY_ALL),
> +//                NS_DISPATCH_SYNC);

This is vital to the test. With the above mentioned modification the test fails with the error message "Need to define crypto API implementation". Which I think is caused by the missing NrIceCtx::Create call.
Attachment #8721016 - Flags: feedback?(drno) → feedback-
This fixes support for the TURN test, fixes signalling cppunittests. All gtests work locally except |ice_unittest| which appears to already have been broken (its currently left unconverted).

I'm seeing an assertion on try which doesn't make much sense as to why I don't see this locally:

> 12:55:44     INFO -  Assertion failure: mod != NULL, at pk11slot.c:1798
> 12:55:44     INFO -  Redirecting call to abort() to mozalloc_abort
> 12:55:44     INFO -  Hit MOZ_CRASH() at /builds/slave/try-lx-d-000000000000000000000/build/src/memory/mozalloc/mozalloc_abort.cpp:33
> 12:55:44     INFO -  ExceptionHandler::GenerateDump cloned child 12337
> 12:55:44     INFO -  ExceptionHandler::SendContinueSignalToChild sent continue signal to child
> 12:55:44     INFO -  ExceptionHandler::WaitForContinueSignal waiting for continue signal...
> 12:55:44     INFO -  mozcrash INFO | Copy/paste: /builds/slave/test/build/linux32-minidump_stackwalk /builds/slave/test/build/tests/gtest/4075f36f-8498-a1b9-6da3e7d3-46103884.dmp /builds/slave/test/build/symbols
> 12:56:01     INFO -  mozcrash INFO | Saved minidump as /builds/slave/test/build/blobber_upload_dir/4075f36f-8498-a1b9-6da3e7d3-46103884.dmp
> 12:56:01     INFO -  mozcrash INFO | Saved app info as /builds/slave/test/build/blobber_upload_dir/4075f36f-8498-a1b9-6da3e7d3-46103884.extra
> 12:56:01  WARNING -  PROCESS-CRASH | gtest | application crashed [@ mozalloc_abort(char const*)]
> 12:56:01     INFO -  Crash dump filename: /builds/slave/test/build/tests/gtest/4075f36f-8498-a1b9-6da3e7d3-46103884.dmp
> 12:56:01     INFO -  Operating system: Linux
> 12:56:01     INFO -                    0.0.0 Linux 3.2.0-76-generic-pae #111-Ubuntu SMP Tue Jan 13 22:34:29 UTC 2015 i686
> 12:56:01     INFO -  CPU: x86
> 12:56:01     INFO -       GenuineIntel family 6 model 62 stepping 4
> 12:56:01     INFO -       1 CPU
> 12:56:01     INFO -  Crash reason:  SIGSEGV
> 12:56:01     INFO -  Crash address: 0x0
> 12:56:01     INFO -  Process uptime: not available
> 12:56:01     INFO -  Thread 0 (crashed)
> 12:56:01     INFO -   0  firefox!mozalloc_abort(char const*) [mozalloc_abort.cpp:ec1df70dcdfe : 33 + 0x0]
> 12:56:01     INFO -      eip = 0x0804d5d0   esp = 0xbf8a0ad0   ebp = 0xbf8a0ae8   ebx = 0x080712bc
> 12:56:01     INFO -      esi = 0xb7612d9c   edi = 0x00000706   eax = 0x00000000   ecx = 0xb76138ac
> 12:56:01     INFO -      edx = 0x00000000   efl = 0x00210286
> 12:56:01     INFO -      Found by: given as instruction pointer in context
> 12:56:01     INFO -   1  firefox!abort [mozalloc_abort.cpp:ec1df70dcdfe : 80 + 0xc]
> 12:56:01     INFO -      eip = 0x0804d576   esp = 0xbf8a0af0   ebp = 0xbf8a0b08   ebx = 0x080712bc
> 12:56:01     INFO -      esi = 0xb7612d9c   edi = 0x00000706
> 12:56:01     INFO -      Found by: call frame info
> 12:56:01     INFO -   2  libnspr4.so!PR_Assert [prlog.c:ec1df70dcdfe : 553 + 0x5]
> 12:56:01     INFO -      eip = 0xb743c9e5   esp = 0xbf8a0b10   ebp = 0xbf8a0b38   ebx = 0xb74674a0
> 12:56:01     INFO -      esi = 0xb7612d9c   edi = 0x00000706
> 12:56:01     INFO -      Found by: call frame info
> 12:56:01     INFO -   3  libnss3.so!PK11_GetInternalSlot [pk11slot.c:ec1df70dcdfe : 1798 + 0x20]
> 12:56:01     INFO -      eip = 0xb7118538   esp = 0xbf8a0b40   ebp = 0xbf8a0b68   ebx = 0xb71fdcd0
> 12:56:01     INFO -      esi = 0x00000004   edi = 0xbf8a0c7c
> 12:56:01     INFO -      Found by: call frame info
> 12:56:01     INFO -   4  libxul.so!mozilla::nr_crypto_nss_random_bytes [nricectx.cpp:ec1df70dcdfe : 117 + 0x5]
> 12:56:01     INFO -      eip = 0xb11b9491   esp = 0xbf8a0b70   ebp = 0xbf8a0b88   ebx = 0xb60be704
> 12:56:01     INFO -      esi = 0x00000004   edi = 0xbf8a0c7c
> 12:56:01     INFO -      Found by: call frame info
> 12:56:01     INFO -   5  libxul.so!nr_ice_random_string [ice_ctx.c:ec1df70dcdfe : 831 + 0x15]
> 12:56:01     INFO -      eip = 0xb2aa485a   esp = 0xbf8a0b90   ebp = 0xbf8a0c38   ebx = 0xb60be704
> 12:56:01     INFO -      esi = 0x00000004   edi = 0xbf8a0c7c
> 12:56:01     INFO -      Found by: call frame info
> 12:56:01     INFO -   6  libxul.so!nr_ice_ctx_create [ice_ctx.c:ec1df70dcdfe : 337 + 0x12]
> 12:56:01     INFO -      eip = 0xb2aa4a68   esp = 0xbf8a0c40   ebp = 0xbf8a0d78   ebx = 0xb60be704
> 12:56:01     INFO -      esi = 0x968010cc   edi = 0xbf8a0c7c
> 12:56:01     INFO -      Found by: call frame info
> 12:56:01     INFO -   7  libxul.so!mozilla::NrIceCtx::Create(std::string const&, bool, bool, bool, bool, bool, mozilla::NrIceCtx::Policy) [nricectx.cpp:ec1df70dcdfe : 495 + 0x1b]
> 12:56:01     INFO -      eip = 0xb11babea   esp = 0xbf8a0d80   ebp = 0xbf8a0ea8   ebx = 0xb60be704
> 12:56:01     INFO -      esi = 0xbf8a0da4   edi = 0x00000005
> 12:56:01     INFO -      Found by: call frame info
> 12:56:01     INFO -   8  libxul.so!MultiTcpSocketTest::SetUp [multi_tcp_socket_unittest.cpp:ec1df70dcdfe : 54 + 0x2a]
> 12:56:01     INFO -      eip = 0xb094a98a   esp = 0xbf8a0eb0   ebp = 0xbf8a0f18   ebx = 0xb60be704
> 12:56:01     INFO -      esi = 0xbf8a0ef8   edi = 0xa880c480
> 12:56:01     INFO -      Found by: call frame info

That's one of the first mtransport gtests to run, so I'm not sure if others have the same issue. This feels like NSS isn't initialized properly.
Attachment #8721016 - Attachment is obsolete: true
Yes, NSS is not being initialized correctly. The mtransport test driver has an explicit call
to NSS_NoDBInit(). You will need something similar.
(In reply to Eric Rescorla (:ekr) from comment #12)
> Yes, NSS is not being initialized correctly. The mtransport test driver has
> an explicit call
> to NSS_NoDBInit(). You will need something similar.

We *do* call that in the new base MtransportTest in gtest_utils.h. I just don't understand why it would be initialized during a local run and not on the try run.

>  void SetUp() {
>    printf("MtransportTest::SetUp\n");
>    test_utils = new MtransportTestUtils();
>    NSS_NoDB_Init(nullptr);
>    NSS_SetDomesticPolicy();
In that case, I don't know.
Last try push is much happier. It looks like we were having some bad interaction with other NSS using tests. I was able to repro the issue by running the pkix tests and the mtransport tests, removing the NSS_Shutdown call fixed things.

This leaves one last test to convert, |ice_unittest|. This fails for me locally (prior to conversion), although I do see it passing in automation. :bwc, do you know if this test is expected to pass locally on a Linux machine?
Flags: needinfo?(docfaraday)
This is the failure log of the ice unittest. It was generated by running:

> ./mach cppunittest obj-x86_64-unknown-linux-gnu-clang/dist/bin/ice_unittest

My ifconfig details if that helps:

> [erahm@mozilla21268 mozilla-central]$ ifconfig
> eth0      Link encap:Ethernet  HWaddr 34:17:eb:d7:ea:19  
>           inet addr:192.168.0.9  Bcast:192.168.0.255  Mask:255.255.255.0
>           inet6 addr: fe80::3617:ebff:fed7:ea19/64 Scope:Link
>           UP BROADCAST RUNNING MULTICAST  MTU:1500  Metric:1
>           RX packets:220884 errors:0 dropped:0 overruns:0 frame:0
>           TX packets:126386 errors:0 dropped:0 overruns:0 carrier:0
>           collisions:0 txqueuelen:1000 
>           RX bytes:142009094 (142.0 MB)  TX bytes:24327584 (24.3 MB)
>           Interrupt:20 Memory:f7100000-f7120000 
> 
> lo        Link encap:Local Loopback  
>           inet addr:127.0.0.1  Mask:255.0.0.0
>           inet6 addr: ::1/128 Scope:Host
>           UP LOOPBACK RUNNING  MTU:65536  Metric:1
>           RX packets:61294 errors:0 dropped:0 overruns:0 frame:0
>           TX packets:61294 errors:0 dropped:0 overruns:0 carrier:0
>           collisions:0 txqueuelen:0 
>           RX bytes:5238805 (5.2 MB)  TX bytes:5238805 (5.2 MB)
> 
> tun0      Link encap:UNSPEC  HWaddr 00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00  
>           inet addr:10.22.252.6  P-t-P:10.22.252.5  Mask:255.255.255.255
>           UP POINTOPOINT RUNNING NOARP MULTICAST  MTU:1500  Metric:1
>           RX packets:0 errors:0 dropped:0 overruns:0 frame:0
>           TX packets:0 errors:0 dropped:0 overruns:0 carrier:0
>           collisions:0 txqueuelen:100 
>           RX bytes:0 (0.0 B)  TX bytes:0 (0.0 B)
>
Once files are all compiled into the same gtest this will cause duplicate
symbol errors. It's also unused.
Attachment #8723363 - Flags: review?(ekr)
Attachment #8721014 - Attachment is obsolete: true
This splits NrIceCtx initialization into its own function so that the tests
can initialize without having to create a dummy instance.

Byron I think I discussed this with you, can you please review?
Attachment #8723364 - Flags: review?(docfaraday)
Attachment #8721015 - Attachment is obsolete: true
This adds a base test for other mtransport tests to be derived from. It
handles common setup used by the mtransport tests and parses relevant env
vars.

This is the basis for the final set of these patches where the standalone
gtests are converted to xul-gtests.
Attachment #8723365 - Flags: review?(ekr)
Attachment #8721575 - Attachment is obsolete: true
Several of the proxy tunnel tests are broken. The proxy tunnel test is also
not run in automation.

Byron it looks like you've reviewed this portion most recently, would you
mind reviewing or redirecting? I understand if you think fixing these would
be a better solution, but perhaps we can file a follow up instead.
Attachment #8723366 - Flags: review?(docfaraday)
This is the tentative final patch. I'll hold of on review until the ice
unittest issue is resolved and a try run comes back.
Attachment #8723364 - Flags: review?(docfaraday) → review+
Comment on attachment 8723366 [details] [diff] [review]
Part 4: Disable broken proxy tunnel tests

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

Filed bug 1251212, please add some TODO comments pointing at it.
Attachment #8723366 - Flags: review?(docfaraday) → review+
(In reply to Eric Rahm [:erahm] from comment #19)
> Created attachment 8723343 [details]
> ice_unittest failure log
> 
> This is the failure log of the ice unittest. It was generated by running:
> 
> > ./mach cppunittest obj-x86_64-unknown-linux-gnu-clang/dist/bin/ice_unittest
> 
> My ifconfig details if that helps:
> 
> > [erahm@mozilla21268 mozilla-central]$ ifconfig
> > eth0      Link encap:Ethernet  HWaddr 34:17:eb:d7:ea:19  
> >           inet addr:192.168.0.9  Bcast:192.168.0.255  Mask:255.255.255.0
> >           inet6 addr: fe80::3617:ebff:fed7:ea19/64 Scope:Link
> >           UP BROADCAST RUNNING MULTICAST  MTU:1500  Metric:1
> >           RX packets:220884 errors:0 dropped:0 overruns:0 frame:0
> >           TX packets:126386 errors:0 dropped:0 overruns:0 carrier:0
> >           collisions:0 txqueuelen:1000 
> >           RX bytes:142009094 (142.0 MB)  TX bytes:24327584 (24.3 MB)
> >           Interrupt:20 Memory:f7100000-f7120000 
> > 
> > lo        Link encap:Local Loopback  
> >           inet addr:127.0.0.1  Mask:255.0.0.0
> >           inet6 addr: ::1/128 Scope:Host
> >           UP LOOPBACK RUNNING  MTU:65536  Metric:1
> >           RX packets:61294 errors:0 dropped:0 overruns:0 frame:0
> >           TX packets:61294 errors:0 dropped:0 overruns:0 carrier:0
> >           collisions:0 txqueuelen:0 
> >           RX bytes:5238805 (5.2 MB)  TX bytes:5238805 (5.2 MB)
> > 
> > tun0      Link encap:UNSPEC  HWaddr 00-00-00-00-00-00-00-00-00-00-00-00-00-00-00-00  
> >           inet addr:10.22.252.6  P-t-P:10.22.252.5  Mask:255.255.255.255
> >           UP POINTOPOINT RUNNING NOARP MULTICAST  MTU:1500  Metric:1
> >           RX packets:0 errors:0 dropped:0 overruns:0 frame:0
> >           TX packets:0 errors:0 dropped:0 overruns:0 carrier:0
> >           collisions:0 txqueuelen:100 
> >           RX bytes:0 (0.0 B)  TX bytes:0 (0.0 B)
> >

   I bet it is that tun interface; the failures you're seeing are gathering timeouts, which we would expect when trying to use an interface that is not routable.
Flags: needinfo?(docfaraday)
(In reply to Byron Campen [:bwc] from comment #26)
> Comment on attachment 8723366 [details] [diff] [review]
> Part 4: Disable broken proxy tunnel tests
> 
> Review of attachment 8723366 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Filed bug 1251212, please add some TODO comments pointing at it.

Will do.
(In reply to Byron Campen [:bwc] from comment #27)
>    I bet it is that tun interface; the failures you're seeing are gathering
> timeouts, which we would expect when trying to use an interface that is not
> routable.

That seems to have been it! I hopped off VPN and it was much happier. I still have one failure in TestSrflxCandPairingFilter [1]. Note that this test is not run in automation and seems to have wording that it needs a rather specific setup:

> Don't run this test at IETF meetings!
> /home/erahm/dev/mozilla-central/media/mtransport/test/ice_unittest.cpp:2988: Failure
> Failed
> This test needs one private IPv4 host candidate to work

[1] https://dxr.mozilla.org/mozilla-central/rev/d848a5628d801a460a7244cbcdea22d328d8b310/media/mtransport/test/ice_unittest.cpp#2976-3016
Small update to how env vars are loaded.
Attachment #8723843 - Flags: review?(ekr)
Attachment #8723365 - Attachment is obsolete: true
Attachment #8723365 - Flags: review?(ekr)
This converts the individual cppunit gtests into the combined mozilla gtest
which has access to xpcom internals. The build file is simplified to reflect
this change, individual main functions are removed, and duplicate symbols are
removed.

Sorry for the size of the patch, there really wasn't a better way to split
things up and still have everything compile.

The try run was happy on all platforms and all tests work for me locally aside
from the one expected failure as noted previously.
Attachment #8723847 - Flags: review?(docfaraday)
Attachment #8723367 - Attachment is obsolete: true
Comment on attachment 8723847 [details] [diff] [review]
Part 5: Switch over mtransport tests to mozilla gtests

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

This is probably easiest to review from bottom of file up. Generally I removed the main function, moved initialization to the test's SetUp (or possibly to the shared MtransportTest), moved cleanup to the TearDown function.

::: media/mtransport/test/ice_unittest.cpp
@@ +79,5 @@
>  namespace {
>  
> +// DNS resolution helper code
> +static std::string
> +Resolve(const std::string& fqdn, int address_family)

This was just moved from the bottom of the file, nothing new.

::: media/mtransport/test/mtransport_test_utils.h
@@ -65,5 @@
>      MOZ_ASSERT(NS_SUCCEEDED(rv));
>      sts_ = do_GetService(NS_SOCKETTRANSPORTSERVICE_CONTRACTID, &rv);
>      MOZ_ASSERT(NS_SUCCEEDED(rv));
> -
> -#ifdef MOZ_CRASHREPORTER

The xul gtest setup handles this.

::: media/mtransport/test/sctp_unittest.cpp
@@ +309,5 @@
>    }
>    return NS_OK;
>  }
>  
> +class SctpTransportTest : public MtransportTest {

Renamed to avoid clashing with other |TransportTest|.

::: media/webrtc/signaling/test/mediapipeline_unittest.cpp
@@ +700,5 @@
>  }  // end namespace
>  
>  
>  int main(int argc, char **argv) {
> +  ScopedXPCOM xpcom("mediapipeline_unittest");

MtransportTestUtils used to provide a ScopedXPCOM, but now it doesn't so we add one explicitly. The cpp unit tests still pass so we should be good.
(In reply to Eric Rahm [:erahm] from comment #29)
> That seems to have been it! I hopped off VPN and it was much happier. I
> still have one failure in TestSrflxCandPairingFilter [1]. Note that this
> test is not run in automation and seems to have wording that it needs a
> rather specific setup:

Michael pointed out the other day that the TestSrflxCandPairingFilter test is broken. So it is known, but we don't have ticket for it. I mentioned it in another ticket for improving the Pairing Filter tests that it should be fixed together with the improvement.
Comment on attachment 8723847 [details] [diff] [review]
Part 5: Switch over mtransport tests to mozilla gtests

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

Mostly convention/formatting nits, but there are a few relatively important issues.

::: media/mtransport/test/ice_unittest.cpp
@@ +124,5 @@
> +
> +class StunTest : public MtransportTest {
> +public:
> +  StunTest() : MtransportTest() {
> +    g_stun_server_hostname = kDefaultStunServerHostname;

I think the "g_" prefix is supposed to signify "global", which isn't true anymore. It probably makes sense to rename, so that it looks like a member variable. (Remove the "g_", and append a '_')

@@ +127,5 @@
> +  StunTest() : MtransportTest() {
> +    g_stun_server_hostname = kDefaultStunServerHostname;
> +  }
> +
> +  virtual void SetUp() {

override

@@ +151,5 @@
> +        WrapRunnableNM(&TestStunTcpServer::GetInstance, AF_INET6),
> +                       NS_DISPATCH_SYNC);
> +  }
> +
> +  virtual void TearDown() {

override

@@ +301,5 @@
>        peer_(peer),
>        stream_(stream),
>        candidate_(candidate),
> +      timer_handle_(nullptr),
> +      test_utils(utils) {

I think you want to do a %s/test_utils/test_utils_/g since this is a member variable everywhere now.

@@ +737,5 @@
>        remote_->GetCandidates(stream);
>  
>      for (size_t j=0; j<candidates.size(); j++) {
>        controlled_trickle_candidates_[stream].push_back(
> +          new SchedulableTrickleCandidate(this, stream, candidates[j], test_utils));

Wrap to 80

@@ +1278,5 @@
>   public:
>    void SetUp() {
> +    StunTest::SetUp();
> +
> +    //prev_tcp_so_sock_count = Preferences::GetInt("media.peerconnection.ice.tcp_so_sock_count");

Stray comment. I would be ok with not restoring this, but I'll leave it up to you.

@@ +1283,5 @@
> +    Preferences::SetInt("media.peerconnection.ice.tcp_so_sock_count", 3);
> +
> +    // Make sure NrIceCtx is in a testable state.
> +    test_utils->sts_target()->Dispatch(WrapRunnable(this,
> +                                                    &IceGatherTest::TearDown_s),

Let's do this in one place (StunTest::TearDown should do the trick).

@@ +1480,5 @@
>  
> +  void SetUp_s() {
> +    // Make sure NrIceCtx is in a testable state.
> +    NrIceCtx::internal_DeinitializeGlobal();
> +    RLogRingBuffer::CreateInstance();

If we do the RLogRingBuffer::CreateInstance(); in StunTest::SetUp(), and the NrIceCtx::internal_DeinitializeGlobal(); in StunTest::TearDown(), we don't need to override this.

@@ +1491,5 @@
>      test_utils->sts_target()->Dispatch(WrapRunnable(this,
>                                                      &IceConnectTest::TearDown_s),
>                                         NS_DISPATCH_SYNC);
> +
> +    RLogRingBuffer::DestroyInstance();

Let's do this in StunTest::TearDown()

@@ +2632,5 @@
>  }
>  
>  void AddNonPairableCandidates(
>      std::vector<SchedulableTrickleCandidate*>& candidates,
> +    IceTestPeer *peer, size_t stream, int net_type, MtransportTestUtils* test_utils) {

Wrap to 80

@@ +2639,5 @@
>        continue;
>      switch (i) {
>        case 1:
>          candidates.push_back(new SchedulableTrickleCandidate(peer, stream,
> +                   "candidate:0 1 UDP 2113601790 10.0.0.1 12345 typ host", test_utils));

Wrap all of these to 80

@@ +3073,5 @@
>    }
>  
>    ConnectTrickle();
> +  AddNonPairableCandidates(p1_->ControlTrickle(0), p1_, 0, host_net, test_utils);
> +  AddNonPairableCandidates(p2_->ControlTrickle(0), p2_, 0, host_net, test_utils);

Wrap to 80

@@ -3344,5 @@
> -  ::testing::TestEventListeners& listeners =
> -        ::testing::UnitTest::GetInstance()->listeners();
> -  // Adds a listener to the end.  Google Test takes the ownership.
> -
> -  listeners.Append(new test::RingbufferDumper(test_utils));

This is actually pretty important. I guess we should move this into MtransportTest::SetUp, remember the pointer to the RingbufferDumper, and then remove it in TearDown with TestEventListeners.Release(pointer).

::: media/mtransport/test/multi_tcp_socket_unittest.cpp
@@ +542,5 @@
>    RecvData(socks[1], socks[0], buf2, sizeof(buf2));
>    RecvData(socks[1], socks[0], buf1, sizeof(buf1));
>  }
>  
> +// TODO(ER): Figure out if this matters.

Yeah, it does.

::: media/mtransport/test/proxy_tunnel_socket_unittest.cpp
@@ +117,5 @@
>      nr_socket_destroy(&nr_socket_);
>      nr_proxy_tunnel_config_destroy(&config_);
>    }
>  
> +  virtual void SetUp() {

override

::: media/mtransport/test/sctp_unittest.cpp
@@ +61,5 @@
>  
>  
>  class TransportTestPeer : public sigslot::has_slots<> {
>   public:
> +  TransportTestPeer(std::string name, int local_port, int remote_port, MtransportTestUtils* utils)

Wrap to 80

::: media/mtransport/test/test_nr_socket_unittest.cpp
@@ +47,5 @@
>      private_addrs_(),
>      nats_() {
> +  }
> +
> +  virtual void SetUp() {

override

@@ +56,5 @@
>      sts_ = do_GetService(NS_SOCKETTRANSPORTSERVICE_CONTRACTID, &rv);
>      EXPECT_TRUE(NS_SUCCEEDED(rv)) << "Failed to get STS: " << (int)rv;
>    }
>  
> +  virtual void TearDown() {

override

::: media/mtransport/test/transport_unittests.cpp
@@ +831,5 @@
>      fds_[0] = nullptr;
>      fds_[1] = nullptr;
>    }
>  
> +  virtual void TearDown() {

override

::: media/mtransport/test/turn_unittest.cpp
@@ +107,5 @@
> +  static void SetUpTestCase() {
> +    NrIceCtx::Init(false, false, false);
> +  }
> +
> +  virtual void SetUp() {

override
Attachment #8723847 - Flags: review?(docfaraday)
This removes the g_ prefixes and adds a trailing '_' to the member variables.
Redirecting to :bwc as he's familiar with the changes now.
Attachment #8724202 - Flags: review?(docfaraday)
Attachment #8723843 - Attachment is obsolete: true
Attachment #8723843 - Flags: review?(ekr)
Updates per review, the g_ variables have been renamed, virtual -> override, long lines fixed. We now register the RingBufferDumper in the base MtransportTest and common initialization and teardown was moved to the base StunTest.
Attachment #8724204 - Flags: review?(docfaraday)
Attachment #8723847 - Attachment is obsolete: true
Attachment #8724202 - Flags: review?(docfaraday) → review+
Attachment #8724204 - Flags: review?(docfaraday) → review+
Attachment #8723363 - Flags: review?(ekr) → review+
Flags: needinfo?(erahm)
Depends on: 1274025
You need to log in before you can comment on or make changes to this bug.