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

test: add integration tests #40

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from
Open

test: add integration tests #40

wants to merge 3 commits into from

Conversation

akash2237778
Copy link
Contributor

Description

Added Integration tests that test the functionality of each endpoint.

Fixes #39

@uniqueg uniqueg changed the title test: add integeration tests test: add integration tests May 18, 2021
Copy link
Member

@uniqueg uniqueg left a comment

Choose a reason for hiding this comment

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

This looks good - thanks a lot! Some minor refactoring to clean this up a little and would be good to add some tests for 400 (for all endpoints) and 404 (for GET endpoints only) responses. I'm also wondering why Travis CI wasn't triggered.... Any idea?

base_url = "http://localhost:8080/ga4gh/drs/v1"
data_objects_path = "data_objects.json"
headers = {
'accept': 'application/json',
Copy link
Member

Choose a reason for hiding this comment

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

I think it should be Accept with a capital A: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Accept

'accept': 'application/json',
'Content-Type': 'application/json'
}
payload_objects = json.dumps({
Copy link
Member

Choose a reason for hiding this comment

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

Can some object from "data_objects.json" perhaps be reused here instead of hardcoding another 50 lines of payload in here?

"alias1"
]
})
payload_service_info = json.dumps({
Copy link
Member

Choose a reason for hiding this comment

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

Can the payload JSON perhaps be externalized so that it can be easily reused and keep this module cleaner? See here, e.g.: https://github.com/elixir-cloud-aai/trs-filer/blob/dev/tests/mock_data.py

endpoint = "/objects/" + object_id
response = requests.request("DELETE", base_url + endpoint, headers=headers)
assert response.status_code == 200
assert response.text.replace("\"", "").replace("\n", "") == object_id
Copy link
Member

Choose a reason for hiding this comment

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

Would be good to add test cases for some errors as well, especially 404 and 400. Eventually we should cover 401 and 403 as well, but we can do that once we have a better idea of how controlled access management will look like

.travis.yml Outdated
@@ -7,7 +7,10 @@ language: python
# this ensures that the pipeline is triggered for internal pushes,
# PRs from forks and pushes to existing PRs from forks
if: (type == push) OR (type == pull_request AND fork == true)

branches:
Copy link
Member

Choose a reason for hiding this comment

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

Oh, this is why it didn't trigger a Travis run. Why did you add that? I think we would like to run tests always, for each PR. If you would like for specific tests (like integration tests), you should make use of different stages or a job matrix, then introduce conditions for branches separately for each stage/job. It should also be possible to use conditions on the exact commands, but the syntax is not great and it can quickly become unreadable for more complicated files. In any case, I don't see a good reason why we should not run the Travis CI for all PRs as well, the tests should be fast enough...

Copy link
Member

Choose a reason for hiding this comment

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

(so I would just remove this block again unless there is a convincing reason to keep it)

Added integration tests for responses  and  & created mock_data.py to avoid hard coding payload at integration_tests.py
Minor fix: mock_data
@akash2237778
Copy link
Contributor Author

akash2237778 commented May 22, 2021

Made required changes. But still not sure about tests for response 400, although added for POST /objects and POST /service-info. Some endpoints never return response 400 e.g. server.py.

I think there is a need to add tests for response 404 at DELETE /objects/{object_id}/access/{access_id} and DELETE /objects/{object_id} endpoints too.

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.

Add integration tests
2 participants