-
Notifications
You must be signed in to change notification settings - Fork 13
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.
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
265 order in cleanup function #267
Conversation
tests/clients/test_ngsi_v2_iota.py
Outdated
device.add_command(DeviceCommand(name="dummy_cmd")) | ||
self.client.post_device(device=device) | ||
clear_context_broker(settings.CB_URL,self.fiware_header) | ||
self.assertRaises(requests.HTTPError,clear_iot_agent, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SystemsPurge we prefer to write assertRaises
this way:
with self.assertRaises(requests.HTTPError):
clear_iot_agent(settings.IOTA_URL, self.fiware_header)
I have commited
tests/clients/test_ngsi_v2_iota.py
Outdated
clear_all(fiware_header=self.fiware_header, | ||
cb_url=settings.CB_URL, | ||
iota_url=settings.IOTA_JSON_URL) | ||
except requests.HTTPError: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why must this except
be added? I think in clear_all
the oder of the clear functions is correct. And here by capturing all HTTPError, it will show "Already cleared" even if the server is not reachable. Could you refine the check for exception? Otherwise, I would suggest to roll it back
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@djs0109 Since the teardown method is called after every test, including the one where the order of the cleanup function is tested, the teardown is guaranteed to raise an error and fail the test, exactly because the agent and broker get cleared in the last test. It's a problem that does not happen if you dont run the last test. For that same reason, the self.teardown() call at the end of test_service_group should also be removed. One way to avoid the try except block is to have the test in its own class with its own teardown and setup methods, does that sound better ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SystemsPurge OK, I understand now. Thanks for explaining. I actually found out that if the registration is deleted, the device can never be deleted via the API ... To make it stable, I would suggest that we can add a flag in clear_context_broker to control whether to clear registrations. By default, we can set it to false, i.e. not to clear regestrations. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@djs0109 That's convenient, should solve the order issue completely since apparently it only fails to delete a device ( after deleting registration ) if the device has a command. I will refine the test and check whether a changed clear_context_broker method still raises an error in that case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, sounds great :)
8e27c6e
to
fbc7187
Compare
…nto 265-order-in-cleanup-function # Conflicts: # tests/clients/test_ngsi_v2_iota.py
# Conflicts: # filip/utils/cleanup.py # tests/clients/test_ngsi_v2_iota.py
Closes #265. Test involves clear methods that are subsequently used by the teardown method, so i added a try , except at the teardown but maybe someone should have a better look into that.