906324 - Add media/mtransport test framework for ICE

Status: NEW

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

Description

[Adam Cain]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20100101 Firefox/22.0 (Beta/Release)
Build ID: 20130618035212

Steps to reproduce:

Need test programs to handle a single instance of an ICE peer
(the existing ICE unit tests run both peer instances), to be used
in the WebRTC test automation.

First step (per :ekr) is to split the existing ICE unit test into single-peer
utilities that run as separate processes (with the signaling done using
stdin & stdout, initially).  A Python wrapper should launch both peers
with arguments for various test cases.



Actual results:

n/a


Expected results:

n/a

Comment 1

[Adam Cain]

Attachment: Add media/mtransport test framework for ICE

Comment 2

[Adam Cain]

Attachment: Refactored IceTestPeer, added ice_peer

Revised IceTestPeer refactor, ice_peer improvements

Comment 3

[Eric Rescorla (:ekr)]

Attachment: Replace ice_unittest.cpp with IceTestPeerImpl.h to make reviewing easier

Comment 4

[Eric Rescorla (:ekr)]

Attachment: Replace ice_unittest.cpp with IceTestPeerImpl.h to make reviewing easier

Comment 5

[Eric Rescorla (:ekr)]

Comment on attachment 801185 [details] [diff] [review]
Replace ice_unittest.cpp with IceTestPeerImpl.h to make reviewing easier

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

This looks like a good start.

Adam: can you fix the indentation and whitespace issues
Ted: can you fix the make system to have this be IceTestPeer.cpp

Once we have that, I'll review completely.

::: media/mtransport/test/ice_unittest.cpp
@@ +35,3 @@
>        ice_ctx_(NrIceCtx::Create(name, offerer, set_priorities)),
>        streams_(),
> +      stream_index_(),

This doesn't seem to be used usefully because you're not setting the value in the map.

@@ +143,1 @@
>    }

This doesn't seem to be called.

Comment 6

[Eric Rescorla (:ekr)]

Attachment: Merge b2g-inbound to m-c.

Comment 7

[Eric Rescorla (:ekr)]

Comment on attachment 801189 [details] [diff] [review]
Merge b2g-inbound to m-c.

Mistekan upload

Comment 8

[Adam Cain]

Comment on attachment 801185 [details] [diff] [review]
Replace ice_unittest.cpp with IceTestPeerImpl.h to make reviewing easier

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

Sorry, could you clarify the whitespace issues?

::: media/mtransport/test/ice_unittest.cpp
@@ +35,3 @@
>        ice_ctx_(NrIceCtx::Create(name, offerer, set_priorities)),
>        streams_(),
> +      stream_index_(),

Indeed. Will fix.

@@ +143,1 @@
>    }

It's called from do_ice() in ice_peer.cpp.

Comment 9

[Eric Rescorla (:ekr)]

The whitespace issues are about not indenting the methods.

Please http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml

Comment 10

[Adam Cain]

(In reply to Eric Rescorla (:ekr) from comment #9)
> The whitespace issues are about not indenting the methods.
> 
> Please http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml

OK, so everything (inside the mozilla namespace) in IceTestPeerImpl.h 
should *not* be indented two spaces.  I think that's it.  Let me know
if there are other glitches.

Do I submit a new patch that obsoletes the previous ones?

Comment 11

[Eric Rescorla (:ekr)]

Please.

Comment 12

[Adam Cain]

Attachment: Refactored IceTestPeer, added ice_peer

Revised IceTestPeer refactor, ice_peer improvements, spacing fix

Comment 13

[Adam Cain]

Comment on attachment 802825 [details] [diff] [review]
Refactored IceTestPeer, added ice_peer

Changes from previous patch:
-- fixed spacing in IceTestPeerImpl.h
-- added setting of stream_index_ map entry in IceTestPeer::AddStream

Comment 14

[Adam Cain]

Comment on attachment 802825 [details] [diff] [review]
Refactored IceTestPeer, added ice_peer

I need to fix a glitch noticed in the build changes.  Will re-submit new patch.

Comment 15

[Adam Cain]

Comment on attachment 802825 [details] [diff] [review]
Refactored IceTestPeer, added ice_peer

No build problem with patch.  Was user error in testing.

Comment 16

[Eric Rescorla (:ekr)]

Comment on attachment 802825 [details] [diff] [review]
Refactored IceTestPeer, added ice_peer

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

This is looking pretty good.

Adam, can you please clean up the C++ code first, which is mostly
style nits. We've got a bunch of other people working on ice_unittest
and it would be nice to minimize merge conflicts. Separate out
the python patch and we'll land that separately.

Thanks!

::: media/mtransport/test/IceTestPeer.h
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// Original author: [e-mail]

Adam, please give yourself some credit here.

@@ +25,5 @@
> +namespace mozilla {
> +
> +class IceTestPeer : public sigslot::has_slots<> {
> + public:
> +

Remove unnecessary whitespace.

@@ +38,5 @@
> +  ~IceTestPeer();
> +
> +  void AddStream(int components);
> +
> +  std::vector<mozilla::RefPtr<NrIceMediaStream> > GetStreams() {

const

@@ +48,5 @@
> +  void SetTurnServer(const std::string addr, uint16_t port,
> +                     const std::string username,
> +                     const std::string password);
> +
> +  void SetTurnServer(const std::string addr, uint16_t port,

This seems like it could be private.

@@ +52,5 @@
> +  void SetTurnServer(const std::string addr, uint16_t port,
> +                     const std::string username,
> +                     const std::vector<unsigned char> password);
> +
> +  void SetFakeResolver(const std::string stun_server_addr, 

Please remove all trailing whitespace.

@@ +67,5 @@
> +  }
> +
> +  std::vector<std::string> GetCandidates(size_t stream);
> +
> +  bool gathering_complete() { return gathering_complete_; }

These all look like they could be const.

@@ +80,5 @@
> +  void SetExpectedTypes(NrIceCandidate::Type local, 
> +                        NrIceCandidate::Type remote);
> +
> +  // Start connecting to another peer
> +  void Connect(IceTestPeer *remote, TrickleMode trickle_mode,

Please remove unnecessary vertical whitespace between fxn decls here and below.

@@ +121,5 @@
> +  // Allow us to parse candidates directly on the current thread.
> +  void ParseCandidate(size_t i, const std::string& candidate);
> +
> +  void DisableComponent(size_t stream, int component_id);
> + private:

Please add whitespace betwofr private.

::: media/mtransport/test/IceTestPeerImpl.h
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// Original author: [e-mail]

Adam, please give yourself some credit.

@@ +24,5 @@
> +#include "gtest_utils.h"
> +
> +namespace mozilla {
> +
> +typedef bool (*CandidateFilter)(const std::string& candidate);

Duplicate.

@@ +26,5 @@
> +namespace mozilla {
> +
> +typedef bool (*CandidateFilter)(const std::string& candidate);
> +
> +IceTestPeer::IceTestPeer(const std::string& name, 

Fix trailing whitespace, here and below.

@@ +48,5 @@
> +    expected_local_type_(NrIceCandidate::ICE_HOST),
> +    expected_remote_type_(NrIceCandidate::ICE_HOST),
> +    packet_received_hook_(nullptr) {
> +
> +  ice_ctx_->SignalGatheringCompleted.connect(this,

Remove extra vertical whitespace.

@@ +69,5 @@
> +  int stream_num = streams_.size();
> +  snprintf(name, sizeof(name), "%s:stream%d", name_.c_str(), stream_num);
> +
> +  mozilla::RefPtr<NrIceMediaStream> stream =
> +      ice_ctx_->CreateStream(static_cast<char *>(name), components);

This cast shouldn't be needed, since char foo[]  is convertible to char *

@@ +83,5 @@
> +void IceTestPeer::SetStunServer(const std::string addr, uint16_t port) {
> +  std::vector<NrIceStunServer> stun_servers;
> +  ScopedDeletePtr<NrIceStunServer> server(NrIceStunServer::Create(addr,
> +                                                                  port));
> +  stun_servers.push_back(*server);

ASSERT_NE(0, server);

@@ +121,5 @@
> +  ASSERT_TRUE(NS_SUCCEEDED(ice_ctx_->SetResolver(
> +      dns_resolver_->AllocateResolver())));
> +}
> +
> +void IceTestPeer::Gather() {

Can we rename this StartGathering?

@@ +214,5 @@
> +
> +void IceTestPeer::DoTrickle(size_t stream) {
> +  std::cerr << "Doing trickle for stream " << stream << std::endl;
> +  // If we are in trickle deferred mode, now trickle in the candidates
> +  // for |stream}

|stream|

::: media/mtransport/test/ice_peer.cpp
@@ +10,5 @@
> +#include <map>
> +#include <string>
> +#include <vector>
> +#include <stdio.h>
> +#include <sys/un.h>

Do we need this?

@@ +62,5 @@
> +  return candidate.find("typ relay") != std::string::npos;
> +}
> +
> +static void log_attrs_block(const char *msg, const std::string& block) {
> +

Please remove all whitespace between function opening and the fxn body.

@@ +63,5 @@
> +}
> +
> +static void log_attrs_block(const char *msg, const std::string& block) {
> +
> +  if (msg != NULL)

if (msg)

@@ +78,5 @@
> +  }
> +  std::cerr << std::endl;
> +}
> +
> +static void append_attrsblock_to_string(const std::vector<std::string>& attrs, std::string& s) {

convention in this code is that out parameters should be pointers, not refs

@@ +104,5 @@
> +}
> +
> +static bool send_my_stream_attrs(IceTestPeer *p) {
> +
> +  std::vector<mozilla::RefPtr<mozilla::NrIceMediaStream> > my_streams = p->GetStreams();

This line may be too long.

@@ +106,5 @@
> +static bool send_my_stream_attrs(IceTestPeer *p) {
> +
> +  std::vector<mozilla::RefPtr<mozilla::NrIceMediaStream> > my_streams = p->GetStreams();
> +
> +  for (size_t i=0; i< my_streams.size(); ++i) {

Consistent linear whitespaceing. So, i<my_streams

@@ +138,5 @@
> +    }
> +
> +    if ((sscanf(buf, "<%s", tag_buf) == 1) && 
> +        (strcmp(tag_buf, "/>")))   // keep going if it's an end tag
> +      break; 

Can you rewrite this w/o sscanf...

Suggest just doing find and then substr?

@@ +140,5 @@
> +    if ((sscanf(buf, "<%s", tag_buf) == 1) && 
> +        (strcmp(tag_buf, "/>")))   // keep going if it's an end tag
> +      break; 
> +  }
> +  char* endtag = strchr(tag_buf, '>');

Let's use C++ std::string here...

@@ +163,5 @@
> +      break;
> +
> +    // trim newline character
> +    char *n = strchr(buf, '\n');
> +    if (n != NULL)

if (n)

@@ +173,5 @@
> +}
> +
> +static void packet_received(IceTestPeer *p, int stream, int component,
> +                            const unsigned char *data, int len) {
> +   if ((stream == 0) && (!memcmp(data, "DONE", 4))) {

if (stream)

@@ +181,5 @@
> +
> +   if (!g_offerer) {
> +     std::cerr << "Answerer echoing " << len << " byte packet on stream #"
> +               << stream << " comp #" << component << std::endl;
> +     p->SendPacket(stream, component, data, len);

Suggest we modify the packet just to make disambiguation earlier.

Generally, it would be nice if we mark the packets.

Suggest offerer packets are:

OFFER<index>
ANSWER<index>

@@ +191,5 @@
> +  p->SendPacket(0, 1, reinterpret_cast<const unsigned char *>("DONE"), 4);
> +}
> +
> +static void send_and_check_data(IceTestPeer *p) {
> +  unsigned char *data = new unsigned char[g_data_len];

Is this really so big we can't just allocate on the stack?

@@ +194,5 @@
> +static void send_and_check_data(IceTestPeer *p) {
> +  unsigned char *data = new unsigned char[g_data_len];
> +  unsigned int len = g_data_len;
> + 
> +  for (int n = 0; n < g_data_packets; ++n) {

Trim down the vertical whitespace.

@@ +216,5 @@
> +static void do_ice() {
> +  // SetUp:
> +  nsresult res;
> +
> +  mozilla::ScopedDeletePtr<IceTestPeer> p;

just initialize this here.

p(new ...)

@@ +267,5 @@
> +      std::cerr << "Peer Attributes for Stream #" << i << " (" << attrs_name << "):" << std::endl;
> +      log_attrs(stream_attrs);
> +    }
> +    test_utils->sts_target()->Dispatch(
> +        WrapRunnableRet(my_streams[i], &mozilla::NrIceMediaStream::ParseAttributes,

Can you add a ParseAttributes wrapper for consistency?

@@ +312,5 @@
> +
> +static bool parse_opts(int argc, char **argv) {
> +
> +  for (int i=1; i < argc; i++) {
> +

gratuitous whitespace.

nit: this code does ++i

@@ +323,5 @@
> +      continue;
> +    }
> +    if (PL_strcasecmp(argv[i], "-c") == 0) {
> +      g_num_comps = atoi(argv[++i]);
> +      continue;

We may find this to be a PITA, since you can't do streams with non-uniform # of comps.

@@ +374,5 @@
> +
> +#ifdef LINUX
> +  // This test can cause intermittent oranges on the builders on Linux
> +  CHECK_ENVIRONMENT_FLAG("MOZ_WEBRTC_TESTS")
> +#endif

I don't think we need this. Instead let's just require some argument
and otherwise exit (0)

::: media/mtransport/test/test_ice_peer.py
@@ +10,5 @@
> +import sys
> +import threading
> +
> +# Defaults:
> +ice_peer_bin = './ice_peer'

This may make us sad later, but it's fine for now.

@@ +21,5 @@
> +    """
> +    def __init__(self, pid, timeout, event):
> +        threading.Thread.__init__(self)
> +        # Make this thread die when the main thread exits
> +        self.setDaemon(True) 

Trailing whitesapce.

@@ +66,5 @@
> +    p2watchdog = WatchdogThread(p2.pid, peer_timeout, p2event)
> +    p2watchdog.start()
> +
> +    p2err = p2.communicate()[1]
> +    p1err = p1.communicate()[1]

If I'm reading this correctly, this imposes lockstep semantics.

Suggest, we have two threads, one for each direction and then wait for both
processes to end.

@@ +98,5 @@
> +        f2 = outfile_basename + "_stderr2.log"
> +
> +        for (filename, contents) in [(f1, p1result[1]), (f2, p2result[1])]:
> +            f = open(filename, 'w')
> +            f.write(contents)

Is there a way we can interlace these so that they are time sorted?

@@ +146,5 @@
> +    #p2args = ['-t']
> +    #result &= run_ice_peer_pair(ice_peer_bin, p1args, p2args,
> +    #                             "trickle_one_stream")
> +
> +    return result

Let's remove this fxn. We'll have a wrapper take care of this.

@@ +148,5 @@
> +    #                             "trickle_one_stream")
> +
> +    return result
> +
> +def main(argv):

And have this just run one program with a python config file to provide the args.

Comment 17

[(not currently active) Ted Mielczarek]

Attachment: Try putting IceTestPeer into its own library

I'm attaching this just for reference, although it doesn't actually build. ekr asked me to look into fixing things so that IceTestPeer could be a .cpp file and link into the CPP_UNIT_TESTS. Unfortunately that goes down a rabbit hole because TestHarness.h defines a bunch of symbols inline, so the only way to fix it for real would be to split some of it out into a TestHarness.cpp. We can do that, but it's going to be a bit more work.

Anyway, just putting this here so it doesn't get lost. We can finish this up in a followup if it turns out to be annoying.

Comment 18

[Adam Cain]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #17)
> Created attachment 806160 [details] [diff] [review]
> Try putting IceTestPeer into its own library
> 
> I'm attaching this just for reference, although it doesn't actually build.
> ekr asked me to look into fixing things so that IceTestPeer could be a .cpp
> file and link into the CPP_UNIT_TESTS. Unfortunately that goes down a rabbit
> hole because TestHarness.h defines a bunch of symbols inline, so the only
> way to fix it for real would be to split some of it out into a
> TestHarness.cpp. We can do that, but it's going to be a bit more work.
> 
> Anyway, just putting this here so it doesn't get lost. We can finish this up
> in a followup if it turns out to be annoying.

Thanks, Ted.

I'll leave it to EKR to say if it's cleaner to create a new icetestpeer.lib
(with IceTestPeer.h and IceTestPeer.cpp in mtransport/test_lib), or just
keep using IceTestPeerImpl.h.

Comment 19

[Adam Cain]

(In reply to Eric Rescorla (:ekr) from comment #16)
> Comment on attachment 802825 [details] [diff] [review]
> Refactored IceTestPeer, added ice_peer
> 
> Review of attachment 802825 [details] [diff] [review]:
> -----------------------------------------------------------------
[...]
> @@ +83,5 @@
> > +void IceTestPeer::SetStunServer(const std::string addr, uint16_t port) {
> > +  std::vector<NrIceStunServer> stun_servers;
> > +  ScopedDeletePtr<NrIceStunServer> server(NrIceStunServer::Create(addr,
> > +                                                                  port));
> > +  stun_servers.push_back(*server);
> 
> ASSERT_NE(0, server);

I get a compiler error: no match for 'operator!=' ...


> @@ +323,5 @@
> > +      continue;
> > +    }
> > +    if (PL_strcasecmp(argv[i], "-c") == 0) {
> > +      g_num_comps = atoi(argv[++i]);
> > +      continue;
> 
> We may find this to be a PITA, since you can't do streams with non-uniform #
> of comps.

We could use a comma-separated list to specify num_components.
E.g., "-c 1,2,3".  Would that work?

> 
> @@ +374,5 @@
> > +
> > +#ifdef LINUX
> > +  // This test can cause intermittent oranges on the builders on Linux
> > +  CHECK_ENVIRONMENT_FLAG("MOZ_WEBRTC_TESTS")
> > +#endif
> 
> I don't think we need this. Instead let's just require some argument
> and otherwise exit (0)

Not understanding.  Just have ice_peer expect some "-webrtc" or "-linux" arg
and exit(0) if not found?



> @@ +146,5 @@
> > +    #p2args = ['-t']
> > +    #result &= run_ice_peer_pair(ice_peer_bin, p1args, p2args,
> > +    #                             "trickle_one_stream")
> > +
> > +    return result
> 
> Let's remove this fxn. We'll have a wrapper take care of this.
> 
> @@ +148,5 @@
> > +    #                             "trickle_one_stream")
> > +
> > +    return result
> > +
> > +def main(argv):
> 
> And have this just run one program with a python config file to provide the
> args.

So test_ice_peer.py takes a config file argument, and that config file
specifies a number of tests, each test specifies the args to be used for
the offerer and answerer?

Comment 20

[Eric Rescorla (:ekr)]

(In reply to Adam Cain from comment #19)
> (In reply to Eric Rescorla (:ekr) from comment #16)
> > Comment on attachment 802825 [details] [diff] [review]
> > Refactored IceTestPeer, added ice_peer
> > 
> > Review of attachment 802825 [details] [diff] [review]:
> > -----------------------------------------------------------------
> [...]
> > @@ +83,5 @@
> > > +void IceTestPeer::SetStunServer(const std::string addr, uint16_t port) {
> > > +  std::vector<NrIceStunServer> stun_servers;
> > > +  ScopedDeletePtr<NrIceStunServer> server(NrIceStunServer::Create(addr,
> > > +                                                                  port));
> > > +  stun_servers.push_back(*server);
> > 
> > ASSERT_NE(0, server);
> 
> I get a compiler error: no match for 'operator!=' ...

OK, try ASSERT_TRUE(server != NULL)




> 
> > @@ +323,5 @@
> > > +      continue;
> > > +    }
> > > +    if (PL_strcasecmp(argv[i], "-c") == 0) {
> > > +      g_num_comps = atoi(argv[++i]);
> > > +      continue;
> > 
> > We may find this to be a PITA, since you can't do streams with non-uniform #
> > of comps.
> 
> We could use a comma-separated list to specify num_components.
> E.g., "-c 1,2,3".  Would that work?

Sure. Or you could just defer it.

 
> > 
> > @@ +374,5 @@
> > > +
> > > +#ifdef LINUX
> > > +  // This test can cause intermittent oranges on the builders on Linux
> > > +  CHECK_ENVIRONMENT_FLAG("MOZ_WEBRTC_TESTS")
> > > +#endif
> > 
> > I don't think we need this. Instead let's just require some argument
> > and otherwise exit (0)
> 
> Not understanding.  Just have ice_peer expect some "-webrtc" or "-linux" arg
> and exit(0) if not found?

It looks to me like we need argument processing in any case, so I suspect
you can just arrange that as long as no arguments are provided, it just
exit(0)s



> 
> > @@ +146,5 @@
> > > +    #p2args = ['-t']
> > > +    #result &= run_ice_peer_pair(ice_peer_bin, p1args, p2args,
> > > +    #                             "trickle_one_stream")
> > > +
> > > +    return result
> > 
> > Let's remove this fxn. We'll have a wrapper take care of this.
> > 
> > @@ +148,5 @@
> > > +    #                             "trickle_one_stream")
> > > +
> > > +    return result
> > > +
> > > +def main(argv):
> > 
> > And have this just run one program with a python config file to provide the
> > args.
> 
> So test_ice_peer.py takes a config file argument, and that config file
> specifies a number of tests, each test specifies the args to be used for
> the offerer and answerer?

Sounds good

Comment 21

[Adam Cain]

Attachment: Refactored IceTestPeer, added ice_peer (C++ changes)

Revised IceTestPeer refactor, ice_peer improvements (C++ changes)

Comment 22

[Adam Cain]

Attachment: Refactored IceTestPeer, added ice_peer (C++ changes)

Revised IceTestPeer refactor, ice_peer improvements (C++ changes, no Makefile change)

Comment 23

[Adam Cain]

Attachment: Added ICE test python wrapper and test configs

ICE test python wrapper and sample test configs

Comment 24

[Adam Cain]

Comment on attachment 814269 [details] [diff] [review]
Added ICE test python wrapper and test configs

Note that the test wrapper saves the stderr output (with timestamps) for both child processes, and then it creates a merged log with the stderr output from both children sorted by timestamp.

Comment 25

[Adam Cain]

(In reply to Eric Rescorla (:ekr) from comment #20)
> (In reply to Adam Cain from comment #19)
> > (In reply to Eric Rescorla (:ekr) from comment #16)
> > > Comment on attachment 802825 [details] [diff] [review]
> > > Refactored IceTestPeer, added ice_peer
> > > 
> > > Review of attachment 802825 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > [...]
> > > @@ +323,5 @@
> > > > +      continue;
> > > > +    }
> > > > +    if (PL_strcasecmp(argv[i], "-c") == 0) {
> > > > +      g_num_comps = atoi(argv[++i]);
> > > > +      continue;
> > > 
> > > We may find this to be a PITA, since you can't do streams with non-uniform #
> > > of comps.
> > 
> > We could use a comma-separated list to specify num_components.
> > E.g., "-c 1,2,3".  Would that work?
> 
> Sure. Or you could just defer it.

Deferred, for now.


> > > @@ +374,5 @@
> > > > +
> > > > +#ifdef LINUX
> > > > +  // This test can cause intermittent oranges on the builders on Linux
> > > > +  CHECK_ENVIRONMENT_FLAG("MOZ_WEBRTC_TESTS")
> > > > +#endif
> > > 
> > > I don't think we need this. Instead let's just require some argument
> > > and otherwise exit (0)
> > 
> > Not understanding.  Just have ice_peer expect some "-webrtc" or "-linux" arg
> > and exit(0) if not found?
> 
> It looks to me like we need argument processing in any case, so I suspect
> you can just arrange that as long as no arguments are provided, it just
> exit(0)s

I changed things so that ice_peer requires either an "-o" or "-a"
argument to specify offerer or answerer mode.  If neither is supplied
(e.g., when *no* CLI args are supplied), it does exit(0) immediately.

Comment 26

[Eric Rescorla (:ekr)]

Comment on attachment 814267 [details] [diff] [review]
Refactored IceTestPeer, added ice_peer (C++ changes)

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

Byron,

Can you please:

1. Clean up my comments in ice_peer.cpp
2. Review the refactor to make sure nothing bad crept in (don't try to
fix stuff that was bad before unless it is now worse)

If the changes in #2 are small, just r+ the whole thing and we can land
it.

I think that will be easier since this is sort of a live code area and
Byron can work full time on it.

::: media/mtransport/test/IceCandidatePairCompare.h
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// Original author: [e-mail]

I just skimmed this file. I am going to assume that the code is unchanged.

@@ +9,5 @@
> +#ifndef icecandidatepaircompare_h__
> +#define icecandidatepaircompare_h__
> +
> +#include "nricemediastream.h"
> + 

Fix trailing whitespace.

::: media/mtransport/test/ice_peer.cpp
@@ +72,5 @@
> +  if (gettimeofday(&tv, NULL) != 0)
> +    return;
> +  struct tm tm;
> +  localtime_r(&tv.tv_sec, &tm);
> +  fprintf(stderr, "[%02d:%02d:%02d.%06lu] ", tm.tm_hour, tm.tm_min, tm.tm_sec, tv.tv_usec);

These seem like really unixy apis.

Suggest just using the r_time() integer values.

@@ +162,5 @@
> +  size_t start_tag = sbuf.find('<');
> +  if ((start_tag == std::string::npos) || (start_tag == sbuf.length()))
> +    return false;
> +  size_t end_tag = sbuf.find('>', start_tag+1);
> +  if ((end_tag == std::string::npos) || (end_tag == start_tag))

Is there any way for end_tag to == start_tag?

@@ +170,5 @@
> +}
> +
> +static bool receive_peer_attrs(IceTestPeer *p, std::vector<std::string>& attrs,
> +                               std::string* attrs_name) {
> +  char buf[1024];

Is this long enough?

@@ +180,5 @@
> +    if (!fgets(buf, 1024, stdin)) {
> +      log_to_stderr("Error reading attr start from peer.");
> +      return false;
> +    }
> +    if ((!find_and_parse_tag(buf, attrs_name)) || (*attrs_name == "/"))

What case is this "/" handling?

@@ +213,5 @@
> +
> +static std::string packet_label(int index, int stream_num, int comp_num) {
> +  char buf[256];
> +  snprintf(buf, 255, "packet_%d_on_stream_%d_component_%d",
> +           index, stream_num, comp_num);

use sizeof()

Also, use PR_snprintf()

@@ +220,5 @@
> +
> +static void mark_data_packet(unsigned char *data, unsigned int max_len,
> +                             int index, int stream_num, int comp_num) {
> +  // could include the stream_num, comp_num in data (for extra checking)
> +  snprintf(reinterpret_cast<char *>(data), max_len, "%s<%d>",

PR_snprintf()

@@ +314,5 @@
> +    p->SetCandidateFilter(IsRelayCandidate);
> +  }
> +
> +  p->Gather();
> +  ASSERT_TRUE_WAIT(p->gathering_complete(), 10000);

This will not work with TRICKLE_REAL but let that go for now.

@@ +338,5 @@
> +             "Error parsing peer's Global Attributes." << std::endl;
> +
> +  // Receive and parse peer's stream Attributes (Candidates)
> +  std::vector<mozilla::RefPtr<mozilla::NrIceMediaStream> > my_streams =
> +                                                             p->GetStreams();

Fix indent.

@@ +403,5 @@
> +      g_mode = IceTestPeer::MODE_OFFERER;
> +      continue;
> +    }
> +    if (PL_strcasecmp(argv[i], "-a") == 0) {
> +      g_mode = IceTestPeer::MODE_ANSWERER;

Can you add some primitive logic to detect conflicting instructions?

E.g., -o and -a together?

I don't care if you catch everything, but just what's easy to catch
and clearly insane.

@@ +480,5 @@
> +  // Gets hold of the event listener list.
> +  ::testing::TestEventListeners& listeners =
> +    ::testing::UnitTest::GetInstance()->listeners();
> +
> +   // Remove default listener (as it mucks up stdout)

Can you merge this comment with the one above, since we only use listeners for this...

::: media/mtransport/test/ice_unittest.cpp
@@ +222,5 @@
>      ASSERT_TRUE_WAIT(p2_->gathering_complete(), 10000);
>    }
>  
> +  void ConnectTrickle(IceTestPeer::TrickleMode 
> +                      trickle = IceTestPeer::TRICKLE_SIMULATE) {

trailing whitespace.

Comment 27

[Byron Campen [:bwc]]

(In reply to Eric Rescorla (:ekr) from comment #26)
> @@ +162,5 @@
> > +  size_t start_tag = sbuf.find('<');
> > +  if ((start_tag == std::string::npos) || (start_tag == sbuf.length()))
> > +    return false;
> > +  size_t end_tag = sbuf.find('>', start_tag+1);
> > +  if ((end_tag == std::string::npos) || (end_tag == start_tag))
> 
> Is there any way for end_tag to == start_tag?
>

Not unless find() is bonkers. Have removed the check.

> @@ +170,5 @@
> > +}
> > +
> > +static bool receive_peer_attrs(IceTestPeer *p, std::vector<std::string>& attrs,
> > +                               std::string* attrs_name) {
> > +  char buf[1024];
> 
> Is this long enough?
>

Almost certainly for tags. For the attributes themselves, it probably makes sense to be willing to accumulate until we see an endl. I can implement this. Also, will convert the hard-coded 1024 literals to sizeof(buf).

> @@ +180,5 @@
> > +    if (!fgets(buf, 1024, stdin)) {
> > +      log_to_stderr("Error reading attr start from peer.");
> > +      return false;
> > +    }
> > +    if ((!find_and_parse_tag(buf, attrs_name)) || (*attrs_name == "/"))
> 
> What case is this "/" handling?
> 

Looks like a stray "</>" or something? I am not sure we want to be silently eating these. Pretty sure we should either not do the check, or assert. Have a preference?


> @@ +314,5 @@
> > +    p->SetCandidateFilter(IsRelayCandidate);
> > +  }
> > +
> > +  p->Gather();
> > +  ASSERT_TRUE_WAIT(p->gathering_complete(), 10000);
> 
> This will not work with TRICKLE_REAL but let that go for now.
>

Adding a TODO with your name on it.

Comment 28

[Byron Campen [:bwc]]

Attachment: Refactored IceTestPeer, added ice_peer (C++ changes)

Comment 29

[Byron Campen [:bwc]]

Attachment: Added ICE test python wrapper and test configs

No changes, just renaming the patch to something that bzexport will not misinterpret as a request to upload changeset sequence number 150021 (https://hg.mozilla.org/integration/mozilla-inbound/rev/4c6899cf5799) to the bug from whence it came (https://bugzilla.mozilla.org/show_bug.cgi?id=923390).

Comment 30

[Byron Campen [:bwc]]

Comment on attachment 814267 [details] [diff] [review]
Refactored IceTestPeer, added ice_peer (C++ changes)

The name on this patch confuses bzexport mightily. Have renamed.

Comment 31

[Byron Campen [:bwc]]

Comment on attachment 814269 [details] [diff] [review]
Added ICE test python wrapper and test configs

The name on this patch confuses bzexport mightily. Have renamed and attached, marking this one as obsolete.

Comment 32

[Byron Campen [:bwc]]

Comment on attachment 818162 [details] [diff] [review]
Added ICE test python wrapper and test configs

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

Asking for review on behalf of Adam Cain, since I needed to replace the patch.

Comment 33

[Eric Rescorla (:ekr)]

Comment on attachment 818162 [details] [diff] [review]
Added ICE test python wrapper and test configs

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

This looks good.

I would suggest that you have the conf file provide the expected success/failure conditions
to make negative tests easier.

::: media/mtransport/test/test_ice_peer.py
@@ +141,5 @@
> +                if ts[1]:
> +                    m += pair[1] + '\n'
> +                if ts[0]:
> +                    lines[0].insert(0, pair[0])
> +    return m

Can I suggest a simpler algorithm here:

Stuff these into one list and then just sort with a comparator that sorts first
by time and then by log file.

@@ +171,5 @@
> +        for (filename, contents) in [(f1, p1result[1]), (f2, p2result[1]),
> +                                     (fm, merged_log)]:
> +            f = open(filename, 'w')
> +            f.write(contents)
> +            f.close()

i would refactor this as "def save_log(name, obj):
  f = open(...)

Or maybe there is a python comnand to just stuff a string into a file.

@@ +225,5 @@
> +            return 2
> +
> +    if not conf_file:
> +        print('Must supply test conf file with -f argument.')
> +        return 2

Suggest use the options default feature here.

Comment 34

[Byron Campen [:bwc]]

Attachment: Refactored IceTestPeer, added ice_peer (C++ changes)

Make the compiler stop whining about the length of some unsigned int printfs.

Comment 35

[Byron Campen [:bwc]]

Attachment: Refactored IceTestPeer, added ice_peer (C++ changes)

Fixing a number of bugs in parse_opts and surrounding code.

Comment 36

[Byron Campen [:bwc]]

Attachment: Refactored IceTestPeer, added ice_peer (C++ changes)

Windows and some of the B2G builds did not like the typedefs in r_types. This seems to work elsewhere in the codebase.

Comment 37

[Byron Campen [:bwc]]

https://tbpl.mozilla.org/?tree=Try&rev=8c3c9956c87d

Comment 38

[Byron Campen [:bwc]]

Attachment: Added ICE test python wrapper and test configs

Remove some now incorrect comments.

Comment 39

[Maire Reavy [:mreavy]]

Still relevant or overtaken by events?

Comment 40

[Byron Campen [:bwc]]

This is still relevant, but the utility we get from it has diminished somewhat now that the NAT simulator has landed.

Comment 41

[Bulk Bug Changes for mreavy's org]

Mass change P3->P4 to align with new Mozilla triage process.