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

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