Tests for image field [#601966]

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

Comment	File	Size	Author
#18	601966-18-eojthebrave-image_tests.patch	12.95 KB	eojthebrave
#18
#15	601966-15-eojthebrave-image_tests.patch	12.45 KB	eojthebrave
#15
#13	601966-13-eojthebrave-image_tests.patch	12.47 KB	eojthebrave
#13
#10	601966-10-chachasikes-image_tests.patch	11.95 KB	chachasikes
#10
#8	601966-7-chachasikes-image_tests.patch	11.96 KB	chachasikes
#8
#5	601966-5-eojthebrave-image_tests.patch	11.95 KB	eojthebrave
#5
#3	601966-3-eojthebrave-image_tests.patch	11.95 KB	eojthebrave
#3

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

catch

he/him

English

CreditAttribution: catch commented 8 December 2009 at 06:15

Priority:

Critical

» Normal

#653074: Review test coverage / outstanding tests before release

Comment #2

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick commented 8 December 2009 at 06:23

Priority:

Normal

» Critical

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.

Comment #3

eojthebrave

he/him

English

Minneapolis, MN

CreditAttribution: eojthebrave commented 22 January 2010 at 04:05

Status:

Active

» Needs review

File	Size
601966-3-eojthebrave-image_tests.patch	11.95 KB

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.

Comment #4

naxoc CreditAttribution: naxoc commented 30 January 2010 at 18:52

Status:

Needs review

» Needs work

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

Comment #5

eojthebrave

he/him

English

Minneapolis, MN

CreditAttribution: eojthebrave commented 31 January 2010 at 20:10

Status:

Needs work

» Needs review

File	Size
601966-5-eojthebrave-image_tests.patch	11.95 KB

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.

Comment #6

chachasikes CreditAttribution: chachasikes commented 31 January 2010 at 20:38

Status:

Needs review

» Needs work

I ran this test and it passed without errors.

Comment #7

naxoc CreditAttribution: naxoc commented 31 January 2010 at 20:45

Status:

Needs work

» Reviewed & tested by the community

You are right about the use of strtolower in other tests, so the test looks good :)

Comment #8

chachasikes CreditAttribution: chachasikes commented 31 January 2010 at 21:23

Status:

Reviewed & tested by the community

» Needs work

File	Size
601966-7-chachasikes-image_tests.patch	11.96 KB

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

Comment #9

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick commented 31 January 2010 at 21:33

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. :)

Comment #10

chachasikes CreditAttribution: chachasikes commented 1 February 2010 at 00:06

File	Size
601966-10-chachasikes-image_tests.patch	11.95 KB

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)

Comment #11

catch

he/him

English

CreditAttribution: catch commented 1 February 2010 at 02:26

Status:

Needs work

» Needs review

Comment #12

9 February 2010 at 21:30

Status:

Needs review

» Needs work

The last submitted patch, 601966-10-chachasikes-image_tests.patch, failed testing.

Comment #13

eojthebrave

he/him

English

Minneapolis, MN

CreditAttribution: eojthebrave commented 11 February 2010 at 05:24

Status:

Needs work

» Needs review

File	Size
601966-13-eojthebrave-image_tests.patch	12.47 KB

@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.

* 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)

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.

* delete a setting/style and make sure it is not available
* edit a style setting and make sure the new formatter choice is used

hmm .. pondering this one.

* add a default image after the field has already been created and make sure the new default image is used

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.

* delete default image and make sure the setting for the default image goes away

Added a test to ensure removing a default image from a field instance works.

* if there are some sort of image permissions enabled, make sure that the imagefield will not show

There are no image.module permissions aside from "Administer image styles" so I'm not sure this is applicable.

* 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)

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.

* make sure that the image field is not available for editing to users without permissions

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.

Comment #14

11 February 2010 at 06:10

Status:

Needs review

» Needs work

The last submitted patch, 601966-13-eojthebrave-image_tests.patch, failed testing.

Comment #15

eojthebrave

he/him

English

Minneapolis, MN

CreditAttribution: eojthebrave commented 11 February 2010 at 17:32

Status:

Needs work

» Needs review

File	Size
601966-15-eojthebrave-image_tests.patch	12.45 KB

Comment #16

naxoc CreditAttribution: naxoc commented 11 February 2010 at 18:22

Status:

Needs review

» Reviewed & tested by the community

I took a look again and all looks good :)

Comment #17

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick commented 11 February 2010 at 18:32

Status:

Reviewed & tested by the community

» Needs work

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.

Comment #18

eojthebrave

he/him

English

Minneapolis, MN

CreditAttribution: eojthebrave commented 14 February 2010 at 04:49

Status:

Needs work

» Needs review

File	Size
601966-18-eojthebrave-image_tests.patch	12.95 KB

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.