[dev] [commits] Horde branch master updated. 1e394986ba8dd83a6f2854f51c361f72fce9e7b5

[Gunnar Wrobel]

Hi Micheal,

Quoting Michael M Slusarz <slusarz at horde.org>:

> Quoting Jan Schneider <jan at horde.org>:
>
>> Zitat von Gunnar Wrobel <p at rdus.de>:
>>
>>> Quoting Michael M Slusarz <slusarz at horde.org>:
>>>>
>>>>  Another Notification rewrite.
>>>>
>>>>  First, fix Bug #8870. Fixed by removing Nag specific event type
>>>>  (nag.alarm).  However, this was just a symptom of a larger problem.
>>>>
>>>>  The problem: using application specific Notification handlers to handle
>>>>  application specific event types.  The problem comes when switching
>>>>  between applications.  Since these application handlers don't have any
>>>>  knowledge of each other, events created by one handler may not be able
>>>>  to be displayed when notify() was eventually called, because another
>>>>  status handler had replaced the original handler.
>>>>
>>>>  The solution: all notifications need to be handled by a single,
>>>>  centralized source - namely, the horde-level handlers.  Application
>>>>  specific details are instead injected into the horde-level handler to
>>>>  extend behavior.
>>>>
>>>>  While reworking the code, also provided opportunity to remove all
>>>>  application-specific code from Notification.  Horde-specific
>>>>  instantiation (i.e. adding Horde logging and Alarm decorators) is now
>>>>  done in Horde_Core rather than in the base Notification object.
>>>
>>> Cool, great! Thanks.
>>>
>>>>
>>>>  Additionally, rework some of the complexity added to the package.  I
>>>>  believe the goal of the recent Notification changes was to make the
>>>>  Notification package testable and/or usable outside of a base Horde
>>>>  install.
>>>
>>> Doing such a rewrite for testing purposes would seem wrong to me.  
>>> The  only intention was to make the package usable outside of a  
>>> standard  Horde installation.
>>>
>>> I hope it is okay to transform Horde framework packages this way.  
>>> My  expectation has always been that the framework packages should  
>>> work  that way and be as independent of each other as reasonable.  
>>> Probably  originates from the fact that the Kolab server always  
>>> relied to a  larger degree on the framework than on the  
>>> applications. This has been  somewhat difficult in the past  
>>> because of the tendency to use global  scope in the current code.  
>>> But things are improving of course.
>>
>> This expectation is absolutely correct.
>
> I agree with Jan.  My mistake in trying to guess your original  
> motivation.  Obviously, the it would be great if all of our  
> framework packages (except 1 - see below) are usable outside of a  
> Horde application.
>
> The one exception is going to be the Core package.  This is the  
> place where all of the glue between Horde *application* and Horde  
> *framework* is going to go.  And with the recent changes that are  
> starting to make this separation a reality, one overriding thought  
> has come to mind: why do we even have a Core package?  It seems to  
> me that everything in Core should be moved to the horde application  
> package.  Is this the eventual goal?
>
>>> One question concerning the unit tests: I added them because they  
>>> are  required to a certain degree by the project where I'm using  
>>> the  Horde_Notification package. I am however not 100% certain if  
>>> these  tests are useful to the Horde project. I see a tendency  
>>> towards  PHPUnit testing within Horde. But we are also ignoring  
>>> them to a  certain degree as we don't seem to care much if they  
>>> break. And I feel  that at that point unit tests may become more  
>>> of a nuisance rather  than being helpful.
>>
>> We rather need more tests than less. The fact that they break from  
>> time to time is probably due to us not having used tests that much  
>> in the past. We still have to get used to run them after each  
>> change. Some continuous integration would probably help too, but I  
>> don't see anyone having the resources at the moment to set this up.
>
> I would agree with the comment that test are good, but not  
> necessarily with the comment that more tests are better.  There can  
> be a problem with too many tests, and then you just run into the  
> issue of having to add additional maintenance costs for the test  
> code on top of the actual code.  There's a fine balance between  
> trying to test all permutations of the code vs. testing that you can  
> replicate a big-picture result.
>
> When I had finished the rewrite, I found that the big-picture  
> results were correct (i.e. given a particular message, the  
> notification output was as expected) but I found myself constantly  
> going in and tweaking what some intermediate steps were doing.  But  
> this probably also had a lot to do with the design of the various  
> Notification libraries - namely that there was a bunch of public  
> functions.  Once I simplified the API and moved a lot of things to  
> protected/private functions, the tests made a lot more sense - they  
> focused more on "given this real-world input, we should expect this  
> output" vs. "let's try to fake some input in order to test the  
> output of a particular intermediate function used."
>
> I'm not a tremendously big fan of PHPUnit from what I know about it  
> so far; it seems somewhat hackish to me - e.g. naming functions like  
> testTryingToDescribeAnEntireTestInA100CharacterFunctionNameUsingStudlyCapsIsNotTheMostClearWayToIdentifyAFailingTest.  Not sure if this is a limitation of PHPUnit or simply a case where we have not yet used all capabilities available in the package.  This makes determining what tests were failing a matter of bashing through a bunch of Exception-like text dumps.  Maybe I am simply using phpunit wrong - a wiki entry might be  
> useful.

These long function names are not necessary at all. They are just a  
current habit of mine not more. I have been using PHPUnit for a while  
now but I'm nowhere near the end of my learning there. When I started  
writing tests I was primarily focused on testing the external  
interfaces of a package. During the last half year I was taught to  
follow what is sometimes called the "mockist" approach. Breaking down  
classes and functions to rather small segments and testing these with  
short tests (one unit test, one assertion). The tests shouldn't cross  
class boundaries so that you start mocking the world around you.

This results in the problem you were seeing: As the tests focus on the  
implementation rather than the external interfaces they require a good  
deal of maintenance during refactoring. This may be a problem.  Though  
I think what you did was okay: Throw away the stuff you don't need  
anymore. The tests are usually rather short anyway so they should not  
cost that much.

I do however think I still need to catch up on the topic of testing a  
little bit. I'm not yet decided what my favorite style is. It is  
already good to see what the other Horde devs think and maybe a wiki  
page with some guidance would be good.

One last thing concerning PHPUnit: I think it is an extremely powerful  
tool and not hackish at all. You can really do a lot with it and can  
use with very different styles.

>
> I just have to get into the habit of running the unit tests when  
> making a change.  This is something I admittedly need to become  
> better at.
>
>>> Did the tests make sense to you while rewriting Horde notification  
>>> or  did you consider them to be a hindrance?
>>>
>>>>  But these changes also made the code unreadable, redundant,
>>>>  and overly complex.
>>>
>>> This sounds very negative which is why I am asking whether making  
>>> the  packages more independant is actually okay.
>>
>> It's absolutely okay and should be forced even more.
>
> I probably should have described this a bit more - my original  
> comments weren't meant to be derogatory in the least.  But I will  
> admit there was a little bit of frustration at the time I wrote the  
> commit message, since it took me several hours of re-reading the  
> code multiple times to figure out what the heck it was trying to  
> accomplish.
>
> I think the frustration had more to do with the coding decisions  
> rather than the unit tests.  As mentioned in the commit message, the  
> use of a PHP interface rather than extending a PHP class made the  
> code extremely unwieldy.  And it led to a bunch of tests in the  
> testing suite that didn't really test anything.  Most of these tests  
> were of the pattern "test if this function in the interface calls a  
> given parent function".  Once I converted to a normal class/extends  
> pattern all these tests weren't needed.
>
> My one bit of constructive criticism is that
>
>
> The commit message was as long as it was not to be confrontational  
> or bragging, but instead as a kind of brain dump of things that I  
> learned while tacking the problem.  Whether or not it is/was of any  
> usefulness to anybody but myself... that's a different question.
>

It was definitely useful to me. It is always good to have someone else  
look at your code at get some comments.

>>> Concerning the changes I did I can easily accept that a lot of the  
>>>  stuff might not be necessary and too complex for the package. I  
>>> looked  at your changes and they made sense to me. So there is  
>>> hope that the  next time I'll do things like that it will be  
>>> somewhat cleaner or more  effective. :)
>
> There is always going to be a difference in opinion when it comes to  
> coding.  Give 100 programmers a problem, and you will get 100  
> opinions on the best way to do that.
>
> But I feel I am fair when it comes to analyzing an issue like this  
> (obviously, this can be debated... :) ).  And my thinking was as  
> follows:
>
> 1.) I have been working with the Notification code for years
> 2.) I had a somewhat simple issue - figuring out why a notification  
> in an application wasn't working
> 3.) Tracing the code to the framework package, I was absolutely  
> stumped as to what the various classes were trying to do.
>
> So that's when I started trying to figure out the motivation as to  
> why the code was changed in the first place.
>
>>> Another thing would be if the desire to make the packages more   
>>> independent meets resistance in itself. That would mean that I   
>>> shouldn't use them at all. But my impression was that this was not  
>>>  what your were aiming at with your comment, right?
>
> Correct.
>
>>>>  e.g. using interfaces where simple class extensions
>>>>  make much more sense (IMHO - there are very few cases where an interface
>>>>  makes more sense than an abstract class. Using interfaces for the
>>>>  Handler class was simply overkill.  Out of the 10 methods defined, there
>>>>  are only 2 methods useful for decorator purposes - push() and notify().
>>>>  And any given decorator won't even use both of these.  Having to contort
>>>>  code to do things like chaining handlers to achieve this in an interface
>>>>  pattern was almost impossible to follow.  It is much simpler to simply
>>>>  add decorators directly to the base handler object.
>>>
>>> Ack and thanks for the hints.
>>>
>>> One remaining question concerning Horde_Notification: Currently   
>>> framework/Notification/lib/Horde/Notification/Listener/Status.php  
>>> and   
>>> framework/Notification/lib/Horde/Notification/Event/Status.php  
>>> have an  optional dependency on Horde_Mobile and Horde_Nls  
>>> respectively. Both  are only required if a specific option has  
>>> been set. Is that  sufficient to make the dependency in  
>>> package.xml optional or is  another check with "class_exists"  
>>> required in order to disable the  dependency in case the  
>>> corresponding packages are not installed?
>>
>> That makes sense to me.
>
> Well... my ultimate desire is to remove Horde_Mobile entirely, and  
> possibly very soon.  It has been superseded (or will be superseded)  
> by Horde_View anyway - or whatever other output/template/view system  
> will eventually decide on.

Okay, I guess I'll add the two lines then.

>
>>> And finally: Is it okay if I grab your recent Horde_LoginTasks and  
>>>  start to transform it in a similar way? :) I would like to use it  
>>> in  the very same project as a standalone version and would need  
>>> to remove  the hard dependency on the usual global Horde constants.
>
> That would be great!

Cool. I hope to be able to start next week.

Cheers,

Gunnar

> Details on the best means of converting to reach this goal are  
> obviously still being worked out (this is probably the time to thank  
> you for all your work on Horde_Injector) and further input from  
> someone converting another library would be great.
>
> Sorry for any misunderstanding I may have caused. And much thanks to  
> all the work you have done for the project recently.
>
> michael
>
> -- 
> ___________________________________
> Michael Slusarz [slusarz at horde.org]
>
>
> -- 
> Horde developers mailing list - Join the hunt: http://horde.org/bounties/
> Frequently Asked Questions: http://horde.org/faq/
> To unsubscribe, mail: dev-unsubscribe at lists.horde.org
>




-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: Digitale PGP-Unterschrift
URL: <http://lists.horde.org/archives/dev/attachments/20100227/975f89b9/attachment-0001.bin>