-
-
Notifications
You must be signed in to change notification settings - Fork 61
Add some tests for project and task routes #2346
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
Conversation
@@ -817,7 +817,7 @@ async def update_project_form( | |||
"project_id": project.id, | |||
}, | |||
) | |||
db.commit() |
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.
Good catch, but I'm not sure we really need db.commit() anywhere in the codebase to be honest:
- When an endpoint is called a transaction is started via a context manager
- Before the cursor / transaction is closed, the SQL is executed automatically
xls_file = BytesIO(updated_xls_content) | ||
xls_file.name = "form.xlsx" | ||
|
||
with patch( |
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.
While this patch is good, we do run the entire stack for these tests (db, api, odk central), so you could get the actual response rather than doing a patch π
|
||
contributor = data[0] | ||
assert contributor["user"] == admin_user.username | ||
assert contributor["contributions"] == 2 |
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.
It might be good to have this as >2
or something, as this could break in future if we added more tests that create task events
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.
This is really nice work - well done π
Test coverage increase 68% --> 72% π |
What type of PR is this? (check all applicable)
Related Issue
Describe this PR
Tests to: