-
Notifications
You must be signed in to change notification settings - Fork 34
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
Adding more tests/ changing errors for REST calls #95
Adding more tests/ changing errors for REST calls #95
Conversation
Codecov Report
@@ Coverage Diff @@
## main #95 +/- ##
=======================================
Coverage 90.65% 90.66%
=======================================
Files 301 301
Lines 51052 51065 +13
Branches 6567 6566 -1
=======================================
+ Hits 46282 46298 +16
Misses 2836 2836
+ Partials 1934 1931 -3
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
@@ -122,7 +122,7 @@ def get_ems_destination(self, name): | |||
fields = 'name,type,destination,filters.name' | |||
query = dict(name=name, fields=fields) | |||
record, error = rest_generic.get_one_record(self.rest_api, api, query) | |||
self.fail_on_error(error) | |||
self.fail_on_error(error, 'GET') |
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.
I think it would look better as
self.fail_on_error(error, 'fetching EMS destinations for %s' % name)
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.
Agreed. Let me update it first.
@@ -145,12 +145,12 @@ def create_ems_destination(self): | |||
'filters': self.generate_filters_list(self.parameters['filters']) | |||
} | |||
dummy, error = rest_generic.post_async(self.rest_api, api, body) | |||
self.fail_on_error(error) | |||
self.fail_on_error(error, 'POST') |
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.
and then
self.fail_on_error(error, 'creating EMS destinations for %s' % name)
Good for me, but if you want to improve error messages, I would made them more end user friendly. |
SUMMARY
Adding few more tests for na_ontap_ems_destination and changing error messages for REST.
ISSUE TYPE
COMPONENT NAME
na_ontap_ems_destination
ADDITIONAL INFORMATION
I decided to add a few more tests to increase test coverage.
Also decided that tests for REST methods could be a bit more helpful if they would specify the method that was called.
While working on this PR I realized that the loop responsible for running helper methods should actually have only one
elif
- no scenario exists whereself.na_helper.changed
is True and neithercd_action
normodify
is in one of the values listed right now.