853127 - Add unit tests for moznetwork

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Comment 1

[Abhishek Singh]

Attachment: unit test for moznetwork module.

Comment 2

[Abhishek Singh]

This patch contains the unit tests for moznetwork module. I have tested it on different platform and it passed well in linux(77 % coverage), windows but it failed on Mac osx that's shows moznetwork module is not compatible with Mac osx. IpAddress parser in conf.py is returning list of ip(s) on mac, which works correctly.

when I ran moznetwork.py alone(by printing get_ip()) it shows the following error:

foo-Mac:testmoz foo$ python moznetwork.py
Traceback (most recent call last):
  File "moznetwork.py", line 71, in <module>
    ip = get_ip()
  File "moznetwork.py", line 52, in get_ip
    interfaces = _get_interface_list()
  File "moznetwork.py", line 35, in _get_interface_list
    raise NetworkError('Unable to call ioctl with SIOCGIFCONF')
__main__.NetworkError: Unable to call ioctl with SIOCGIFCONF

Comment 3

[Jeff Hammel]

Not sure if the platform should be all:all but given the mac comment it shouldn't be linux64

Comment 4

[cmtalbert]

Comment on attachment 727344 [details] [diff] [review]
unit test for moznetwork module.

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

I think this is a great first step, r+ for linux (with the nit addressed)  :)

Do you want to make this a linux only test or are you planning to continue working on this so that it will work properly on windows? (the commandline utility you would have to call would be ipconfig on windows).  I would want to solve the windows and mac issues before landing this or we land this as is for linux (with proper manifest declarations so that it only runs there) and then we file follow on bugs for the other two OS's.

::: moznetwork/tests/test_moznetwork.py
@@ +16,5 @@
> +    def setUp(self):
> +        self.ip_parser = IpAddressParser()
> +
> +    def tearDown(self):
> +        del self.ip_parser

I don't think that del is strictly necessary in this case. The python garbage collector should be able to properly free this.  We can probably just not use a teardown function here since I don't think it is necessary to do anything for this class.

Comment 5

[William Lachance (:wlach)]

Comment on attachment 727344 [details] [diff] [review]
unit test for moznetwork module.

A few drive-by comments on this patch (in addition to what Clint mentioned):

>+# Author : Abhishek Singh <[e-mail]>

We don't include authorship information in source files (that's what the commit log is for)

>+import os
>+import re
>+import subprocess
>+
>+
>+class IpAddressParser:
>+    """Parser class that parses the output of `ifconfig` or `ipconfig`
>+    based on the platform."""
>+
>+    def __init__(self):
>+        if os.name != 'nt':
>+            self.command = 'ifconfig'
>+            self.look_patt = r'inet'
>+        else:
>+            self.command = 'ipconfig'
>+            self.look_patt = r'IPv4 Address'
>+        return
>+    def get_ipaddr(self):
>+        """Returns an available network interface address.
>+        Returns None if no available network interface is found"""
>+
>+        p = subprocess.Popen([self.command], stdout=subprocess.PIPE)
>+        content = p.communicate()
>+        ip_patt = r'[\s\.\:]*(\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3})'
>+        ip_regex = self.look_patt + ip_patt
>+        patt = re.compile(ip_regex)
>+        ip_list = patt.findall(content[0])
>+
>+        return ip_list

A few things here:

1. I don't think this needs to be a class. A single get_ipaddr method should be sufficient. Just include the platform detection logic in __init__ in get_ipaddr.
2. I don't think it needs to be a seperate file. Just include the source in the test_moznetwork.py file (if we ever have multiple moznetwork tests that use this, sure, we can move it out... until then it's easier if we have a smaller number of files)

Comment 6

[William Lachance (:wlach)]

Giving this bug a better title.

Comment 7

[Jeff Hammel]

Comment on attachment 727344 [details] [diff] [review]
unit test for moznetwork module.

I agree with previous points: there's no reason for IpAddressParser to be a class and we no longer put author information in the file. 

In addition, conf.py should not be in the manifest, since it contains no tests.  I would put the code directly in the test in this case and avoid the additional module.  It is worth contemplating if it should go in moznetwork itself, but possibly YAGNI.

I don't think this code is 100% fool-proof, since there can be a variety of network devices and addresses per machine.  That said, the rest of moznetwork has similar issues.

r-ing for now, but very close.  Thanks for the patch!

Comment 8

[Abhishek Singh]

Attachment: unit test for moznetwork

Changes done. tests are added in a single file test_moznetwork module

Comment 9

[Jeff Hammel]

Comment on attachment 728410 [details] [diff] [review]
unit test for moznetwork

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

Apologies for the late review.

In general, it'd be nice to have more comments.

This actually doesn't pass for me :(

(mozbase)│python test_moznetwork.py
F
======================================================================
FAIL: test_get_lan_ip (__main__.TestMoznetwork)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "test_moznetwork.py", line 33, in test_get_lan_ip
    "get_lan_ip() failed !")
AssertionError: get_lan_ip() failed !

----------------------------------------------------------------------
Ran 1 test in 0.003s

FAILED (failures=1)

The regex fails to match :( The interface of note:

wlan0     Link encap:Ethernet  HWaddr 58:91:cf:4d:a8:c8  
          inet addr:192.168.1.43  Bcast:192.168.1.255  Mask:255.255.255.0
          inet6 addr: fe80::5a91:cfff:fe4d:a8c8/64 Scope:Link
          UP BROADCAST RUNNING MULTICAST  MTU:1500  Metric:1
          RX packets:437925 errors:0 dropped:0 overruns:0 frame:0
          TX packets:371848 errors:0 dropped:0 overruns:0 carrier:0
          collisions:0 txqueuelen:1000 
          RX bytes:359784946 (359.7 MB)  TX bytes:113136021 (113.1 MB)

It is pretty hazardous to parse this, since it may vary on various systems.  You can use http://stackoverflow.com/questions/8797130/algorithm-to-extract-network-info-from-ifconfig-ubuntu , maybe, but ultimately we're trying to do something that's hard.

::: moznetwork/tests/test_moznetwork.py
@@ +1,1 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public

It'd be nice to have a shebang

@@ +13,5 @@
> +class TestMoznetwork(unittest.TestCase):
> +    """Test class to test moznetwork module """
> +
> +    def test_get_lan_ip(self):
> +        if os.name != 'nt':

In general, it is better to test for positives than negatives:

  if os.name == 'nt':
    command = 'ipconfig'...

@@ +25,5 @@
> +        content = p.communicate()
> +        ip_patt = r'[\s\.\:]*(\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3})'
> +        ip_regex = look_patt + ip_patt
> +        patt = re.compile(ip_regex)
> +        ip_list = patt.findall(content[0])

I'd split this up into a free standing function to parse the i{f,p}config data.

I'd also tend to put the os conditional and re.compilation at the module level.  Speed up, not that it matters.

@@ +28,5 @@
> +        patt = re.compile(ip_regex)
> +        ip_list = patt.findall(content[0])
> +
> +        self.assertIn(moznetwork.get_lan_ip(), ip_list,
> +                      "get_lan_ip() failed !")

Probably want a better error message than this

Comment 10

[Anhad Jai Singh (:ffledgling)]

Attachment: Unit-test for get_ip()

I've tested this against the outputs of ifconfig from a Mac, two versions of linux- Fedora18, LinuxMint13 , and an XP with python2.4 (since as mcote pointed out, windows slaves are still running 2.4)

I couldn't test it on a BSD OS, since I do not have one at hand.

If someone can please cross-check the patch on a Mac, and an Ubuntu I'd be grateful. :)

If someone wants an explanation for how the regex works, you can look at the state machine for my regex here: http://www.debuggex.com/i/rP_qIsaXNUMJePAc.png

Comment 11

[Andrew Halberstadt [:ahal]]

Comment on attachment 767540 [details] [diff] [review]
Unit-test for get_ip()

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

Looks good to me! But I think we should also test the _get_interface_list path. Maybe we could mock socket.gethostbyname to None and then assert we can still get the correct ip address and that an Exception is raised if os == 'nt'

Comment 12

[cmtalbert]

Comment on attachment 767540 [details] [diff] [review]
Unit-test for get_ip()

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

This looks good to me for the get_ip. A test for _get_interface_list would also be interesting but since it's supposed to be an internal method, I'm less concerned about its tests (i.e. if you're depending on that and it breaks, it's your own fault).

Comment 13

[Andrew Halberstadt [:ahal]]

(In reply to Clint Talbert ( :ctalbert ) from comment #12)
> This looks good to me for the get_ip. A test for _get_interface_list would
> also be interesting but since it's supposed to be an internal method, I'm
> less concerned about its tests (i.e. if you're depending on that and it
> breaks, it's your own fault).

I wasn't so much concerned about people using _get_interface_list directly, more about the fact that it is the failure path in case socket.gethostbyname fails. If we don't force socket.gethostbyname to fail in our tests, we may never know if _get_interface_list still works or not.

Comment 14

[Anhad Jai Singh (:ffledgling)]

Attachment: Updated patch

This patch adds a test that forces get_ip() to use the fallback path via _get_interface_list as :ahal requested.

All tests pass on local testing.

:ahal please take a look and tell me if this what you had in mind. Thanks!

Comment 15

[Andrew Halberstadt [:ahal]]

Comment on attachment 769668 [details] [diff] [review]
Updated patch

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

Thanks! Looks good.

::: moznetwork/tests/test.py
@@ +55,5 @@
> +                standardoutput, standarderror = ps.communicate()
> +
> +                # Generate a list of IPs by parsing the output of ip/ifconfig
> +                ip_list = [x.group() for x in re.finditer(self.regexip,
> +                                                          standardoutput)]

You have this in two places now so I would make a helper method for it

Comment 16

[cmtalbert]

Comment on attachment 769668 [details] [diff] [review]
Updated patch

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

::: moznetwork/tests/test.py
@@ +57,5 @@
> +                # Generate a list of IPs by parsing the output of ip/ifconfig
> +                ip_list = [x.group() for x in re.finditer(self.regexip,
> +                                                          standardoutput)]
> +                # Check the IP returned by moznetwork is in the list
> +                self.assertTrue(ip in ip_list)

In order to make it easier for people to write tests in the future (since everyone will likely want to check ipconfig/ifconfig and verify that an ip is in the list, let's take Andrew's example and make a "verify_ip_in_list" function that performs the list comprehension and returns true or false. Then you can just assert on that function.  That way if we add some new platform (like android, b2g etc) we can add in more interesting commands to verify IPs and the tests can remain simple and pretty. r+ with that change.

Comment 17

[Anhad Jai Singh (:ffledgling)]

Attachment: Rough version of test with verify_ip_in_list() method

I've moved the repeated code out of the test class into it's own method, as suggested. Please take a look and tell me if this is what you had in mind, or if something else is required. Thanks!

Comment 18

[William Lachance (:wlach)]

Comment on attachment 771195 [details] [diff] [review]
Rough version of test with verify_ip_in_list() method

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

LGTM, assuming the test runs and passes as expected

Comment 19

[Andrew Halberstadt [:ahal]]

Comment on attachment 771195 [details] [diff] [review]
Rough version of test with verify_ip_in_list() method

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

Likewise, f+

Comment 20

[Andrew Halberstadt [:ahal]]

I pushed this to try to make sure that the ifconfig/ipconfig parsing works on all platforms:

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

Comment 21

[Andrew Halberstadt [:ahal]]

In that last try run I copied test-manifest.ini over which resulted in tests hitting missing file errors due to m-c not being up to date. Pushed a try run where only moznetwork tests are being enabled:
https://tbpl.mozilla.org/?tree=Try&rev=0726ae536dae

Comment 22

[Andrew Halberstadt [:ahal]]

https://github.com/mozilla/mozbase/commit/970a9b6c4b0118a24d9339edf0be95ec7fa37a6d