Document self.assertEqual(expected, observed) pattern

Tests should use the following pattern to help reviewers:

  self.assertEqual(expected, observed)

This change documents it in effective_neutron.rst and corrects some test
modules[1] including nonobvious ones[2] as an example.

[1] neutron.tests.unit.agent.l3.test_.*
[2] neutron.tests.unit.agent.l3.test_router_processing_queue

Change-Id: I6d6363541e3fef6f66e808aab00507825205b209

diff --git a/doc/source/devref/effective_neutron.rst b/doc/source/devref/effective_neutron.rst
index 98bf8e9fb68138d33016623cf294f793c54d2a61..4cbea6e6febb78e395657eafa3f8edaa7433e4c5 100644 (file)
--- a/doc/source/devref/effective_neutron.rst
+++ b/doc/source/devref/effective_neutron.rst
@@ -100,6 +100,10 @@ For anything more elaborate, please visit the testing section.
          self.assertTrue(3 in [1, 2])
          # raise meaningless error: "AssertionError: False is not true"
 
+* Use the pattern "self.assertEqual(expected, observed)" not the opposite, it helps
+  reviewers to understand which one is the expected/observed value in non-trivial
+  assertions.
+
 Backward compatibility
 ~~~~~~~~~~~~~~~~~~~~~~
 

diff --git a/neutron/tests/unit/agent/l3/test_agent.py b/neutron/tests/unit/agent/l3/test_agent.py
index ea9409917c58b5f6b8bc4f20e93a52e854fb011e..d02ff30259a8a750a50d8b03475f9e1fb1af57d1 100644 (file)
--- a/neutron/tests/unit/agent/l3/test_agent.py
+++ b/neutron/tests/unit/agent/l3/test_agent.py
@@ -262,7 +262,7 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework):
             'gw_port': ex_gw_port}
         ri = l3router.RouterInfo(ns_id, router, **self.ri_kwargs)
         self.assertTrue(ri.ns_name.endswith(ns_id))
-        self.assertEqual(ri.router, router)
+        self.assertEqual(router, ri.router)
 
     def test_agent_create(self):
         l3_agent.L3NATAgent(HOSTNAME, self.conf)
@@ -283,15 +283,15 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework):
         if action == 'add':
             self.device_exists.return_value = False
             ri.internal_network_added(port)
-            self.assertEqual(self.mock_driver.plug.call_count, 1)
-            self.assertEqual(self.mock_driver.init_router_port.call_count, 1)
+            self.assertEqual(1, self.mock_driver.plug.call_count)
+            self.assertEqual(1, self.mock_driver.init_router_port.call_count)
             self.send_adv_notif.assert_called_once_with(ri.ns_name,
                                                         interface_name,
                                                         '99.0.1.9', mock.ANY)
         elif action == 'remove':
             self.device_exists.return_value = True
             ri.internal_network_removed(port)
-            self.assertEqual(self.mock_driver.unplug.call_count, 1)
+            self.assertEqual(1, self.mock_driver.unplug.call_count)
         else:
             raise Exception("Invalid action %s" % action)
 
@@ -347,8 +347,8 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework):
             ri._internal_network_added = mock.Mock()
             ri._set_subnet_arp_info = mock.Mock()
             ri.internal_network_added(port)
-            self.assertEqual(ri._snat_redirect_add.call_count, 1)
-            self.assertEqual(ri._internal_network_added.call_count, 2)
+            self.assertEqual(1, ri._snat_redirect_add.call_count)
+            self.assertEqual(2, ri._internal_network_added.call_count)
             ri._set_subnet_arp_info.assert_called_once_with(subnet_id)
             ri._internal_network_added.assert_called_with(
                 dvr_snat_ns.SnatNamespace.get_snat_ns_name(ri.router['id']),
@@ -397,10 +397,10 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework):
             router[l3_constants.FLOATINGIP_KEY] = fake_fip['floatingips']
         ri.external_gateway_added(ex_gw_port, interface_name)
         if not router.get('distributed'):
-            self.assertEqual(self.mock_driver.plug.call_count, 1)
-            self.assertEqual(self.mock_driver.init_router_port.call_count, 1)
+            self.assertEqual(1, self.mock_driver.plug.call_count)
+            self.assertEqual(1, self.mock_driver.init_router_port.call_count)
             if no_subnet and not dual_stack:
-                self.assertEqual(self.send_adv_notif.call_count, 0)
+                self.assertEqual(0, self.send_adv_notif.call_count)
                 ip_cidrs = []
                 gateway_ips = []
                 if no_sub_gw:
@@ -556,7 +556,7 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework):
         router[l3_constants.FLOATINGIP_KEY] = fake_fip['floatingips']
         ri.external_gateway_updated(ex_gw_port, interface_name)
         self.assertEqual(1, self.mock_driver.plug.call_count)
-        self.assertEqual(self.mock_driver.init_router_port.call_count, 1)
+        self.assertEqual(1, self.mock_driver.init_router_port.call_count)
         exp_arp_calls = [mock.call(ri.ns_name, interface_name,
                                    '20.0.0.30', mock.ANY)]
         if dual_stack:
@@ -810,8 +810,8 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework):
         ri.process_floating_ip_addresses.reset_mock()
         ri.process_floating_ip_nat_rules.assert_called_with()
         ri.process_floating_ip_nat_rules.reset_mock()
-        self.assertEqual(ri.external_gateway_added.call_count, 0)
-        self.assertEqual(ri.external_gateway_updated.call_count, 0)
+        self.assertEqual(0, ri.external_gateway_added.call_count)
+        self.assertEqual(0, ri.external_gateway_updated.call_count)
         ri.external_gateway_added.reset_mock()
         ri.external_gateway_updated.reset_mock()
 
@@ -825,8 +825,8 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework):
         ri.process(agent)
         ri.process_floating_ip_addresses.reset_mock()
         ri.process_floating_ip_nat_rules.reset_mock()
-        self.assertEqual(ri.external_gateway_added.call_count, 0)
-        self.assertEqual(ri.external_gateway_updated.call_count, 1)
+        self.assertEqual(0, ri.external_gateway_added.call_count)
+        self.assertEqual(1, ri.external_gateway_updated.call_count)
 
         # remove just the floating ips
         del router[l3_constants.FLOATINGIP_KEY]
@@ -840,12 +840,10 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework):
         del router[l3_constants.INTERFACE_KEY]
         del router['gw_port']
         ri.process(agent)
-        self.assertEqual(self.send_adv_notif.call_count, 1)
+        self.assertEqual(1, self.send_adv_notif.call_count)
         distributed = ri.router.get('distributed', False)
-        self.assertEqual(ri.process_floating_ip_addresses.called,
-                         distributed)
-        self.assertEqual(ri.process_floating_ip_nat_rules.called,
-                         distributed)
+        self.assertEqual(distributed, ri.process_floating_ip_addresses.called)
+        self.assertEqual(distributed, ri.process_floating_ip_nat_rules.called)
 
     @mock.patch('neutron.agent.linux.ip_lib.IPDevice')
     def _test_process_floating_ip_addresses_add(self, ri, agent, IPDevice):
@@ -960,8 +958,8 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework):
             ri.create_dvr_fip_interfaces(ext_gw_port)
             self.assertTrue(fip_gw_port.called)
             self.assertTrue(fips.called)
-            self.assertEqual(ri.fip_ns.agent_gateway_port,
-                             agent_gateway_port[0])
+            self.assertEqual(agent_gateway_port[0],
+                             ri.fip_ns.agent_gateway_port)
             self.assertTrue(ri.rtr_fip_subnet)
             self.assertEqual(1, agent.process_router_add.call_count)
 
@@ -1010,8 +1008,8 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework):
             ri.create_dvr_fip_interfaces(ext_gw_port)
             self.assertTrue(fip_gw_port.called)
             self.assertTrue(fips.called)
-            self.assertEqual(ri.fip_ns.agent_gateway_port,
-                             agent_gateway_port[0])
+            self.assertEqual(agent_gateway_port[0],
+                             ri.fip_ns.agent_gateway_port)
             self.assertTrue(ri.rtr_fip_subnet)
 
     def test_process_router_cent_floating_ip_add(self):
@@ -1050,14 +1048,14 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework):
         # IpTablesRule instances
         nat_rules_delta = [r for r in orig_nat_rules
                            if r not in ri.iptables_manager.ipv4['nat'].rules]
-        self.assertEqual(len(nat_rules_delta), 3)
+        self.assertEqual(3, len(nat_rules_delta))
         mangle_rules_delta = [
             r for r in orig_mangle_rules
             if r not in ri.iptables_manager.ipv4['mangle'].rules]
-        self.assertEqual(len(mangle_rules_delta), 1)
+        self.assertEqual(1, len(mangle_rules_delta))
         self._verify_snat_mangle_rules(nat_rules_delta, mangle_rules_delta,
                                        router)
-        self.assertEqual(self.send_adv_notif.call_count, 1)
+        self.assertEqual(1, self.send_adv_notif.call_count)
 
     def test_process_router_snat_enabled(self):
         agent = l3_agent.L3NATAgent(HOSTNAME, self.conf)
@@ -1077,14 +1075,14 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework):
         # IpTablesRule instances
         nat_rules_delta = [r for r in ri.iptables_manager.ipv4['nat'].rules
                            if r not in orig_nat_rules]
-        self.assertEqual(len(nat_rules_delta), 3)
+        self.assertEqual(3, len(nat_rules_delta))
         mangle_rules_delta = [
             r for r in ri.iptables_manager.ipv4['mangle'].rules
             if r not in orig_mangle_rules]
-        self.assertEqual(len(mangle_rules_delta), 1)
+        self.assertEqual(1, len(mangle_rules_delta))
         self._verify_snat_mangle_rules(nat_rules_delta, mangle_rules_delta,
                                        router)
-        self.assertEqual(self.send_adv_notif.call_count, 1)
+        self.assertEqual(1, self.send_adv_notif.call_count)
 
     def test_update_routing_table(self):
         # Just verify the correct namespace was used in the call
@@ -1121,7 +1119,7 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework):
         ri.router = router
         ri.process(agent)
         # send_ip_addr_adv_notif is called both times process is called
-        self.assertEqual(self.send_adv_notif.call_count, 2)
+        self.assertEqual(2, self.send_adv_notif.call_count)
 
     def _test_process_ipv6_only_or_dual_stack_gw(self, dual_stack=False):
         agent = l3_agent.L3NATAgent(HOSTNAME, self.conf)
@@ -1322,7 +1320,7 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework):
         ri.router = router
         ri.process(agent)
         # send_ip_addr_adv_notif is called both times process is called
-        self.assertEqual(self.send_adv_notif.call_count, 2)
+        self.assertEqual(2, self.send_adv_notif.call_count)
 
     def test_process_router_ipv6_interface_removed(self):
         agent = l3_agent.L3NATAgent(HOSTNAME, self.conf)
@@ -1529,7 +1527,7 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework):
                 mock.ANY, ri.router_id,
                 {fip_id: l3_constants.FLOATINGIP_STATUS_ERROR})
 
-            self.assertEqual(ri.iptables_manager._apply.call_count, 1)
+            self.assertEqual(1, ri.iptables_manager._apply.call_count)
 
     def test_handle_router_snat_rules_distributed_without_snat_manager(self):
         agent = l3_agent.L3NATAgent(HOSTNAME, self.conf)
@@ -1562,8 +1560,8 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework):
         for call in nat.mock_calls:
             name, args, kwargs = call
             if name == 'add_rule':
-                self.assertEqual(args, ('snat', '-j $float-snat'))
-                self.assertEqual(kwargs, {})
+                self.assertEqual(('snat', '-j $float-snat'), args)
+                self.assertEqual({}, kwargs)
                 break
 
     def test_handle_router_snat_rules_add_rules(self):
@@ -1613,7 +1611,7 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework):
         ri = l3router.RouterInfo(router['id'], router, **self.ri_kwargs)
 
         internal_ports = ri.router.get(l3_constants.INTERFACE_KEY, [])
-        self.assertEqual(len(internal_ports), 1)
+        self.assertEqual(1, len(internal_ports))
         internal_port = internal_ports[0]
 
         with mock.patch.object(ri, 'internal_network_removed'
@@ -1627,12 +1625,12 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework):
 
             ri.process(agent)
 
-            self.assertEqual(external_gateway_added.call_count, 1)
+            self.assertEqual(1, external_gateway_added.call_count)
             self.assertFalse(external_gateway_removed.called)
             self.assertFalse(internal_network_removed.called)
             internal_network_added.assert_called_once_with(internal_port)
-            self.assertEqual(self.mock_driver.unplug.call_count,
-                             len(stale_devnames))
+            self.assertEqual(len(stale_devnames),
+                             self.mock_driver.unplug.call_count)
             calls = [mock.call(stale_devname,
                                namespace=ri.ns_name,
                                prefix=l3_agent.INTERNAL_DEV_PREFIX)
@@ -1711,7 +1709,7 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework):
             'qrouter-bar', self.conf, agent.driver, agent.use_ipv6)
         ns.create()
         ns.delete()
-        self.assertEqual(self.mock_ip.netns.delete.call_count, 0)
+        self.assertEqual(0, self.mock_ip.netns.delete.call_count)
 
     def test_destroy_router_namespace_removes_ns(self):
         agent = l3_agent.L3NATAgent(HOSTNAME, self.conf)
@@ -1944,9 +1942,10 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework):
                 ns_manager.keep_router(r['id'])
         qrouters = [n for n in stale_namespace_list
                     if n.startswith(namespaces.NS_PREFIX)]
-        self.assertEqual(mock_router_ns.call_count, len(qrouters))
-        self.assertEqual(mock_snat_ns.call_count,
-            len(stale_namespace_list) - len(qrouters))
+        self.assertEqual(len(qrouters), mock_router_ns.call_count)
+        self.assertEqual(
+            len(stale_namespace_list) - len(qrouters),
+            mock_snat_ns.call_count)
 
         self.assertFalse(agent.namespaces_manager._clean_stale)
 
@@ -2017,8 +2016,8 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework):
 
         # check 2 internal ports are plugged
         # check 1 ext-gw-port is plugged
-        self.assertEqual(self.mock_driver.plug.call_count, 3)
-        self.assertEqual(self.mock_driver.init_router_port.call_count, 3)
+        self.assertEqual(3, self.mock_driver.plug.call_count)
+        self.assertEqual(3, self.mock_driver.init_router_port.call_count)
 
     def test_get_service_plugin_list(self):
         service_plugins = [p_const.L3_ROUTER_NAT]
@@ -2043,7 +2042,7 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework):
         )
         agent = l3_agent.L3NATAgent(HOSTNAME, self.conf)
 
-        self.assertEqual(agent.neutron_service_plugins, tuple())
+        self.assertEqual(tuple(), agent.neutron_service_plugins)
 
     def test_get_service_plugin_list_retried_max(self):
         raise_timeout = oslo_messaging.MessagingTimeout()

diff --git a/neutron/tests/unit/agent/l3/test_dvr_fip_ns.py b/neutron/tests/unit/agent/l3/test_dvr_fip_ns.py
index 951149f39fd1eb96bbfb60dd35ec0fced75a43bc..a87aa31243d6fed63298bc35d3c40bb57a59c24a 100644 (file)
--- a/neutron/tests/unit/agent/l3/test_dvr_fip_ns.py
+++ b/neutron/tests/unit/agent/l3/test_dvr_fip_ns.py
@@ -87,8 +87,8 @@ class TestDvrFipNs(base.BaseTestCase):
         device_exists.return_value = False
         self.fip_ns._gateway_added(agent_gw_port,
                                    mock.sentinel.interface_name)
-        self.assertEqual(self.driver.plug.call_count, 1)
-        self.assertEqual(self.driver.init_l3.call_count, 1)
+        self.assertEqual(1, self.driver.plug.call_count)
+        self.assertEqual(1, self.driver.init_l3.call_count)
         send_adv_notif.assert_called_once_with(self.fip_ns.get_name(),
                                                mock.sentinel.interface_name,
                                                '20.0.0.30',
@@ -144,7 +144,7 @@ class TestDvrFipNs(base.BaseTestCase):
 
         device = IPDevice()
         device.link.set_mtu.assert_called_with(2000)
-        self.assertEqual(device.link.set_mtu.call_count, 2)
+        self.assertEqual(2, device.link.set_mtu.call_count)
         device.route.add_gateway.assert_called_once_with(
             '169.254.31.29', table=16)
 

diff --git a/neutron/tests/unit/agent/l3/test_namespace_manager.py b/neutron/tests/unit/agent/l3/test_namespace_manager.py
index 228ffdad4a0e1060ad8cb2fbd468e8e98e3027dc..99cc3ae34adca5e1c2b1faacea856f84671f463b 100644 (file)
--- a/neutron/tests/unit/agent/l3/test_namespace_manager.py
+++ b/neutron/tests/unit/agent/l3/test_namespace_manager.py
@@ -46,13 +46,13 @@ class TestNamespaceManager(NamespaceManagerTestCaseFramework):
 
         ns_prefix, ns_id = self.ns_manager.get_prefix_and_id(
             namespaces.NS_PREFIX + router_id)
-        self.assertEqual(ns_prefix, namespaces.NS_PREFIX)
-        self.assertEqual(ns_id, router_id)
+        self.assertEqual(namespaces.NS_PREFIX, ns_prefix)
+        self.assertEqual(router_id, ns_id)
 
         ns_prefix, ns_id = self.ns_manager.get_prefix_and_id(
             dvr_snat_ns.SNAT_NS_PREFIX + router_id)
-        self.assertEqual(ns_prefix, dvr_snat_ns.SNAT_NS_PREFIX)
-        self.assertEqual(ns_id, router_id)
+        self.assertEqual(dvr_snat_ns.SNAT_NS_PREFIX, ns_prefix)
+        self.assertEqual(router_id, ns_id)
 
         ns_name = 'dhcp-' + router_id
         self.assertIsNone(self.ns_manager.get_prefix_and_id(ns_name))

diff --git a/neutron/tests/unit/agent/l3/test_router_processing_queue.py b/neutron/tests/unit/agent/l3/test_router_processing_queue.py
index 0e6287e27fea91f21535fea8d7793309f7564790..8f8db193689ceda20c984cab01c5b09092ce124f 100644 (file)
--- a/neutron/tests/unit/agent/l3/test_router_processing_queue.py
+++ b/neutron/tests/unit/agent/l3/test_router_processing_queue.py
@@ -49,10 +49,10 @@ class TestExclusiveRouterProcessor(base.BaseTestCase):
         master_2 = l3_queue.ExclusiveRouterProcessor(FAKE_ID_2)
         not_master_2 = l3_queue.ExclusiveRouterProcessor(FAKE_ID_2)
 
-        self.assertEqual(master._master, master)
-        self.assertEqual(not_master._master, master)
-        self.assertEqual(master_2._master, master_2)
-        self.assertEqual(not_master_2._master, master_2)
+        self.assertEqual(master, master._master)
+        self.assertEqual(master, not_master._master)
+        self.assertEqual(master_2, master_2._master)
+        self.assertEqual(master_2, not_master_2._master)
 
         master.__exit__(None, None, None)
         master_2.__exit__(None, None, None)
@@ -77,16 +77,16 @@ class TestExclusiveRouterProcessor(base.BaseTestCase):
 
     def test_data_fetched_since(self):
         master = l3_queue.ExclusiveRouterProcessor(FAKE_ID)
-        self.assertEqual(master._get_router_data_timestamp(),
-                         datetime.datetime.min)
+        self.assertEqual(datetime.datetime.min,
+                         master._get_router_data_timestamp())
 
         ts1 = datetime.datetime.utcnow() - datetime.timedelta(seconds=10)
         ts2 = datetime.datetime.utcnow()
 
         master.fetched_and_processed(ts2)
-        self.assertEqual(master._get_router_data_timestamp(), ts2)
+        self.assertEqual(ts2, master._get_router_data_timestamp())
         master.fetched_and_processed(ts1)
-        self.assertEqual(master._get_router_data_timestamp(), ts2)
+        self.assertEqual(ts2, master._get_router_data_timestamp())
 
         master.__exit__(None, None, None)