Merge pull request #245 from jathanism/nd_match-fix33

Another bug fix to NetDevices.match(), with unit tests to make sure!

docs/changelog.rst

@@ -2,6 +2,18 @@
 Changelog
 =========
 
+.. _v1.5.5:
+
+1.5.5
+=====
+
+Bug Fixes
+---------
+
++ Bugfix in `~trigger.netdevices.NetDevices.match()` where keyword arguments
+  were not properly filtering out devices that matched, sometimes resulting in
+  a confusing union of matching devices.
+
 .. _v1.5.4:
 
 1.5.4

tests/data/netdevices.xml

@@ -38,4 +38,40 @@
       <serialNumber>987654321</serialNumber>
       <site>LAB</site>
    </device>
+   <device nodeName="test2-abc.net.aol.com">
+      <adminStatus>PRODUCTION</adminStatus>
+      <assetID>0000012345</assetID>
+      <authMethod>tacacs</authMethod>
+      <barcode>0101010101</barcode>
+      <budgetCode>1234578</budgetCode>
+      <budgetName>Data Center</budgetName>
+      <coordinate>16ZZ</coordinate>
+      <deviceType>SWITCH</deviceType>
+      <enablePW>boguspassword</enablePW>
+      <lastUpdate>2010-07-19 19:56:32.0</lastUpdate>
+      <layer2>1</layer2>
+      <layer3>1</layer3>
+      <layer4>1</layer4>
+      <lifecycleStatus>INSTALLED</lifecycleStatus>
+      <loginPW></loginPW>
+      <make>EX-SERIES SWITCH</make>
+      <manufacturer>JUNIPER</manufacturer>
+      <model>EX4200-48T</model>
+      <nodeName>test2-abc.net.aol.com</nodeName>
+      <onCallEmail>nobody@aol.net</onCallEmail>
+      <onCallID>17</onCallID>
+      <onCallName>Data Center</onCallName>
+      <owningTeam>Data Center</owningTeam>
+      <OOBTerminalServerConnector>C</OOBTerminalServerConnector>
+      <OOBTerminalServerFQDN>ts1.oob.aol.com</OOBTerminalServerFQDN>
+      <OOBTerminalServerNodeName>ts1</OOBTerminalServerNodeName>
+      <OOBTerminalServerPort>5</OOBTerminalServerPort>
+      <OOBTerminalServerTCPPort>5005</OOBTerminalServerTCPPort>
+      <operationStatus>MONITORED</operationStatus>
+      <owner>12345678 - Network Engineering</owner>
+      <projectName>Test Lab</projectName>
+      <room>CR10</room>
+      <serialNumber>987654321</serialNumber>
+      <site>LAB</site>
+   </device>
 </NetDevices>

tests/test_netdevices.py

@@ -28,6 +28,7 @@
 
 # Constants
 DEVICE_NAME = 'test1-abc.net.aol.com'
+DEVICE2_NAME = 'test2-abc.net.aol.com'
 NETDEVICE_DUMP_EXPECTED = \
 '\n\tHostname:          test1-abc.net.aol.com\n\tOwning Org.:       12345678 - Network Engineering\n\tOwning Team:       Data Center\n\tOnCall Team:       Data Center\n\n\tVendor:            Juniper (JUNIPER)\n\tMake:              M40 INTERNET BACKBONE ROUTER\n\tModel:             M40-B-AC\n\tType:              ROUTER\n\tLocation:          LAB CR10 16ZZ\n\n\tProject:           Test Lab\n\tSerial:            987654321\n\tAsset Tag:         0000012345\n\tBudget Code:       1234578 (Data Center)\n\n\tAdmin Status:      PRODUCTION\n\tLifecycle Status:  INSTALLED\n\tOperation Status:  MONITORED\n\tLast Updated:      2010-07-19 19:56:32.0\n\n'
 
@@ -43,13 +44,14 @@ class TestNetDevicesWithAcls(unittest.TestCase):
     """
     def setUp(self):
         self.nd = NetDevices()
-        self.nodename = self.nd.keys()[0]
-        self.device = self.nd.values()[0]
+        self.device = self.nd[DEVICE_NAME]
+        self.device2 = self.nd[DEVICE2_NAME]
+        self.nodename = self.device.nodeName
         self.device.explicit_acls = set(['test1-abc-only'])
 
     def test_basics(self):
         """Basic test of NetDevices functionality."""
-        self.assertEqual(len(self.nd), 1)
+        self.assertEqual(len(self.nd), 2)
         self.assertEqual(self.device.nodeName, self.nodename)
         self.assertEqual(self.device.manufacturer, 'JUNIPER')
 
@@ -70,22 +72,34 @@ def test_find(self):
 
     def test_all(self):
         """Test the all() method."""
-        expected = [self.device]
-        self.assertEqual(expected, self.nd.all())
+        expected = [self.device, self.device2]
+        self.assertEqual(sorted(expected), sorted(self.nd.all()))
 
     def test_search(self):
         """Test the search() method."""
         expected = [self.device]
-        self.assertEqual(expected, self.nd.search(self.nodename))
-        self.assertEqual(expected, self.nd.search('17', field='onCallID'))
-        self.assertEqual(expected, self.nd.search('juniper', field='vendor'))
+        self.assertEqual([self.device], self.nd.search(self.nodename))
+        self.assertEqual(self.nd.all(), self.nd.search('17', field='onCallID'))
 
     def test_match(self):
         """Test the match() method."""
-        expected = [self.device]
-        self.assertEqual(expected, self.nd.match(nodename=self.nodename))
-        self.assertEqual(expected, self.nd.match(vendor='juniper'))
-        self.assertNotEqual(expected, self.nd.match(vendor='cisco'))
+        self.assertEqual([self.device], self.nd.match(nodename=self.nodename))
+        self.assertEqual(self.nd.all(), self.nd.match(vendor='juniper'))
+        self.assertEqual([], self.nd.match(vendor='cisco'))
+
+    def test_multiple_filter_match(self):
+        """Test that passing multiple kwargs filters properly."""
+        # There should be only one Juniper router.
+        self.assertEqual(
+            self.nd.match(nodename='test1-abc'),
+            self.nd.match(vendor='juniper', devicetype='router')
+        )
+
+        # And only one Juniper switch.
+        self.assertEqual(
+            self.nd.match(nodename='test2-abc'),
+            self.nd.match(vendor='juniper', devicetype='switch')
+        )
 
     def test_match_with_null_value(self):
         """Test the match() method when attr value is ``None``."""
@@ -102,7 +116,7 @@ def test_match_with_null_value(self):
         self.assertEqual(expected, self.nd.match(SITE='NONE'))
 
     def tearDown(self):
-        NetDevices._Singleton = None
+        _reset_netdevices()
 
 
 class TestNetDevicesWithoutAcls(unittest.TestCase):
@@ -133,12 +147,12 @@ class TestNetDeviceObject(unittest.TestCase):
     """
     def setUp(self):
         self.nd = NetDevices()
-        self.nodename = self.nd.keys()[0]
-        self.device = self.nd.values()[0]
+        self.device = self.nd[DEVICE_NAME]
+        self.nodename = self.device.nodeName
 
     def test_stringify(self):
         """Test casting NetDevice to string"""
-        expected = DEVICE_NAME
+        expected = self.nodename
         self.assertEqual(expected, str(self.device))
 
     def test_bounce(self):
@@ -197,6 +211,7 @@ def test_dump(self):
     def tearDown(self):
         _reset_netdevices()
 
+
 class TestVendorObject(unittest.TestCase):
     """Test Vendor object"""
     def setUp(self):
@@ -235,5 +250,6 @@ def test_determine_vendor(self):
         expected = 'cisco'
         self.assertEqual(expected, self.vendor.determine_vendor(self.mfr))
 
+
 if __name__ == "__main__":
     unittest.main()

trigger/__init__.py

@@ -1,4 +1,4 @@
-__version__ = (1, 5, 4)
+__version__ = (1, 5, 5)
 
 full_version = '.'.join(str(x) for x in __version__[0:3]) + \
                ''.join(__version__[3:])

trigger/netdevices/__init__.py

@@ -933,17 +933,17 @@ def map_attr(attr):
                 """Helper function for lower-to-regular attribute mapping."""
                 return self._all_field_names[attr.lower()]
 
-            # Use generators to keep filtering the iterator of devices.
+            # Use list comp. to keep filtering out the devices.
             for attr, val in kwargs.iteritems():
                 attr = map_attr(attr)
                 val = str(val).lower()
-                devices = (
+                devices = [
                     d for d in devices if (
                         val in str(getattr(d, attr, '')).lower()
                     )
-                )
+                ]
 
-            return list(devices)
+            return devices
 
         def get_devices_by_type(self, devtype):
             """