[dev] Continuous Integration for Horde (was: Re: [commits] Horde branch master updated. 1e394986ba8dd83a6f2854f51c361f72fce9e7b5)
Gunnar Wrobel
p at rdus.de
Sat Feb 27 17:55:13 UTC 2010
Hi Jan,
Quoting Jan Schneider <jan at horde.org>:
> I'm not talking for Michael obviously, but I'm still gonna add my two cent.
>
> Zitat von Gunnar Wrobel <p at rdus.de>:
>
>> Hi Micheal,
>>
>> Quoting Michael M Slusarz <slusarz at horde.org>:
>>
>> [snip]
>>
>>>
>>> commit e48b8a54fc54a179be749967a6a8b6b73cc1162e
>>> Author: Michael M Slusarz <slusarz at curecanti.org>
>>> Date: Mon Feb 15 23:27:31 2010 -0700
>>>
>>> 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.
>
>> 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 have a high interest in doing so but of course it is always hard to
find the time. Nevertheless it is important to me as I consider
running the tests manually before each commit to be a little bit of an
additional burden and hard to enforce. For many commits one can
assumes that no test was affected. So one commits right away. Of
course sometimes this assumption does not hold. I consider it
sufficient to get an email from your friendly build bot in these cases.
I've seen the system in action for the past half year and it is indeed
very helpful to have it.
A while ago I have briefly looked at some of the available solutions.
I admit I don't like the Java approaches like phpUnderControl very
much. The project that uses Horde_Notification and that I'm involved
in runs bitten (http://bitten.edgewall.org/). It is trac based and
coded in Python. It is not bad but I still consider it too much of an
overhead for Horde. We don't need another source code viewer that
links potential build failures with commits.
So I lean towards something really lightweight that basically runs
framework/bin/test_framework for each commit and sends a mail to
dev at horde.org on failure. A build report and a link to git.horde.org
should be possible.
Would that match your expectations?
Cheers,
Gunnar
>
>> 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.
>
>> 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. :)
>>
>> 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?
>>
>>>
>>> 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.
>
>> 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.
>>
>> Cheers,
>>
>> Gunnar
>>
>> [snip]
>>
>>
>>
>>
>
>
>
> Jan.
>
> --
> Do you need professional PHP or Horde consulting?
> http://horde.org/consulting/
>
>
> --
> 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/16e0f924/attachment.bin>
More information about the dev
mailing list