Posted by webchick on October 12, 2009 at 4:18am
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | image.module |
| Category: | task |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | Needs tests |
Issue Summary
Assuming #560780: Add Image Field to image.module makes it in before freeze (it's damn close), we need to write tests for it.
Here's a non-exhaustive list of things to check:
- Public/Private/Temporary access, for both the files themselves and derived image styles
- With/without a default image
- With/without title/alt fields
- Max/Min resolution
- Preview image style
- Image as formatter
- Image link to content as formatter
- Image link to file as formatter
- Image style as formatter
Comments
#1
#653074: Review test coverage / outstanding tests before release
#2
Hm. This one I actually would classify as critical, and separate from the other issue. There's a gigantic canyon of difference between a particular branch of code or a regression not having a test and an entire huge swath of functionality missing them. We're probably actively breaking this constantly and just have no idea yet.
#3
Here's a first pass at this.
Things tested.
Displaying of attached images.
- Default formatter
- Image linked to file formatter
- Image linked to content formatter
- Image style 'thumbnail' formatter, ensures image fields work with image styles
Image Field instance settings.
- Set mix/max resolution and validate they appear on edit form
- Set max file size and validate it appears on edit form
- Set allowed file extensions and validate they display on edit form
- Set preview image to display using 'medium' style and verify
- Enable alt/title fields and verify they appear on the edit form
- Create a node with alt/title fields added to an image and verify they are displayed properly
Validation tests.
- Min/Max resolution
- I believe everything else is covered by file.tests since image field just extends the file field widget.
Default Image Tests
- node type has image field and has default image and no image is attached. Ensure default image is displayed.
- node type has image field and has default image, and image is attached. Ensure attached image is displayed an not default.
#4
Heh, I was writing a test for this too. But I am so slow.. Anyway! I took a look at the test and it looks good! I only have a few comments:
* Use drupal_strtolower() instead of strtolower()
* There is a concatenation dot with no space on the right side in the two lines starting with
$this->assertFieldByName($field_name . '[' . LANGUAGE_NONE#5
Attached patch fixes the concatenation issues. I'm pretty sure
strtolower()is okay, at least it is used in the same way in a number of other core tests.Happy to change it though if someone can make a good argument against strtolower() in this use case.
#6
I ran this test and it passed without errors.
#7
You are right about the use of strtolower in other tests, so the test looks good :)
#8
I reviewed this patch. Found a small grammatical error, which is fixed in the attached patch.
The tests seem to make sense to me and cover basic features that I can think of.
Here is a list of some more tests that could be helpful (note: I am not super familar with this new module, so some of these might be part of the main image module and not just for fields...but mentioning them anyways.)
* add a new image style setting and test that the formatter for that style are used on fields
(currently the test tests the formatters that come with the image module)
* delete a setting/style and make sure it is not available
* edit a style setting and make sure the new formatter choice is used
* add a default image after the field has already been created and make sure the new default image is used
* delete default image and make sure the setting for the default image goes away
* if there are some sort of image permissions enabled, make sure that the imagefield will not show
* if image added is the wrong type, or too big, make sure it can't be used (not sure if this is the right place)
* make sure that the image field is not available for editing to users without permissions
#9
I'm okay committing a base set of tests once they're ready, and then expanding further upon them with additional patches. The key thing to remember is that nothing is being tested right now; partial coverage is better than no coverage. :)
#10
resubmitting the same patch. think maybe it is set to 'ignored' because i forgot to add --no-review when i made the diff with git (per eojthebrave's instructions)
#11
#12
The last submitted patch, 601966-10-chachasikes-image_tests.patch, failed testing.
#13
@chachasikes, thanks for the feedback. I think that most of the things you mentioned are either already being tested, or are in my opinion the responsibility of another module. Though feel free to convince me otherwise.
Not sure if this is necessary. We're already testing the 'thumbnail' formatter, which is an image style and while it is one of the defaults is technically no different than a user provided style, and we're already testing the ability to add/edit image styles.
hmm .. pondering this one.
This is already covered. The current set of tests for default images creates a new field with no default image, creates a new node with no image and makes sure there is no image, then adds a default image to the field and makes sure that the already created node now displays the default image.
Added a test to ensure removing a default image from a field instance works.
There are no image.module permissions aside from "Administer image styles" so I'm not sure this is applicable.
There is a test for the image resolution setting, the type and size aspects of the image fields are provided by file.module and are already being tested there. So I think we're okay on this one.
I believe that any field related permissions should be handled by field.module or sim, not the task of a module that implements a field type, and should be tested there as needed.
#14
The last submitted patch, 601966-13-eojthebrave-image_tests.patch, failed testing.
#15
#16
I took a look again and all looks good :)
#17
Great job on this, folks!! There was one weird thing I noticed as I was glancing through:
+++ modules/image/image.test@@ -505,3 +505,280 @@ class ImageAdminStylesUnitTest extends DrupalWebTestCase {
+ $matches = array();
+ preg_match('/node\/([0-9]+)/', $this->getUrl(), $matches);
+ return isset($matches[1]) ? $matches[1] : FALSE;
What is this doing? At the very least it needs a comment, but it looks like it ought to instead be an assertion of some kind?
+++ modules/image/image.test@@ -505,3 +505,280 @@ class ImageAdminStylesUnitTest extends DrupalWebTestCase {
+ function testResolution() {
Let's get a small PHPDoc summary here.
Also, can we please add explicit @todos for tests we know are missing?
Powered by Dreditor.
#18
Added comments as per #17.
Went to write tests for deleting an image style and choosing a replacement and discovered this bug #713872: Image field formatters not updated when image style deleted and replacement format chosen
With the exception of the issue mentioned above I'm out of ideas for things to test. Feel free to chime in if you can come up with anything else and I'll try and add coverage for it as well.
Otherwise, I think this is ready to go.
#19
This is good to go. The new-opened bug can contain tests for what it is fixing I think.
#20
YAY!! :D Thank you SO much, eojthebrave and for naxoc and chachasikes for pushing this through!
Gleefully committed to HEAD. :)
#21
I don't know if this is related, but here is a filefield/imagefield bug: #714350: Undefined index: display_field in file_field_widget_form() in file.field.inc.
#22
Automatically closed -- issue fixed for 2 weeks with no activity.