Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added delete tests #396

Closed
wants to merge 6 commits into from
Closed

Added delete tests #396

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
40f6db1
added additional conditions to DeleteHostsTestCase to test changes to…
aleccohan Aug 8, 2019
e6de4f2
Fine tuned tests to work with mac_addresses and ip_addresses
aleccohan Aug 8, 2019
d6f8476
tested changes with mock data and removed mock data from events.py
aleccohan Aug 12, 2019
37ae521
Merge remote-tracking branch 'origin' into added_delete_tests
aleccohan Aug 13, 2019
a698d7f
applied suggestions made, added test for timestamp and type, generate…
aleccohan Aug 14, 2019
1bbf69c
Merged with master
aleccohan Aug 14, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions app/events.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import logging
from datetime import datetime

from marshmallow import fields
from marshmallow import Schema
from marshmallow import validate
from app.validators import verify_uuid_format

logger = logging.getLogger(__name__)

Expand All @@ -13,5 +14,5 @@ class HostEvent(Schema):
type = fields.Str()


def delete(id):
return HostEvent(strict=True).dumps({"id": id, "timestamp": datetime.utcnow(), "type": "delete"}).data
def delete(host):
return HostEvent(strict=True).dumps({"id": host['id'], "timestamp": datetime.utcnow(), "type": "delete"}).data
33 changes: 32 additions & 1 deletion test_api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

host_wrapper.subscription_manager_id = host[1]
host_wrapper.satellite_id = host[1]
host_wrapper.bios_uuid = host[1]
host_wrapper.ip_addresses = ["10.0.0.2"]
host_wrapper.fqdn = host[2]
host_wrapper.mac_addresses = ["apple"]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

host_wrapper.external_id = host[1]
host_wrapper.facts = [{"namespace": "ns1", "facts": {"key1": "value1"}}]

response_data = self.post(HOST_URL, [host_wrapper.data()], 207)
Expand Down Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

rhel_machine_id = self.added_hosts[0].rhel_machine_id
subscription_manager_id = self.added_hosts[0].subscription_manager_id
satellite_id = self.added_hosts[0].satellite_id
bios_uuid = self.added_hosts[0].bios_uuid
ip_addresses = self.added_hosts[0].ip_addresses
mac_addresses = self.added_hosts[0].mac_addresses
fqdn = self.added_hosts[0].fqdn
external_id = self.added_hosts[0].external_id
account = self.added_hosts[0].account

url = HOST_URL + "/" + original_id

Expand All @@ -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]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"])

assert rhel_machine_id in m.events[0]
assert subscription_manager_id in m.events[0]
assert satellite_id in m.events[0]
assert bios_uuid in m.events[0]
assert fqdn in m.events[0]
assert external_id in m.events[0]
assert account in m.events[0]

for ip in ip_addresses:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

assert ip in m.events[0]

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

# Try to get the host again
response = self.get(url, 200)

Expand Down