Added delete tests #396

aleccohan · 2019-08-13T18:46:54Z

Added new delete tests to match with the changes made to the delete method.

… delete_by_id method

Glutexo

I have to request some changes here as the test is can easily yield false positives. I also need to ask, whether the behavior described by this test is the intended one, or whether it’s just derived from the actual workings of #392.

Glutexo · 2019-08-14T10:51:17Z

test_api.py

            host_wrapper.fqdn = host[2]
+            host_wrapper.mac_addresses = ["apple"]


This is nice. 😀 It’s not a valid MAC address though. It shouldn’t be a problem though, because we are not validating the format.

Glutexo · 2019-08-14T10:52:22Z

test_api.py

@@ -1110,7 +1110,14 @@ def create_hosts(self):
            host_wrapper.account = ACCOUNT
            host_wrapper.display_name = host[0]
            host_wrapper.insights_id = host[1]
+            host_wrapper.rhel_machine_id = host[1]


All the IDs are the same. This can lead to false positives and in your implementation below, it actually does. Your assert assert rhel_machine_id in m.events[0] would pass even if the value weren’t actually there, because it’s the same as insights_id.

Glutexo · 2019-08-14T11:02:22Z

test_api.py

@@ -1210,6 +1217,16 @@ def test_invalid_host_id(self):
 class DeleteHostsTestCase(PreCreatedHostsBaseTestCase):
    def test_create_then_delete(self):
        original_id = self.added_hosts[0].id
+        insights_id = self.added_hosts[0].insights_id


Can’t we just use the values directly? When it was just the original_id, it was ok, but now its just too many lines without much added value.

Glutexo · 2019-08-14T11:49:01Z

test_api.py

@@ -1227,7 +1244,21 @@ def __call__(self, e):
        with unittest.mock.patch("api.host.emit_event", new=MockEmitEvent()) as m:
            self.delete(url, 200, return_response_as_json=False)
            assert original_id in m.events[0]
-
+            assert insights_id in m.events[0]


These assertions are just vaguely testing the message format. The message here (m.events[0]) is a str and by these in asserts you are just checking that the value is present somewhere in the string. This approach has several flaws:

JSON values are not plain text and escaping may occur. Although for UUIDs this doesn’t matter and so it doesn’t for your non-arbitrary safe values.

The expected value may occur under a different key, it may be just a subset of a longer string etc. If it appears anywhere, the test passes.

Because you used the same value for all the IDs, you aren’t actually even checking that all values from the different fields are there.

The only way to do this properly I think is to deserialize the JSON message, check that it’s a dict and then check for the fields and their values. I’d just add to that that it’d be nice to check not only that there are right values under right keys, but also whether there isn’t anything more, a sort of negative test.

event = json.loads(m.events[0]) self.assertIsInstance(event, dict) expected_keys = { "timestamp", "type", "id", "account", "insights_id", "rhel_machine_id", "subscription_manager_id", "satellite_id", "bios_uuid", "ip_addresses", "fqdn", "mac_addresses", "external_id", } self.assertEqual(set(event.keys()), expected_keys) self.assertEqual(original_id, event["id"]) self.assertEqual(insights_id, event["insights_id"]) self.assertEqual(rhel_machine_id, event["rhel_machine_id"]) self.assertEqual(subscription_manager_id, event["subscription_manager_id"]) self.assertEqual(satellite_id, event["satellite_id"]) self.assertEqual(bios_uuid, event["bios_uuid"]) self.assertEqual(fqdn, event["fqdn"]) self.assertEqual(external_id, event["external_id"]) self.assertEqual(account, event["account"])

Glutexo · 2019-08-14T15:03:52Z

test_api.py

+            assert external_id in m.events[0]
+            assert account in m.events[0]
+
+            for ip in ip_addresses:


Not only this has the same problem as mentioned in my previous comment, but it’s lacking the negative test too. The test verifies whether all the expected IP addresses are there, but not whether there isn’t another IP that shouldn’t be there. This applies to the MAC address test below too.

Since the lists are encoded as ordered all the way around, it’s safe to plainly compare them.

self.assertEqual(ip_addresses, event["ip_addresses"])

If you’d want to compare them regardless of their order, you could do it like this:

from collections import Counter self.assertEqual(Counter(ip_addresses), Counter(event["ip_addresses"]))

But that’s an unnecessary complexity here as these are always lists and never sets.

Glutexo · 2019-08-14T15:11:24Z

test_api.py

+
+            for mac in mac_addresses:
+                assert mac in m.events[0]
+


You’re testing for all canonical facts and the account number. What fields are actually required in the message though? In the tickets I saw only requests for account number (RHCLOUD-1561) and machine ID (RHCLOUD-1995). But it makes sense to include all the canonical facts.

There is no check though to the type and timestamp fields.

Yeah, The only fields required in the message are the account number and machine ID. Derek suggested that I add all of the canonical facts in case someone else in the future needed them for something.

That makes perfect sense! It’d be nice to have this solution documented somewhere, at least in a JIRA ticket comment.

…d unique UUIDs for each variable that needed it

dehort

Looking good.

I'd like to have this change included in PR#392 or vice versa.

dehort · 2019-08-14T21:16:32Z

test_api.py

+            }
+            self.assertEqual(set(event.keys()), expected_keys)
+
+            self.assertEqual(self.added_hosts[0].timestamp, event["timestamp"])


The HostWrapper does not have a timestamp field. I think its ok if you check for the existence of the timestamp field.

> self.assertEqual(self.added_hosts[0].timestamp, event["timestamp"]) E AttributeError: 'HostWrapper' object has no attribute 'timestamp'

It’s also possible to patch the datetime.utcnow method. Like that it’s even possible to check its format.

@unittest.mock.patch("app.events.datetime", **{"utcnow.return_value": datetime.utcnow()}) def test_create_then_delete(self, datetime_mock): … with unittest.mock.patch("api.host.emit_event", new=MockEmitEvent()) as m: … timestamp_iso = datetime_mock.utcnow.return_value.isoformat() self.assertEqual(f"{timestamp_iso}+00:00", event["timestamp"]) …

dehort · 2019-08-14T21:18:09Z

test_api.py

+            self.assertEqual(set(event.keys()), expected_keys)
+
+            self.assertEqual(self.added_hosts[0].timestamp, event["timestamp"])
+            self.assertEqual(self.added_hosts[0].type, event["type"])


The HostWrapper does not have a type field. You can verify that the "type" field is set to "delete".

> self.assertEqual(self.added_hosts[0].type, event["type"]) E AttributeError: 'HostWrapper' object has no attribute 'type'

Suggested change

self.assertEqual(self.added_hosts[0].type, event["type"])

self.assertEqual("delete", event["type"])

Glutexo · 2019-08-16T09:17:40Z

This can be closed, now when it’s merged into #392, right?

aleccohan · 2019-08-16T12:20:47Z

Yeah, this can be closed! I merged it in.

aleccohan added 3 commits August 8, 2019 11:47

added additional conditions to DeleteHostsTestCase to test changes to…

40f6db1

… delete_by_id method

Fine tuned tests to work with mac_addresses and ip_addresses

e6de4f2

tested changes with mock data and removed mock data from events.py

d6f8476

aleccohan requested a review from Glutexo August 13, 2019 18:46

Merge remote-tracking branch 'origin' into added_delete_tests

37ae521

aleccohan self-assigned this Aug 13, 2019

Glutexo requested changes Aug 14, 2019

View reviewed changes

aleccohan added 2 commits August 14, 2019 13:46

applied suggestions made, added test for timestamp and type, generate…

a698d7f

…d unique UUIDs for each variable that needed it

Merged with master

1bbf69c

dehort requested changes Aug 14, 2019

View reviewed changes

aleccohan mentioned this pull request Aug 15, 2019

Added new elements to delete event #392

Merged

aleccohan closed this Aug 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added delete tests #396

Added delete tests #396

aleccohan commented Aug 13, 2019

Glutexo left a comment

Glutexo Aug 14, 2019

Glutexo Aug 14, 2019

Glutexo Aug 14, 2019

Glutexo Aug 14, 2019

Glutexo Aug 14, 2019

Glutexo Aug 14, 2019

aleccohan Aug 14, 2019

Glutexo Aug 15, 2019

dehort left a comment

dehort Aug 14, 2019

Glutexo Aug 15, 2019

Glutexo Aug 15, 2019

dehort Aug 14, 2019

Glutexo Aug 15, 2019

Glutexo Aug 15, 2019

Glutexo commented Aug 16, 2019

aleccohan commented Aug 16, 2019

		host_wrapper.fqdn = host[2]
		host_wrapper.mac_addresses = ["apple"]

	self.assertEqual(self.added_hosts[0].type, event["type"])
	self.assertEqual("delete", event["type"])

Added delete tests #396

Added delete tests #396

Conversation

aleccohan commented Aug 13, 2019

Glutexo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dehort left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Glutexo commented Aug 16, 2019

aleccohan commented Aug 16, 2019