User tests exceptions
c960657 - August 24, 2008 - 10:19
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | tests |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed |
Description
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 testWithGDinvalidDimensionBy 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.
#1
#2
#3
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.
#4
woops, wrong status.
#5
Let's add a comment above that line please, to explain what it's doing, and why "2" was chosen as the arbitary number.
#6
I think that we should actually be dividing by 1024 since the user_validate_picture() is using the following validator:
<?php'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.
#7
whoops, that last patch had an unrelated change to simpletest.install mixed in.
#8
nice work drewish. the comments make the intention much clearer.
wont set this to RTBC til i get a chance to run it.
#9
The updated patch doesn't fix my original problem. By dividing with 1024 rather than 1000,
$test_sizeis 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_sizeor compression ratios of resized files, we may just want to remove the filesize restriction, i.e. letuser_picture_file_sizebe 0.#10
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.
#11
Looks good. Now all tests passes :-)
Just a few style notes (mostly a matter of taste):
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.
#12
c960657, i agree. removed the size variable where it was not referenced elsewhere.
#13
Looks good to me.
#14
alright then.
#15
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!
#16
Automatically closed -- issue fixed for two weeks with no activity.