When running the "Upload user picture" test, I get two exceptions: 70 passes, 2 fails, 0 exceptions

Image is displayed in user's profile page   Other  drupal_web_test_case.php  284  testWithGDinvalidDimension	
File is located in proper directory         Other  drupal_web_test_case.php  284  testWithGDinvalidDimension

By adding some debug output to the test file, I can see that when Simpletest is uploading the file, the following error message is generated:

The selected file image-1.png could not be uploaded. The file is 66.7 KB exceeding the maximum file size of 66.56 KB.

Could it be that the GD library at my machine is doing worse compression than everybody elses?

The test passes, if I make the following change to testWithGDinvalidDimension():
$test_size = floor(filesize($image->filename) / 1000) + 1;

         // set new variables;
-        $test_size = floor(filesize($image->filename) / 1000) + 1;
+        $test_size = floor(filesize($image->filename) / 1000) + 2;
         $test_dim = ($info['width'] - 10) . 'x' . ($info['height'] - 10);

However, I don't know what is going on with all these magic constants, so I'd rather not just change them.

CommentFileSizeAuthor
#12 user_test_299290.patch7.74 KBdrewish
#10 user_test_299290.patch7.65 KBdrewish
#7 user_test_299290.patch4.31 KBdrewish
#6 user_test_299290.patch5.03 KBdrewish
#2 299290-1.patch1.21 KBc960657

Comments

c960657’s picture

c960657 commented
Component: user.module » tests
c960657’s picture

c960657 commented
Status: Active » Needs review
StatusFileSize
new 299290-1.patch1.21 KB
Anonymous’s picture

Anonymous (not verified) commented
Priority: Normal » Critical
Status: Needs review » Needs work

works for me. i think the intention of the patch is to allow the file size to work and test that GD resizes the image, so i think this is the right way to go.

setting to critical, whichever way we go with this, having tests fail on a fresh HEAD needs fixing.

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs work » Reviewed & tested by the community

woops, wrong status.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Needs work

Let's add a comment above that line please, to explain what it's doing, and why "2" was chosen as the arbitary number.

drewish’s picture

drewish commented
Priority: Critical » Normal
Status: Needs work » Needs review
StatusFileSize
new user_test_299290.patch5.03 KB

I think that we should actually be dividing by 1024 since the user_validate_picture() is using the following validator:

    'file_validate_size' => array(variable_get('user_picture_file_size', '30') * 1024),

I corrected this and fixed the other user picture update tests to use 1024. I also added some comments explaining what the desired goal of the settings was. There were a couple of missing t()'s in there that I added in as well.

drewish’s picture

drewish commented
StatusFileSize
new user_test_299290.patch4.31 KB

whoops, that last patch had an unrelated change to simpletest.install mixed in.

Anonymous’s picture

Anonymous (not verified) commented

nice work drewish. the comments make the intention much clearer.

wont set this to RTBC til i get a chance to run it.

c960657’s picture

c960657 commented
Status: Needs review » Needs work

The updated patch doesn't fix my original problem. By dividing with 1024 rather than 1000, $test_size is decreased rather than increased.

My problem is that the resized image gets a greater filesize than the original picture (original: 64,027 bytes, resized: 66,695 bytes). Apparently GD on my machine does poor compressing. I am running PHP 5.2.0-8+etch11 with GD 2.0.33-5.2etch1 on Debian 4 (etch).

Since the purpose of this specific test is not to check user_picture_file_size or compression ratios of resized files, we may just want to remove the filesize restriction, i.e. let user_picture_file_size be 0.

drewish’s picture

drewish commented
Status: Needs work » Needs review
StatusFileSize
new user_test_299290.patch7.65 KB

c960657, I see your point, that test is incorrect. In the case that the image dimensions pass but the file is larger than the limit the upload should fail.

Closer inspection revealed that the error messages we're looking for don't ever occur in core. I've modified this to check for the actual messages.

I also noticed that there was a pretty nasty error with the testWithoutGDinvalidSize() test. It was trying to upload a file that didn't exist causing curl choke and the tests to hang. To test the without GD tests you'll need to open up image.inc and change the first line of image_get_toolkit() to return FALSE;

So summary of changes:
* Expect a failure if the image size is too large but the dimensions are okay (since GD would never attempt to resize in this case).
* Only sets the filesize limit when that's what we're checking.
* Matches the actual upload failure messages.
* Checks that GD actually resizes the image (before we were just assuming that if it was uploaded it was resized).
* Proper comments.
* t() on assert messages.

c960657’s picture

c960657 commented

Looks good. Now all tests passes :-)

Just a few style notes (mostly a matter of taste):

+        // Set new variables: invalid dimensions, valid filesize (0 = no limit).
         $test_dim = ($info['width'] - 10) . 'x' . ($info['height'] - 10);
+        $test_size = 0;
         variable_set('user_picture_dimensions', $test_dim);
         variable_set('user_picture_file_size', $test_size);

Personally I'd just eliminate $test_size like this:

         variable_set('user_picture_file_size', 0);

Also, writing "Set new variables" above a variable_set() call seems like stating the obvious.

drewish’s picture

drewish commented
StatusFileSize
new user_test_299290.patch7.74 KB

c960657, i agree. removed the size variable where it was not referenced elsewhere.

c960657’s picture

c960657 commented

Looks good to me.

drewish’s picture

drewish commented
Status: Needs review » Reviewed & tested by the community

alright then.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

These tests weren't failing for me before, but they continue to not fail for me after the patch. The code that's here looks much better commented and logical than what was there before, and while there's other things I would clean up here (such as variable names like "$test_dim") they were in the code from before and could be dealt with as a separate "Clean up user.test" issue.

Committed to HEAD. Thanks!

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.