-
Notifications
You must be signed in to change notification settings - Fork 7
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
Initial set of CI tests together with a ephemeral MongoDB container #117
Conversation
If we want to add a development mode, it should be added in the config::get_settings() method (similar to where we are checking for running under pytest)
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.
Thank you for adding these unit tests!!!
None of my comments are blocking. I leave to you whether to defer suggestions to a future PR or not. OK to merge.
# This is to make sure that the client is using the same event loop as the rest of the application | ||
client.get_io_loop = asyncio.get_event_loop |
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.
Nice! I think this was defeating me in some rounds of debugging earlier.
) as ac: | ||
response = await ac.get("/v1/beamline/does-not-exist") | ||
assert response.status_code == 404 | ||
assert response.json() == {"detail": "Beamline 'does-not-exist' does not exist"} |
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.
Depending on how strictly this response will be relied upon downstream--i.e., is this part of the API ?--consider a partial match here. E.g., 'does-not-exist' in response.text
assert response.json() == {"detail": "Beamline 'does-not-exist' does not exist"} | |
assert response.json() == {"detail": "Beamline 'does-not-exist' does not exist"} |
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 presume that you meant to enter a suggestion here and not the same code again 😄
Co-authored-by: Padraic Shafer <[email protected]>
This is to add some CI testing - it is not an exhaustive list, just submitting the PR before it gets really long!
The last run of the CI pipeline against this branch can be found here : https://github.com/stuartcampbell/nsls2api/actions/runs/11523289207
or in the test list below 😄