Wow that was a fast turnaround! I thought I had at least a day to rest in between
You won't have anything today so take your time!
bharat wrote:
a better approach is to create a mock GalleryPlatform instance and have it respond correctly ... the imagemagick module uses these mock platforms to simulate testing on 6+ different ImageMagick environments, even though none of them may actually be installed
I didn't quite understand how to do that, but I am looking at ImageMagick testcases, and I think that I can figure it out now. It also answers my other question: How can I run my testcase with specific toolkit (eg, if I don't have IM, and try to upload PNG - it's a special case, since no other toolkit supports 'compress' operation on PNG). And I wasn't even thinking to ask the question about different versions of IM... Would it be possible to have IM testbed available to other modules (not that I can't cut'n'paste :lol: )
bharat wrote:
The easiest way to test [GalleryEntity:save event] is to hand craft an event and call SizeLimitModule::handleEvent() by hand with it.
Eeehhh - that went over my head :oops: Is there some place where it is done?
mindless
Joined: 2004-01-04
Posts: 8601
Posted: Wed, 2005-01-05 20:39
ImageMagick is the uber case.. testing many different environments at a time.. you might look at other tests with a TestPlatform at the bottom for a simpler case.
Just create a GalleryEvent object, set the entity and/or data for your test and call your handleEvent directly.. ie, instead of performing an action that would normally generate an event, just create the event object that you'd expect if such an event did occur.
virshu
Joined: 2003-09-13
Posts: 314
Posted: Thu, 2005-01-06 00:09
I've been looking at several TestPlatform implementations, and I don't think I need it... It seems that the main purpose is to simulate OS commands. My module doesn't do any OS commands; what I think I need to have is a test toolkit; where I can simulate the results of the toolkit. I see in ZipCart
class ZipCartToolkit extends GalleryToolkit {
function performOperation($mimeType, $operationName, $sourceFilename,
$destFilename, $parameters, $context=array()) {
/* Just copy file and return same mime type */
...
}
}
I think that's what I need
Well, on the second thought, if I am simulating the toolkit, I can't assert filesize and getimagesize without simulating the platform as well. But (on the second - A thought) by asserting the filesize, do I test my code or the toolkit's. For example, I found that the file size of the image after resize depends on the toolit (duh!) - so what's the point for me to assert the filesize. Moreover, assuming that there are testcases for the new operation ('compress') - all I need to do is test that I am passing the right parameters to the "performOperation"...
On the final thought, I am confused (as if it isn't obvious from the wrambling post)...
bharat
Joined: 2002-05-21
Posts: 7994
Posted: Thu, 2005-01-06 04:54
virshu wrote:
...assuming that there are testcases for the new operation ('compress') - all I need to do is test that I am passing the right parameters to the "performOperation"...
Right! The goal here is to test *only* your code, as best as you can. In some cases, your code can be so interwoven into the rest of the framework that it's difficult, but in this case it's probably not so bad.
So if you want to test SetSizeOption::handleRequestAfterAdd() you can do the following:
Create your own $form and $item
Call SetSizeOption::handleRequestAfterAdd() with your $form, $item
Assert that the $ret, $errors and $warnings are as expected
Verify that you performed the right toolkit operation
Most of this is easy. The tricky thing is verifying that you performed the right toolkit operation. You don't want to get involved with ImageMagick or any of the other toolkits for this because they'll just muddy the issue. So the easy way to do it is to make it so that the new item you create is of a mime type that none of the other toolkits care about (let's say, "test/file"). Then create a dummy toolkit that supports the "compress" operation on "test/file". Then implement your dummy toolkit's performOperation call so that it expects *exactly* the right values that your code should generate for the compress operation, when your code operates properly.
modules/core/test/phpunit/ItemEditAlbumPluginTest.class is a good example of this.
Let's say that we're writing the appropriate test for SizeLimitHelper::applyLimits(). In setUp() you'd say:
/* Register a dummy toolkit */
GalleryCoreApi::registerFactoryImplementation('GalleryToolkit',
'SizeLimitHelperTestToolkit',
'SizeLimitHelperTestToolkit',
__FILE__);
/* Register an operation that we can perform on our mock data items. Note that the parameters must be the same as all the other toolkit's "compress" operations, but we don't care about the description, etc. */
$ret = GalleryCoreApi::registerToolkitOperation('SizeLimitHelperTestToolkit',
array('test/file'),
'compress',
array(array('type' =>'int')),
'test-description',
'test/file');
/* Switch to using a mock platform -- we'll define this later on in this post */
$gallery->setPlatform(new SizeLimitHelperTestMockPlatform());
Then you can create your item with the mime type of test/file:
Note that we use __FILE__ as our data source. We don't actually care what the data is since we know our dummy toolkit isn't even going to look at the file. We just need to give it a file that is known to be good (and what's easier than __FILE__ for that).
You'll need to create your own SizeLimitHelperTestDataItem (it can just be an empty class that extends GalleryDataItem -- look at ItemEditAlbumPluginTest for an example of this). You'll also need to create your own SizeLimitHelperTestToolkit the same way. But in this toolkit, you overload performOperation like this:
function testApplyLimits() {
list ($ret, $warning, $error) = SizeLimitHelper::applyLimits($item, 'compress', array('test-argument'));
if ($ret->isError()) {
return $this->failWithStatus($ret->wrap(__FILE__, __LINE__));
}
// Assert that the warning and error are proper.
}
Your test code is going to trigger a few platform calls, so we mock them up here:
class SizeLimitHelperTestMockPlatform {
function fopen($filename, $modes) {
// Pass lock requests
if (preg_match("|\blocks\b|", $filename)) {
return fopen($filename, $modes);
}
print "Unexpected fopen($filename, $modes)";
return null;
}
function fclose($handle) {
return fclose($handle);
}
function flock($handle, $operation, &$wouldblock) {
// Pass lock requests
return flock($handle, $operation, $wouldblock);
}
function filesize($path) {
if ($path == __FILE__) {
return 12345; // some expected value
} else {
return 0; // unexpected value!
}
}
}
Then in your tearDown() make sure you unregister the test toolkit.
So let's trace through what's going on here. You create a new item of type "test/file". You then call SizeLimitHelper::applyLimits() on it to compress the item. applyLimits() looks for a toolkit that supports "compress" on the "test/file" mimetype and there is only one -- your test toolkit. It then calls performOperation with your toolkit, which is designed to *only* accept the right performOperation call. If you get through this then you know that your code is functioning properly up to this point. The next thing your code is going to do is to have the GalleryDataItem rescan its input file (since we expect the file size to have changed). Your mock platform will return a known size for __FILE__. Finally, your code is going to acquire and release locks during its process of adjusting derivatives. Your mock platform is set up to pass through fopen and flock calls on lock files so that code should all function. (If I missed something in the platform, it'll give you an error and you can figure out what's missing and then look at ItemEditMoviePluginTest to see what else you can add).
Now we've completely simulated all of the logic for SizeLimitHelper::applyLimits() without relying on the existence of any other toolkits. We've fully exercised the different aspects of your logic. We can try extending this by adding tests for edge cases, for example we can have a second test that unregisters the toolkit before calling applyLimits() then we can verify that we get the appropriate "Cannot compress PNG" warning back.
Then at some later date, when and if we decide to refactor your code into the core, we can be sure that we didn't break your functionality because we've got a unit test for the proper behaviour for your code.
Does that all make sense?[/]
virshu
Joined: 2003-09-13
Posts: 314
Posted: Thu, 2005-01-06 05:15
actually, I am pretty much done with this test case; I just promised not to send you any more tonight - remember I registered my toolkit for 'image/jpeg', so that I catch a jpeg file (which is indeed __FILE__), and for 'file/unknown' to catch "unprocessable" files. I will probably change jpeg to file/known as you suggest.
When I was writing my perplexed message - I just stumbled onto a Toolkit subclass and was wondering loud if I should be using it... But now the battery is pretty much working properly!
Anyway, I ran into a problem with the code (thanks to unit tests) that I am troubleshooting now; I will also write the other tests (for save event) - so I promise not to send anything until tomorrow
virshu
Joined: 2003-09-13
Posts: 314
Posted: Thu, 2005-01-06 17:47
here is the latest and greatest... The code has some bug fixes (most notable, if you specify both dimension and size limits, and want to keep original - it is now building derivatives cleanly).
The test cases cover three areas - setting limits (SizeLimitOptionTest), copying the settings when new album is created (CopySettingsTest), and applying the settings in variety of permutations (AddPhotoOptionTest). The last suite includes custom Toolkit, custom platform and custome mime type (I used image/known, rather than file/know to leverage some GalleryToolkit functionality.
In CopySettingsTest I was able to register for the event, and run album->save() without having to call my handleEvent directly. It feels cleaner that way.
I noticed one thing in the process. Bharat, I believe you added $warnings in ItemAdd* classes, however, it would be great if you could do the same in ItemAddOptionTestCase. Currently, it swallows the warnings from handleRequestAfterAdd.
bharat
Joined: 2002-05-21
Posts: 7994
Posted: Sat, 2005-01-08 02:16
It's looking really good! You've cleaned it up considerably. I tried it out and it seems to be working fine. There's only one issue I noted which is that it seems to create distorted images. I set my restriction to 100x100 and uploaded a file that's 750x500 and wound up with an image that's actually exactly 100x100! I think that you want to use the scale operation and not the resize since scale maintains aspect ratio and resize does not.
I'll start working on adding warnings to ItemAdd tonight. More comments below.
General
11 occurrences of // comment style (according to: grep \/\/ `find . -type f`)
17 occurrences where you're using double quotes instead of single quotes (according to: grep \" `find . -name '*.inc' -o -name '*.class'`)
Still missing tests for the SizeLimitHelper methods (but you knew that)
SizeLimitOptionTest.class
when using assertEquals(), the expected value should be first
and the actual value second (first seen line 65). PHPUnit will
color code them in its output.
CopySettingsTest.class
It's actually better to call your handleEvent directly instead of going through the event handling system. It'll work either way for now, but it makes your test more brittle and if we start refactoring the test framework we may have to come back and fix your test. Calling your method directly guarantees that you only test the code that you wrote.
You might rename this to EventHandlerTest so that you don't have to rename it again if you wind up adding more things to your event handler in addition to copying settings.
You could restructure the data setting part of your code like this for brevity:
You don't need to use "print $ret->getAsHtml()" on failures inside tests, it's only useful in setUp (where if something goes wrong you might wind up seeing the wrong error message).
Your assertEquals all have "width" as their description (oops) and should use single quotes. It's also acceptable to use the array structure I suggested above for your asserts (in fact, you can just have one array that you use to set the data and verify it at the end). As before, expected values should be first, actual second.
AddPhotoOptionTest.class
in setUp() use $gallery->setPlatform(), don't mess with its private variables directly
line 75 should be indented to 8 columns
you don't need to restore the platform in tearDown() (the phpunit framework will take care of that for you).
You can use the array technique I outlined above to initialize your plugin params for your test methods, or create a helper method in the class that you can call, eg:
I don't understand why testChangeDimNoOriginal (and others) is getting and asserting on the 'operation' request variable. Where does that come from and why do we care?
should testChangeDimNoOriginal verify that there is no preferred?
276: GalleryStatus:error() has a lowercase 'e'
SetSizeLimitOption.tpl
The javascript near the top still refers to "g2_dimensions[width]" which should probably be {g->formVar var="dimensions[width]"} (6 different similar occurrences)
In general (and this isn't documented anywhere) we don't indent inside {if}{/if} blocks. We try to let the indentation follow the HTML, so it should be:
<div>
{if ...}
text
text
{/if}
</div>
It looks marginally better when you view the HTML this way.
"No Limits" should be in a {g->text} block
module.inc:
Looks great!
SizelimitHelper.class
Should be named SizeLimitHelper.class (note capital L)
Strings to be localized must be constant (otherwise localizers can't do their jobs). Line 69 should be:
$warning = array($module->translate('WARNING: Can't perform operation %s on mime type %s', $operation, $item->getMimeType()));
The localizer will get the string with %s in it and will do the right thing.
77-80: should be indented under the left paren in performOperation
When you're doing error handling, return null instead of an empty array. The error handler does not expect to get back an answer, anyway. So it should be:
if ($ret->isError()) {
return array($ret->wrap(__FILE__, __LINE__), null, null);
}
67, 141: wrong number of arguments in return
148: check ids instead of using == on objects (more resistant to bugs in different PHP versions)
SizeLimitOption.inc
51: extra empty comment line
in loadTemplate, your variable should be $SizeLimitOption (note capital L and lack of pluralization). Ie, it should exactly match your view name. Use the same exact name when you store it in the template, too.
SetSizeOption.inc
Looks great!
[/]
[/]
[/]
[/]
[/]
[/]
[/]
[/]
[/]
bharat
Joined: 2002-05-21
Posts: 7994
Posted: Sun, 2005-01-09 08:25
I just committed code to fix the problems with ItemAddOptionTestCase swallowing the warning, and I added warnings to ItemEditOptions (and also changed around the way that they're registered).
virshu
Joined: 2003-09-13
Posts: 314
Posted: Mon, 2005-01-10 04:58
I almost uploaded the code, and then I got CVS and realized that the warnng handling changes caused some some changes in the calling prgrams as well... Hence the delay.
bharat wrote:
I set my restriction to 100x100 and uploaded a file that's 750x500 and wound up with an image that's actually exactly 100x100! I think that you want to use the scale operation and not the resize since scale maintains aspect ratio and resize does not.
mindless wrote:
ack! imagemagick is the only toolkit that maintains aspect ratio for resize operation.. grumble, grumble... erg. i think i'll have scale take an optional 2nd parameter instead.
Granted, it was before the baby... (Dec 30, to be exact)
bharat wrote:
Still missing tests for the SizeLimitHelper methods
But SizeLimitHelper methods are not called from UI but from main methods; so both methods are covered in the tests in different permutations
bharat wrote:
when using assertEquals(), the expected value should be first and the actual value second (first seen line 65). PHPUnit will color code them in its output.
That goes so much against my principles, beliefs, and everything that I learned in life Since I was in grad school I was ridiculing students who wrote if 5 == x. But I can't argue with coloring, either. In short, my heart was bleeding when I was doing those changes!
bharat wrote:
I don't understand why testChangeDimNoOriginal (and others) is getting and asserting on the 'operation' request variable. Where does that come from and why do we care?
I set the operation when I call performOperation in the toolkit. ('resize' for dimensions, 'compress' for filesize). So, functionally, I am asserting that I am passing the right request to the toolkit.
Your other problems that you noticed (that I can't find a reason to push back) - were quietly fixed...
virshu
Joined: 2003-09-13
Posts: 314
Posted: Wed, 2005-01-12 17:55
here is the new version that takes into consideration the changes in the core... I also looked at the SizeLimitHelper functionality from the perspective of acid test that you described. There are three places in SizeLimitHelper that, if removed would not break any test. I am not sure, if it's "dead code" though, but I am not sure how to simulate the situation.
applyLimits and buildDerivativeWithLimits calling GalleryCoreApi::adjustDependentDerivatives. When I create a new photo - there are no derivatives, that I don't know about. However, there might be (your example of a watermark comes to mind). And this other derivative could have been created before my code. So, the call seems needed.
applyLimits calling $item->isModified(). In my logic, the item will always get modified. (if it doesn't need modifications, that branch won't get called). However, I can imagine, that the toolkit won't be able (for example) to compress the picture. So, it doesn't hurt to have such check
buildDerivativeWithLimits calling if ($preferred->hasNoOperations()). Same as previous comment. In my logic I can't imagine that there will be no operations, but if some other module created an operation that is similar (or broader) than mine - it 'feels' like a helpful check.
[/]
bharat
Joined: 2002-05-21
Posts: 7994
Posted: Thu, 2005-01-13 06:12
We're getting really close. Almost all of the stuff I'm suggesting now is cosmetic tweaking, polishing off the details having to do with naming, and style. The only big one left is the test issue. There are a couple of things that are still lingering from prior reviews, so I think that probably one more review (which will hopefully have few comments from me) will do it, then you can commit!
General
There are still some inconsistent names, and since PHP is a loosely typed language, it's very important that we have strong guidelines about what you name classes, files, templates and variables so that when we have many modules working together they don't step on each other:
SizeLimitOption lives in SizeLimitOption.inc (good) but it includes SetSizeLimitOption.tpl (bad). That should be renamed to SizeLimitOption.tpl
Inside SizeLimitOption.tpl (I'm assuming that we rename it) the form variables should all be form[SizeLimitOption][...], and the non form variables should be prefixed with SizeLimitOption_ so eg: {g->formVar var="dimensions[width]"} should be {g->formVar var="SizeLimitOption_dimensions[width]"}. It looks like you're using {$SizeLimitOption} for all the smarty variables so that looks correct to me.
These naming changes will have to be propagated all through your view, controller, template and tests.
Empty.tpl is still in the tarball -- that can be deleted.
SizeLimitHelper tests. I can understand your reservations about writing tests for this class, as the core behaviour is already tested by the controller. I'm pretty firm about this though; it's going to have to have a test before it can go into the codebase. One of the main reasons (and there are several reasons, but this should be enough for now) for this is that sooner or later we're going to want to refactor this code and at that point if we try to refactor it without unit tests we're going to have a really hard time figuring out whether or not we are breaking something. We'll only know that the stuff covered by the controller test still works. My mantra is "if it's not tested, it's probably broken and we haven't noticed it". When you do test driven design, you write the test first and then you write code to satisfy the test. So you don't wind up in the situation where you have code that you don't understand and that doesn't have tests. That's a lot easier. In your situation, you've copied some fairly hairy code that doesn't have obvious implications and it's unclear if it's getting tested or not.
So my suggestion to you is that you go through the two helper methods and remove anything that you don't understand. This should simplify the code. Then you should be able to write fairly simple tests for everything else because it's all code that you understand. If, later on, we find out that some of that code was necessary because of a bug, we can always pull it back in again (we know where it came from originally anyway) and at that time we can follow the standard XP methodology which is to write a new test to expose the bug and then change the code so that the test passes (and then the bug is fixed).
SizeLimitOptionTest.class
You don't assert the state of the $warning returned from _handleRequestAfterEdit in any of the tests
EventHandlerTest.class
53,72: height is misspelled. This brings up an interesting point. If it's misspelled here, why doesn't the test fail? It turns out that your event handler in module.inc copies all the parameters from the parent to the child without examining them. I guess that's not a bug, I can't see why it's bad in particular. It does mean that your test doesn't actually even need to use real values; you can use bogus values and it'll be just fine so long as it copies all of them.
66: you're not checking the return status from the handleEvent() call.
AddPhotosOptionTest.class
67: should be indented to 18 columns
130: should be indented to 17 columns
testChangeDimNoOriginal() (and most of your other tests) don't assert the value of $warning
You explained that your dummy toolkit puts the operation into the request. That's clever; I was doing it a different way (basically, I was taking an approach where we'd have the toolkit store the value and then ask it later for the most recent operation). Your approach is simpler, though and maybe we should mimic that elsewhere. I'll think on that.
293: should be indented to 8 columns. It'd also be nice to assert what the warnings actually are, instead of just counting them. You can always get the size limit module and translate the string for yourself inside the test and then compare the two strings.
Still have 4 occurrences of GalleryStatus::Error (the 'e' should be lowercase)
SetSizeLimitOption.tpl
Should be renamed to SizeLimitOption.tpl and internal form variables should be renamed as per my general comment.
There's some bad XHTML in there. I noticed <input ...> (should be <input .../>) but there may be other stuff. Click the "XHTML" button at the bottom of the page and read the report that you get to make sure that your XHTML bugs are resolved.
module.inc
38, 39: the comments have funny indentation and the comment on line 39 has a lot of extra whitespace in it
45: multiple -> Multiple
102: should be indented to 20 columns to avoid confusion with line 21 which is properly indented at 16 columns
SizeLimitHelper.class:
29: the phpdoc block should also have:
* @subpackage Helpers
* @static
34: "reduce" => "Reduce"
39: should be "@param args operation arguments to pass to the toolkit"
91: should be indented to 12 columns
You're still returning array() instead of null when passing an error back up (see my prior review) in 19 places (according to egrep 'return.*wrap.*array\(\)' classes/SizeLimitHelper.class | wc -l)
SizeLimitOption.inc:
42: extra blank line
$form['sizelimit'] should be $form['SizeLimitOption'] as I mentioned in the general comment
SetSizeOption.inc:
After the next CVS up, you can delete loadTemplate() since I just changed it so that the default ItemAddOption::loadTemplate() return success and does nothing.
82, 85: you're merging the returned erors/warnings into your errors and warnings array that you defined as empty just above. Since this doesn't happen in a loop, there'll never be anything on the left hand side of the merge, so why not just do an assignment? And if you're going to do that, then you can just do (for example): list($ret, $errors, $warnings) = SizeLimitHelper::applyLimits($item, 'resize', $args);
and skip the intermediate variables altogether. Am I missing something?
[/]
[/]
[/]
[/]
[/]
[/]
[/]
[/]
[/]
[/]
virshu
Joined: 2003-09-13
Posts: 314
Posted: Thu, 2005-01-13 19:47
I realize that I had a philosophical problem with unit testing the helper class. I was trying to build the test cases to have the broadest possible coverage. As such I felt that all my controller cases covered the helper class (and even satisfied the acidity criteria) and specific helper test cases were redundant. Well, unit tests should be as narrow as practical (your example of refactoring was almost eye-opening); so from that perspective, I even pulled one of the test cases (unknown format) from controller to helper, since it is the helper that recognizes the mime format as unknown.
I also corrected all the other observations. I think I caught all XHTML violations (all of them were in input); since I am still on localhost - I can't get the validator to confirm that.
And I missed some of your comments last time (eg., returning array() vs. null) - so I probably missed something here, as well. But I think I combed through all of them...
bharat
Joined: 2002-05-21
Posts: 7994
Posted: Fri, 2005-01-14 07:19
Woo! I think we're there! I have just a few small comments, and one bigger one. Since you've put so much effort into this I really want you to get it in and stop having to chase after all of the changes we make. So, I'll give you the following choice. You can either fix all the little things and commit it, then come back and refactor it to fix the one bigger thing, or you can fix the bigger thing before you commit.
Little things
General
I'm getting 12 unit test failures. See the attached HTML file. I didn't dig into them too far.
EventHandlerTest.class
testCopy returns an array (4 places) when it should return a single GalleryStatus
module.inc
39, 40: the comments have funny indentation and the comment on line 40 has a lot of extra whitespace in it
49: this comment is misleading and should probably be deleted
SetSizeOption.inc:
in handleRequestAfterAdd() all error returns should have 3 parameters
Big thing
When I look at SizeLimitHelperTest::testUnknownFormat() it becomes glaringly obvious to me that you've got a helper that's returning localized content. We typically try to avoid that because we like to make our helper methods be utilitarian and have little to do with the front end. We try to do all content generation in the front end. This gives us the tremendous advantage of being able to create lots of little light-weight views that depend on the helper methods for the heavy lifting, but can present their own view on what happened.
The easy solution for this is to change applyLimits to return an extra success/fail boolean. If it returns a failure, then you put up the warning. If it returns success, you don't. But the warning is generated in SetSizeOption which is in the UI code.
Here's how I'd do it. Put testUnknownFormat() back into the SizeLimitOptionTest, that way you guarantee that unknown objects return the warning. Now you have it in both places and both work. Then what you can do is update the applyLimit tests in SizeLimitHelperTest so that it expects the appropriate true/false instead of the warning. Now you'll have to fix the code in SizeLimitHelper to make those tests pass. But in the process you'll still have to maintain the invariant that you created in SizeLimitOptionTest. Once both sets of unit tests pass, you're done. You don't even need to verify it via the UI -- once the tests pass you KNOW that you've got it right.[/]
[/]
[/]
[/]
cajun_junky
Joined: 2005-01-21
Posts: 12
Posted: Fri, 2005-01-21 17:10
I really appreciate everyones effort to complete this size limiter. It will save alot of time! I have been watching the progress and was wondering how it is coming and if it is usable/stable now.
mindless
Joined: 2004-01-04
Posts: 8601
Posted: Fri, 2005-01-21 18:44
oops, no one made an announcement here... the module is now in cvs!
Posts: 314
You won't have anything today
so take your time!
I didn't quite understand how to do that, but I am looking at ImageMagick testcases, and I think that I can figure it out now. It also answers my other question: How can I run my testcase with specific toolkit (eg, if I don't have IM, and try to upload PNG - it's a special case, since no other toolkit supports 'compress' operation on PNG). And I wasn't even thinking to ask the question about different versions of IM... Would it be possible to have IM testbed available to other modules (not that I can't cut'n'paste :lol: )
Eeehhh - that went over my head :oops: Is there some place where it is done?
Posts: 8601
ImageMagick is the uber case.. testing many different environments at a time.. you might look at other tests with a TestPlatform at the bottom for a simpler case.
Just create a GalleryEvent object, set the entity and/or data for your test and call your handleEvent directly.. ie, instead of performing an action that would normally generate an event, just create the event object that you'd expect if such an event did occur.
Posts: 314
I've been looking at several TestPlatform implementations, and I don't think I need it... It seems that the main purpose is to simulate OS commands. My module doesn't do any OS commands; what I think I need to have is a test toolkit; where I can simulate the results of the toolkit. I see in ZipCart
class ZipCartToolkit extends GalleryToolkit { function performOperation($mimeType, $operationName, $sourceFilename, $destFilename, $parameters, $context=array()) { /* Just copy file and return same mime type */ ... } }I think that's what I need
Well, on the second thought, if I am simulating the toolkit, I can't assert filesize and getimagesize without simulating the platform as well. But (on the second - A thought) by asserting the filesize, do I test my code or the toolkit's. For example, I found that the file size of the image after resize depends on the toolit (duh!) - so what's the point for me to assert the filesize. Moreover, assuming that there are testcases for the new operation ('compress') - all I need to do is test that I am passing the right parameters to the "performOperation"...
On the final thought, I am confused (as if it isn't obvious from the wrambling post)...
Posts: 7994
Right! The goal here is to test *only* your code, as best as you can. In some cases, your code can be so interwoven into the rest of the framework that it's difficult, but in this case it's probably not so bad.
So if you want to test SetSizeOption::handleRequestAfterAdd() you can do the following:
Most of this is easy. The tricky thing is verifying that you performed the right toolkit operation. You don't want to get involved with ImageMagick or any of the other toolkits for this because they'll just muddy the issue. So the easy way to do it is to make it so that the new item you create is of a mime type that none of the other toolkits care about (let's say, "test/file"). Then create a dummy toolkit that supports the "compress" operation on "test/file". Then implement your dummy toolkit's performOperation call so that it expects *exactly* the right values that your code should generate for the compress operation, when your code operates properly.
modules/core/test/phpunit/ItemEditAlbumPluginTest.class is a good example of this.
Let's say that we're writing the appropriate test for SizeLimitHelper::applyLimits(). In setUp() you'd say:
/* Register a dummy toolkit */ GalleryCoreApi::registerFactoryImplementation('GalleryToolkit', 'SizeLimitHelperTestToolkit', 'SizeLimitHelperTestToolkit', __FILE__); /* Register an operation that we can perform on our mock data items. Note that the parameters must be the same as all the other toolkit's "compress" operations, but we don't care about the description, etc. */ $ret = GalleryCoreApi::registerToolkitOperation('SizeLimitHelperTestToolkit', array('test/file'), 'compress', array(array('type' =>'int')), 'test-description', 'test/file'); /* Switch to using a mock platform -- we'll define this later on in this post */ $gallery->setPlatform(new SizeLimitHelperTestMockPlatform());Then you can create your item with the mime type of test/file:
Note that we use __FILE__ as our data source. We don't actually care what the data is since we know our dummy toolkit isn't even going to look at the file. We just need to give it a file that is known to be good (and what's easier than __FILE__ for that).
You'll need to create your own SizeLimitHelperTestDataItem (it can just be an empty class that extends GalleryDataItem -- look at ItemEditAlbumPluginTest for an example of this). You'll also need to create your own SizeLimitHelperTestToolkit the same way. But in this toolkit, you overload performOperation like this:
function performOperation($mimeType, $operationName, $sourceFilename, $destFilename, $parameters, $context=array()) { if ($mimeType != 'test/file' || $operationName != 'compress' || $sourceFilename != __FILE__ || $destFilename != __FILE__ || $parameters != array('test-argument')) { return GalleryStatus::error(ERROR_BAD_PARAMETERS, __FILE__, __LINE__); } return GalleryStatus::success(); }Then in your test, you can do this:
function testApplyLimits() { list ($ret, $warning, $error) = SizeLimitHelper::applyLimits($item, 'compress', array('test-argument')); if ($ret->isError()) { return $this->failWithStatus($ret->wrap(__FILE__, __LINE__)); } // Assert that the warning and error are proper. }Your test code is going to trigger a few platform calls, so we mock them up here:
class SizeLimitHelperTestMockPlatform { function fopen($filename, $modes) { // Pass lock requests if (preg_match("|\blocks\b|", $filename)) { return fopen($filename, $modes); } print "Unexpected fopen($filename, $modes)"; return null; } function fclose($handle) { return fclose($handle); } function flock($handle, $operation, &$wouldblock) { // Pass lock requests return flock($handle, $operation, $wouldblock); } function filesize($path) { if ($path == __FILE__) { return 12345; // some expected value } else { return 0; // unexpected value! } } }Then in your tearDown() make sure you unregister the test toolkit.
So let's trace through what's going on here. You create a new item of type "test/file". You then call SizeLimitHelper::applyLimits() on it to compress the item. applyLimits() looks for a toolkit that supports "compress" on the "test/file" mimetype and there is only one -- your test toolkit. It then calls performOperation with your toolkit, which is designed to *only* accept the right performOperation call. If you get through this then you know that your code is functioning properly up to this point. The next thing your code is going to do is to have the GalleryDataItem rescan its input file (since we expect the file size to have changed). Your mock platform will return a known size for __FILE__. Finally, your code is going to acquire and release locks during its process of adjusting derivatives. Your mock platform is set up to pass through fopen and flock calls on lock files so that code should all function. (If I missed something in the platform, it'll give you an error and you can figure out what's missing and then look at ItemEditMoviePluginTest to see what else you can add).
Now we've completely simulated all of the logic for SizeLimitHelper::applyLimits() without relying on the existence of any other toolkits. We've fully exercised the different aspects of your logic. We can try extending this by adding tests for edge cases, for example we can have a second test that unregisters the toolkit before calling applyLimits() then we can verify that we get the appropriate "Cannot compress PNG" warning back.
Then at some later date, when and if we decide to refactor your code into the core, we can be sure that we didn't break your functionality because we've got a unit test for the proper behaviour for your code.
Does that all make sense?[/]
Posts: 314
actually, I am pretty much done with this test case; I just promised not to send you any more tonight - remember
I registered my toolkit for 'image/jpeg', so that I catch a jpeg file (which is indeed __FILE__), and for 'file/unknown' to catch "unprocessable" files. I will probably change jpeg to file/known as you suggest.
When I was writing my perplexed message - I just stumbled onto a Toolkit subclass and was wondering loud if I should be using it... But now the battery is pretty much working properly!
Anyway, I ran into a problem with the code (thanks to unit tests) that I am troubleshooting now; I will also write the other tests (for save event) - so I promise not to send anything until tomorrow
Posts: 314
here is the latest and greatest... The code has some bug fixes (most notable, if you specify both dimension and size limits, and want to keep original - it is now building derivatives cleanly).
The test cases cover three areas - setting limits (SizeLimitOptionTest), copying the settings when new album is created (CopySettingsTest), and applying the settings in variety of permutations (AddPhotoOptionTest). The last suite includes custom Toolkit, custom platform and custome mime type (I used image/known, rather than file/know to leverage some GalleryToolkit functionality.
In CopySettingsTest I was able to register for the event, and run album->save() without having to call my handleEvent directly. It feels cleaner that way.
I noticed one thing in the process. Bharat, I believe you added $warnings in ItemAdd* classes, however, it would be great if you could do the same in ItemAddOptionTestCase. Currently, it swallows the warnings from handleRequestAfterAdd.
Posts: 7994
It's looking really good! You've cleaned it up considerably. I tried it out and it seems to be working fine. There's only one issue I noted which is that it seems to create distorted images. I set my restriction to 100x100 and uploaded a file that's 750x500 and wound up with an image that's actually exactly 100x100! I think that you want to use the scale operation and not the resize since scale maintains aspect ratio and resize does not.
I'll start working on adding warnings to ItemAdd tonight. More comments below.
General
SizeLimitOptionTest.class
and the actual value second (first seen line 65). PHPUnit will
color code them in its output.
CopySettingsTest.class
foreach (array('width' => 768, 'height' => 1024', ...) as $key => $value) { $ret = GalleryCoreApi::setPluginParameter('module', 'sizelimit', $key, $value, $this->_album->getId()); if ($ret->isError()) { return $this->failWithStatus($ret->wrap(__FILE__, __LINE__)); } }AddPhotoOptionTest.class
$ret = $this->_initParameters(array('width' => 768, ...));to make your tests smaller.
SetSizeLimitOption.tpl
<div> {if ...} text text {/if} </div>It looks marginally better when you view the HTML this way.
module.inc:
SizelimitHelper.class
$warning = array($module->translate('WARNING: Can't perform operation %s on mime type %s', $operation, $item->getMimeType()));The localizer will get the string with %s in it and will do the right thing.
if ($ret->isError()) { return array($ret->wrap(__FILE__, __LINE__), null, null); }SizeLimitOption.inc
SetSizeOption.inc
[/]
[/]
[/]
[/]
[/]
[/]
[/]
[/]
[/]
Posts: 7994
I just committed code to fix the problems with ItemAddOptionTestCase swallowing the warning, and I added warnings to ItemEditOptions (and also changed around the way that they're registered).
Posts: 314
I almost uploaded the code, and then I got CVS and realized that the warnng handling changes caused some some changes in the calling prgrams as well... Hence the delay.
Granted, it was before the baby...
(Dec 30, to be exact)
But SizeLimitHelper methods are not called from UI but from main methods; so both methods are covered in the tests in different permutations
That goes so much against my principles, beliefs, and everything that I learned in life
Since I was in grad school I was ridiculing students who wrote if 5 == x. But I can't argue with coloring, either. In short, my heart was bleeding when I was doing those changes!
I set the operation when I call performOperation in the toolkit. ('resize' for dimensions, 'compress' for filesize). So, functionally, I am asserting that I am passing the right request to the toolkit.
Your other problems that you noticed (that I can't find a reason to push back) - were quietly fixed...
Posts: 314
here is the new version that takes into consideration the changes in the core... I also looked at the SizeLimitHelper functionality from the perspective of acid test that you described. There are three places in SizeLimitHelper that, if removed would not break any test. I am not sure, if it's "dead code" though, but I am not sure how to simulate the situation.
[/]
Posts: 7994
We're getting really close. Almost all of the stuff I'm suggesting now is cosmetic tweaking, polishing off the details having to do with naming, and style. The only big one left is the test issue. There are a couple of things that are still lingering from prior reviews, so I think that probably one more review (which will hopefully have few comments from me) will do it, then you can commit!
General
{g->formVar var="dimensions[width]"}should be{g->formVar var="SizeLimitOption_dimensions[width]"}. It looks like you're using {$SizeLimitOption} for all the smarty variables so that looks correct to me.These naming changes will have to be propagated all through your view, controller, template and tests.
So my suggestion to you is that you go through the two helper methods and remove anything that you don't understand. This should simplify the code. Then you should be able to write fairly simple tests for everything else because it's all code that you understand. If, later on, we find out that some of that code was necessary because of a bug, we can always pull it back in again (we know where it came from originally anyway) and at that time we can follow the standard XP methodology which is to write a new test to expose the bug and then change the code so that the test passes (and then the bug is fixed).
SizeLimitOptionTest.class
EventHandlerTest.class
AddPhotosOptionTest.class
SetSizeLimitOption.tpl
module.inc
SizeLimitHelper.class:
SizeLimitOption.inc:
SetSizeOption.inc:
list($ret, $errors, $warnings) = SizeLimitHelper::applyLimits($item, 'resize', $args);and skip the intermediate variables altogether. Am I missing something?
[/]
[/]
[/]
[/]
[/]
[/]
[/]
[/]
[/]
[/]
Posts: 314
I realize that I had a philosophical problem with unit testing the helper class. I was trying to build the test cases to have the broadest possible coverage. As such I felt that all my controller cases covered the helper class (and even satisfied the acidity criteria) and specific helper test cases were redundant. Well, unit tests should be as narrow as practical (your example of refactoring was almost eye-opening); so from that perspective, I even pulled one of the test cases (unknown format) from controller to helper, since it is the helper that recognizes the mime format as unknown.
I also corrected all the other observations. I think I caught all XHTML violations (all of them were in input); since I am still on localhost - I can't get the validator to confirm that.
And I missed some of your comments last time (eg., returning array() vs. null) - so I probably missed something here, as well. But I think I combed through all of them...
Posts: 7994
Woo! I think we're there! I have just a few small comments, and one bigger one. Since you've put so much effort into this I really want you to get it in and stop having to chase after all of the changes we make. So, I'll give you the following choice. You can either fix all the little things and commit it, then come back and refactor it to fix the one bigger thing, or you can fix the bigger thing before you commit.
Little things
General
EventHandlerTest.class
module.inc
SetSizeOption.inc:
Big thing
When I look at SizeLimitHelperTest::testUnknownFormat() it becomes glaringly obvious to me that you've got a helper that's returning localized content. We typically try to avoid that because we like to make our helper methods be utilitarian and have little to do with the front end. We try to do all content generation in the front end. This gives us the tremendous advantage of being able to create lots of little light-weight views that depend on the helper methods for the heavy lifting, but can present their own view on what happened.
The easy solution for this is to change applyLimits to return an extra success/fail boolean. If it returns a failure, then you put up the warning. If it returns success, you don't. But the warning is generated in SetSizeOption which is in the UI code.
Here's how I'd do it. Put testUnknownFormat() back into the SizeLimitOptionTest, that way you guarantee that unknown objects return the warning. Now you have it in both places and both work. Then what you can do is update the applyLimit tests in SizeLimitHelperTest so that it expects the appropriate true/false instead of the warning. Now you'll have to fix the code in SizeLimitHelper to make those tests pass. But in the process you'll still have to maintain the invariant that you created in SizeLimitOptionTest. Once both sets of unit tests pass, you're done. You don't even need to verify it via the UI -- once the tests pass you KNOW that you've got it right.[/]
[/]
[/]
[/]
Posts: 12
I really appreciate everyones effort to complete this size limiter. It will save alot of time! I have been watching the progress and was wondering how it is coming and if it is usable/stable now.
Posts: 8601
oops, no one made an announcement here... the module is now in cvs!