-
Notifications
You must be signed in to change notification settings - Fork 0
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
Admin tests #1775
Admin tests #1775
Conversation
actlikewill
commented
Mar 15, 2024
•
edited
Loading
edited
- this adds tests for the judgment admin
🔍 Existing Issues For ReviewYour pull request is modifying functions with the following pre-existing issues: 📄 File: peachjam/models/core_document_model.py
Did you find this useful? React with a 👍 or 👎 |
@longhotsummer these tests are already revealing some issues. There is some inconsistency with the way we set mimetypes for source documents. When adding new documents, to set the mimetype, we read from file.content_type but when we are modifying, we use the python magic library. These two dont always give the same results. It could also be because of the way Webtest is mocking the files but its making the tests that I'm writing fail. |
This is good to learn. We An option is to add some small PDF and DOCX files as fixture objects and post real files. |
@longhotsummer I've tried this but it doesn't seem to work properly. The webtes.Upload doesn't seem to work with actual files. Still checking though |
peachjam/tests/test_admin.py
Outdated
self.assertRedirects(response2, judgment_list_url) | ||
|
||
judgment.refresh_from_db() | ||
self.assertIn("test case", judgment.content_html) |
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.
@longhotsummer this is the assertion that revealed the issue. I'm checking that the content_html has been changed, but since the mimetype is incorrect, it is not extracting the content. The mimetype is reported as text/plain
@longhotsummer this pr will merge to this one #1771 with the bug fix, the we can merge it to master |