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.

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

Add tests #1

Merged
merged 9 commits into from
Feb 1, 2017
Merged

Add tests #1

merged 9 commits into from
Feb 1, 2017

Conversation

Natim
Copy link
Contributor

@Natim Natim commented Jan 31, 2017

Copy link

@leplatrem leplatrem left a comment

Choose a reason for hiding this comment

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

I found that the tests were a bit heavy to read :)

@@ -0,0 +1,20 @@
language: python
python: 2.7

Choose a reason for hiding this comment

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

You can do better ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should also work with Py3

@@ -0,0 +1,151 @@
import mock
import pytest
from portier.client import discover_keys, get_verified_email

Choose a reason for hiding this comment

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

nit: separation between imports


assert mocked_requests.get.call_count == 2
mocked_requests.get.assert_any_call(
"http://broker-url.tld//.well-known/openid-configuration")

Choose a reason for hiding this comment

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

Double slash ?

)
keys = discover_keys("http://broker-url.tld/", cache)

assert isinstance(keys, dict)

Choose a reason for hiding this comment

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

I think that part doesn't have to be in the with


def test_discover_key_raises_a_value_error_if_keys_not_found():
cache = mock.MagicMock()
cache.get.return_value = None

Choose a reason for hiding this comment

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

Declare an empty_cache once on top somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You want me to create a class with a setup phase right?

{"kid": "abc",
"e": "KHTPnNouCvwROWeIWQkJiw",
"n": "ZgKgqvEo_GZMamwy293IvA",
"alg": "RS256"}]},

Choose a reason for hiding this comment

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

I have the feeling that tests could be a lot easier to read if those crazy values would be declared once on top (eg. warm_cache, TOKEN, etc...)

}
result = get_verified_email("http://broker-url.tld/",
"eyJraWQiOiAiYWJjIn0.foo.bar",
"audience", "issuer", cache)

Choose a reason for hiding this comment

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

Add kwargs names?

@Natim
Copy link
Contributor Author

Natim commented Feb 1, 2017

I found that the tests were a bit heavy to read :)

Is it better now?

Copy link

@leplatrem leplatrem left a comment

Choose a reason for hiding this comment

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

Yeah! better :)

@Natim Natim merged commit f0b6ae1 into master Feb 1, 2017
@Natim Natim deleted the tests branch February 1, 2017 10:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants