-
Notifications
You must be signed in to change notification settings - Fork 30
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
Test add-streaming-sdk #50
Conversation
Starting demo at: https://anbox-cloud-io-canonical-web-and-design-pr-50.run.demo.haus/ |
Codecov Report
|
7d9fb7c
to
9d51403
Compare
1435fa2
to
5ee2618
Compare
872c6c2
to
a9fbf15
Compare
a9fbf15
to
7ef8ef1
Compare
fe2a043
to
46ef183
Compare
tests/test_macaroons.py
Outdated
|
||
openid_macaroon = MacaroonRequest(caveat_id=self.caveat_id) | ||
self.assertEqual( | ||
openid_macaroon.getExtensionArgs(), self.request_result |
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.
Here are you simply testing internal logic of the MacaroonRequest
library? We shouldn't test other people's code directly - that code should be tested by them, but even if it hasn't we should test it works through testing our usage of their module, rather than the module itself.
Again, I'd suggest removing this test.
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.
Well, it's not really a library, it's just a module I took from snapcraft. It was not unit tested on that project.
tests/test_api.py
Outdated
) | ||
|
||
# Make API call | ||
response = _api_request("/1.0/login", method="POST", json=data) |
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.
If we think about what you're testing here - I think you're testing 2 things:
- That if you mock the API endpoint
/1.0/login
withresponses
, that it genuinely returns the data in the mock - That the
_api_request
method successfully adds{ANBOXCLOUD_API_BASE}
to the beginning of the URL it's passed, and that it returns the content ofresponse.json()
To me, neither of these are particularly consequential. The first is effectively just making sure the responses
library works, and the second is some very small internal logic of _api_request
. I think if you're gonna add tests you should think about testing something more consequential.
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 don't know how to improve this. Should I just do it with VCR?
46ef183
to
8badada
Compare
8badada
to
434e757
Compare
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.
OK works for me in Chrome. In Firefox I get an error, as mentioned above. I think it's fine to merge this so we can do further testing though, since it's only accessible to people who know it's there and have invite codes.
Filed: #62 |
Done
QA:
./run serve
A few considerations:
Issue / Cards: