Fix #11716: Failure in Dynamic Image ServeView #11825
Conversation
Added back a line that was removed in a previous commit,"rendition.file.open("rb")" in serve.py.
Also, created a test in admin/tests/viewsets , test_image_presence.py that confirms that
the issue has been resolved. The test looks for a status error code 500 in the display of an image,
something that happenned when the dynamic serve view failed. If it finds it, the test fails.
The test also passes when we dont't use a dynamic serve view.
Manage this branch in SquashTest this branch here: https://afonsomatos3bugfix11716-dynami-k1g5h.squash.io |
|
|
||
|
|
||
| class TestImagePresence(WagtailTestUtils, TestCase): | ||
| def test_image_presence(self): |
There was a problem hiding this comment.
Issue: This test case feels rather complex considering what it's testing. You should be able to create an image URL for an existing image, and then check the response looks like an image.
There are already tests for the image serve view, so this would make sense there. I'm also not sure what this is testing that the other tests aren't already.
There was a problem hiding this comment.
I added test_image_presence.py because i saw that all of the tests that were in the main code were passing while the bug was still present. Pardon me if i am mistaken but it seems none of them were testing the Dynamic Serve View. To resolve that, I made this test and i made sure that it passed with the fix and it failed without it. All of this code stands for the fact that i had some troubles replicating the bug on a test, hence all of the code. With that being said, you prefer i reduce the complexety of the test? I could try to do that but i think it's presence is necessary to verify the bug fix. I am new to wagtail and open source code in general, so I apologize in advance for my lack of experience, and especially my late reply.
Thank you for your time,
Afonso Matos
There was a problem hiding this comment.
There are definitely tests for the serve view here:
wagtail/wagtail/images/tests/tests.py
Line 336 in 252bae9
You should be able to adapt what you have to work with the setup there, which shows you how to find the serve URL for a given image. Your tests seem to also assume you have a separate Wagtail instance running on port 8000, which is definitely not right - you'll need to be using the test client.
To write the test, I'd recommend something test-driven. Undo the fix in serve.py, and then write a test case which fails, and thus exercises the bug. Fix the bug, and the test case should pass. That is the perfect test 🙂
Following the line of thought of @LANCECORREIA, I added back the line that was removed in a previous commit,"rendition.file.open("rb")" in serve.py. I also, created a test in admin/tests/viewsets , test_image_presence.py that confirms that the issue has been resolved. The test looks for a status error code 500 in the display of an image, something that happenned when the dynamic serve view failed. If it finds it, the test fails. The test also passes when we dont't use a dynamic serve view.
Have in mind that test_image_presence.py in order to run, needs to have already have an image in our home page that we are trying to display.
I am new to wagtail so if something isn't to your liking, please let me know.