[jbpm-dev] jbpm human task core tests refactorings <- feedback plz!

[Maciej Swiderski]

+1 for cleaning up the test, it takes quite some time to run all of them.

Regarding changes of package good to keep in mind:
- installer stuff that it declares DefaultUserGroupCallback in both web.xml
and jbpm.usergroup.callback.properties.
- human task war tests could declare...

There should be LDAPUserInfo class that could go into identity package as
well.

Coverage report look quite ok :) and improvement is always welcome.

Cheers
Maciej

2012/5/11 Mauricio Salatino <salaboy at gmail.com>

> Sure, I will do that if all the other points make sense, and I will not
> push the commented out tests :)
>
>
> On Fri, May 11, 2012 at 12:04 PM, Marco Rietveld <mrietvel at redhat.com>wrote:
>
>>  Hi Mauricio,
>>
>> Feel free to delete the code instead of commenting it out, especially if
>> you can't use @Ignore.
>>
>> Commented out code is more often confusing than not. :)
>>
>> Thanks,
>> Marco
>>
>> 11-05-12 10:46, Mauricio Salatino:
>>
>> Hi guys,
>> I was working yesterday trying to understand why we have 390 test
>> running inside jbpm-human-task-core and if they are really necessary.
>> This is my initial approach so forgive me if I'm missing something, the
>> following notes and changes are what I found until now:
>>
>>  1)  All the test classes using directly TaskClient should be removed,
>> now everything should go through TaskService and AsyncTaskService
>> test which are placed inside async and sync. For that reason all the test
>> inside org.jbpm.task.service are commented out.
>> I've checked that all the test classes that were using TaskClient now has
>> their correspondency inside the org.jbpm.task.service.base.async package.
>> Where the only important change is from TaskClient to AsyncTaskService.
>> We were running the same tests for both wasting a lot of time.
>> Now the base.sync package tests everything using the TaskService
>> interface. Right now there are not the same tests for the sync and the
>> async interface but I can migrate them soon.
>>
>>
>>  2) Reviewing the org.jbpm.task.service package I found all the
>> UserGroupCallback tests and the class called BaseTestNoUserGroupSetup,
>> which basically run almost
>> all the test using no hardcoded users and using the UserCallbacks
>> resolutions. Talking yesterday with Tiho, we both agree that we should
>> remove the tests
>> that are using hardcoded users and just leave the one that are using
>> callbacks to avoid running unnecessary tests. Just doing this we will
>> remove a lot of overhead
>> and we can have a simplified way of organizing and running the tests.
>> I've moved all the UserGroupCallback tests to the async package and migrate
>> them to use the AsyncTaskService instead of the TaskClient.
>>
>>  3) I've created a package called org.jbpm.task.identity to place the
>> LDAP stuff which is ignored and the UserGroupCallBack stuff that is not
>> using the TaskService/TaskClient stuff, when I was doing that I notice that
>> those tests uses some properties files that needs to be placed inside the
>> same package structure to be picked up, for that reason I've moved two
>> classes inside src/main/java to the following new
>> package: org.jbpm.task.identity -> UserGroupCallbackManager,
>> UserGroupCallback, DefaultUserGroupCallbackImpl, LDAPUserGroupCallbackImpl,
>> which I believe that is better than have the UserGroupCallback stuff mixed
>> with the task.service stuff. I notice that the
>> package: org.jbpm.task.service is a wild area, that we should avoid as much
>> as possible. I know that this is a change that can impact in other modules
>> like the jbpm-console but I think that it will avoid future troubles. I
>> really need your feedback on this.. to see if you guys think that this can
>> cause a lot of troubles.
>>
>>  4) for the same reason as 1 I've commented out all the tests
>> inside org.jbpm.task.service.test, those were using the TaskClient and
>> because the async and sync test are already provided we don't miss
>> anything. The only thing that we need to do inside
>> the org.jbpm.task.service.test package is to align these tests to cover all
>> the base async and sync tests, which is not the case right now, but it's
>> simple to do it.
>>
>>  5) I've added covertura to the jbpm-human-task-core project to do an
>> initial analysis about what we are covering with our tests. Attached is the
>> initial chart that I would love to see improved in the following months :)
>>
>>
>>  6) I need to work on the other modules to propagate these changes as
>> well, but for doing that I need your feedback on the previous points, I
>> think that during the weekend I will be able to finish the other modules,
>> if we all are OK with this suggestions.
>>
>>  Cheers
>>
>>
>>
>>  --
>>  - MyJourney @ http://salaboy.wordpress.com
>>  - Co-Founder @ http://www.jugargentina.org
>>  - Co-Founder @ http://www.jbug.com.ar
>>
>>  - Salatino "Salaboy" Mauricio -
>>
>>
>>
>> --
>> jBPM/Drools developer
>> Utrecht, the Netherlands
>>
>>
>
>
> --
>  - MyJourney @ http://salaboy.wordpress.com
>  - Co-Founder @ http://www.jugargentina.org
>  - Co-Founder @ http://www.jbug.com.ar
>
>  - Salatino "Salaboy" Mauricio -
>
>
> _______________________________________________
> jbpm-dev mailing list
> jbpm-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/jbpm-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.jboss.org/pipermail/jbpm-dev/attachments/20120511/b7068092/attachment-0001.html