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

Added tests for delete participant endpoint (fixes #193) #194

Merged

Conversation

stanislawK
Copy link
Contributor

  • make start
  • make test

@magul magul changed the title Added tests for delete participant endpoint(fixes #193) Added tests for delete participant endpoint (fixes #193) Nov 10, 2019
@magul
Copy link
Member

magul commented Nov 10, 2019

Can you, @stanislawK, merge back master here as #185 was merged and it would be great to see these 2 tests passing.

Copy link
Member

@w1stler w1stler left a comment

Choose a reason for hiding this comment

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

Minor stuff, altough it's not crucial.

):
"""Test get detais of non-existent participant"""
rv = client.get(
"""Test get and delete non-existent participant"""
Copy link
Member

Choose a reason for hiding this comment

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

Period at the end.

Copy link
Member

Choose a reason for hiding this comment

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

Non-existing

"""Test get participant details for not logged in user."""
@pytest.mark.parametrize("method", ["get", "delete"])
def test_get_delete_participant_unauthorized(client, add_participants, method):
"""Test get and delete participant with not logged in user."""
Copy link
Member

Choose a reason for hiding this comment

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

I would remove get from all the scenarios and ensure that we have explicit one for testing get method. Others will be eg.: "Test delete participant when user logged-in / logged-out".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it would make tests less readable, especially if we would merge #191. Following magul suggesion I introduced there logged in client fixture. So e.g. for testing get method I will have to inject two different test clients, which could be confussing.

@OtisRed
Copy link
Contributor

OtisRed commented Dec 7, 2019

@magul or @w1stler please respond and/or approve because this one seems like it could be merged

@w1stler w1stler merged commit 380bebe into CodeForPoznan:master Dec 9, 2019
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.

5 participants