-
Notifications
You must be signed in to change notification settings - Fork 3
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 Unit Tests #112
Initial Unit Tests #112
Conversation
|
||
- name: Python unit testing | ||
working-directory: ./backend/ | ||
run: uv run 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.
A question, will we eventually want to run a percentage check for unit test as well? Or do you see you see this as something we might add later on. I see it as the latter but would like to know your opinion.
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.
Yeah, eventually. Right now we're at some super low percentage. Sadness. But yes, something later on.
@@ -0,0 +1,58 @@ | |||
import json |
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.
Do you think the refactor work I did for src/external/lambdas/helpers/s3_file_upload_helper.py
moving someplace like src/storage/cloud_upload.py
or something of that nature?
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 wasn't thinking storage
, but in documents
. So, src/documents/new_document.py
, upload_document.py
, or something like that.
@@ -0,0 +1,63 @@ | |||
import json | |||
import logging |
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.
Shall I also include adding logging over our print statements? I thought we had a story for this but I don't mind the approach of refactoring as we go through testing either.
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.
If you have the presence of mind to convert to logging
, yes, please do it.
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.
Looks good! I left some comments that were more questions for my understanding.
Changes proposed in this pull request
Changed the organization of the code. In
external
, I added anaws
module and moved most of everything external in there. Created adocuments
module that will contain nearly all of our usecases for now.Moved a lot of logic from the Lambda handlers into a new usecase. This is better organization of the code, and will aid in testing.
Created some unit tests for testing the usecases.
Made the unit tests run as part of the CI pipeline.
Towards #110.