Re: [jbpm-dev] jbpm human task core tests refactorings - next steps

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(a)gmail.com&gt;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(a)redhat.com&gt;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(a)gmail.com&gt; >> >>> 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(a)redhat.com&gt;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(a)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 -