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

Sun May 13 10:26:13 EDT 2012

Ok, because I didn't get more feedback I'm pushing the changes today to
master.
I will be looking jenkins to see if something else gets affected, please
get in touch with me if you notice something wrong before I do.
Cheers

On Fri, May 11, 2012 at 7:16 PM, Mauricio Salatino <salaboy at gmail.com>wrote:

> @Marco: Hehe totally agree with the approach. I just don't want to make
> those reports looks better, I want to be sure that the code that we have is
> good.
>
> I've already migrated the changes to the transport projects, which it was
> easier than I thought, meaning that the changes doesn't affect too much. I
> will delete all the commented classes as well.
>
> I'm ready to push now, but I will wait for a while to see if I get more
> feedback. I'm confident with fixing issues with the callbacks if they
> appear. If you guys knows where the callbacks are being used in the
> jbpm-console please let me know.
> Cheers
>
>
>
> On Fri, May 11, 2012 at 12:48 PM, Marco Rietveld <mrietvel at redhat.com>wrote:
>
>>  Oh yeah: coverage reports.
>>
>> IMHO, coverage reports are like weather reports: good to check now and
>> then, but not something you want to plan around.
>> Also, wrt agile/efficiency, we're better off making sure that the tests
>> do actually test what we want them to -- which is pretty much exactly what
>> you've been doing, Mauricio.
>>
>> 11-05-12 11:36, 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
>>>
>>>
>>
>>
>> --
>> 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 -
>
>

-- 
 - MyJourney @ http://salaboy.wordpress.com
 - Co-Founder @ http://www.jugargentina.org
 - Co-Founder @ http://www.jbug.com.ar

 - Salatino "Salaboy" Mauricio -
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.jboss.org/pipermail/jbpm-dev/attachments/20120513/ea6e793d/attachment.html 