From: "Yair Zaslavsky" <yzaslavs(a)redhat.com&gt; To: "engine-devel" <engine-devel(a)ovirt.org&gt; Sent: Tuesday, March 27, 2012 7:33:18 PM Subject: [Engine-devel] Getting rid of PowerMock? Hi all, As (almost) all of us can see, Running BLL tests both takes considerable time, and also we have to take class loading dependencies between classes with static methods when we use mockStatic - IMHO, this is kinda frustrating. Maybe we should start get rid of PowerMock. Here is what I'm thinking of - Currently, as we use no IoC for DAOs , for tested class we do not use DbFacade.getInstance().getXXXDao() instead we define: protected XXXDao getXXXDao() { return DBFacade.getInstance().getXXXDao(); } And then in our tests we use Mockito to define how getXXDao acts in the test. The following can be achieved also for config values , the following way - protected <T> T getConfig(ConfigValues key) { return Config.<T> GetValue(key); } And then in code (for example, in a query test) - doReturn(5).when(query).<Integer> getConfig(any(ConfigValues.SomeIntegerConstant)); This effort can be done in small steps - a. Define getConfig method in base classes (i.e AuditLoggalbeBase). b. Rewrite parts (i.e Commands, and their tests) step by step (small commits) Thoughts and ideas are more than welcome, Yair _______________________________________________ Engine-devel mailing list Engine-devel(a)ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel

From: "Yair Zaslavsky" <yzaslavs(a)redhat.com&gt; To: "Laszlo Hornyak" <lhornyak(a)redhat.com&gt; Cc: "engine-devel" <engine-devel(a)ovirt.org&gt; Sent: Wednesday, March 28, 2012 9:31:34 AM Subject: Re: [Engine-devel] Getting rid of PowerMock? On 03/28/2012 09:23 AM, Laszlo Hornyak wrote: > Hi, > > How much faster could the unit tests run after removing powermock > and using the instance methods instead? Faster. I remind you we reached OutOfMemory when using PowerMock (it consumes lots of memory) and we had to split the tests and run them on separate java processes.

> > I like this idea anyway because it could be a step towards > dependency injection. > Could be a more motivating long term objective than just to get rid > of powermock :) I totally agree here. > > Laszlo > > ----- Original Message ----- >> From: "Yair Zaslavsky" <yzaslavs(a)redhat.com&gt; >> To: "engine-devel" <engine-devel(a)ovirt.org&gt; >> Sent: Tuesday, March 27, 2012 7:33:18 PM >> Subject: [Engine-devel] Getting rid of PowerMock? >> >> Hi all, >> As (almost) all of us can see, >> Running BLL tests both takes considerable time, and also we have >> to >> take >> class loading dependencies between classes with static methods >> when >> we >> use mockStatic - IMHO, this is kinda frustrating. >> Maybe we should start get rid of PowerMock. >> Here is what I'm thinking of - >> Currently, as we use no IoC for DAOs , for tested class we do not >> use >> DbFacade.getInstance().getXXXDao() >> >> instead we define: >> >> protected XXXDao getXXXDao() { >> return DBFacade.getInstance().getXXXDao(); >> } >> >> And then in our tests we use Mockito to define how getXXDao acts >> in >> the >> test. >> >> The following can be achieved also for config values , the >> following >> way - >> >> protected <T> T getConfig(ConfigValues key) { >> return Config.<T> GetValue(key); >> } >> >> And then in code (for example, in a query test) - >> >> doReturn(5).when(query).<Integer> >> getConfig(any(ConfigValues.SomeIntegerConstant)); >> >> >> This effort can be done in small steps - >> a. Define getConfig method in base classes (i.e >> AuditLoggalbeBase). >> b. Rewrite parts (i.e Commands, and their tests) step by step >> (small >> commits) >> >> Thoughts and ideas are more than welcome, >> >> Yair >> _______________________________________________ >> Engine-devel mailing list >> Engine-devel(a)ovirt.org >> http://lists.ovirt.org/mailman/listinfo/engine-devel >>

Hi all, As (almost) all of us can see,

Running BLL tests both takes considerable time, and also we have to take class loading dependencies between classes with static methods when we use mockStatic - IMHO, this is kinda frustrating. Maybe we should start get rid of PowerMock.

Here is what I'm thinking of - Currently, as we use no IoC for DAOs , for tested class we do not use DbFacade.getInstance().getXXXDao() instead we define: protected XXXDao getXXXDao() { return DBFacade.getInstance().getXXXDao(); } And then in our tests we use Mockito to define how getXXDao acts in the test. The following can be achieved also for config values , the following way - protected <T> T getConfig(ConfigValues key) { return Config.<T> GetValue(key); } And then in code (for example, in a query test) - doReturn(5).when(query).<Integer> getConfig(any(ConfigValues.SomeIntegerConstant));

This effort can be done in small steps - a. Define getConfig method in base classes (i.e AuditLoggalbeBase).

b. Rewrite parts (i.e Commands, and their tests) step by step (small commits) Thoughts and ideas are more than welcome, Yair _______________________________________________ Engine-devel mailing list Engine-devel(a)ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel

On 03/27/2012 07:33 PM, Yair Zaslavsky wrote: > Hi all, > As (almost) all of us can see, > Running BLL tests both takes considerable time, and also we have to take > class loading dependencies between classes with static methods when we > use mockStatic - IMHO, this is kinda frustrating. > Maybe we should start get rid of PowerMock. > Here is what I'm thinking of - > Currently, as we use no IoC for DAOs , for tested class we do not use > DbFacade.getInstance().getXXXDao() > > instead we define: > > protected XXXDao getXXXDao() { > return DBFacade.getInstance().getXXXDao(); > } > +1 for this. IMO all DAOs should be reachable from the CommandBase as this is a repeated code scattered in the various commands. I'd suggest to create an interface that declares all of available DAOs. The new interface should be implemented by both DbFacade (which already contains that implementation) and CommandBase to assure new DAOs added to DbFacade will be added to the CommandBase as well.

> And then in our tests we use Mockito to define how getXXDao acts in the > test. > > The following can be achieved also for config values , the following way - > > protected <T> T getConfig(ConfigValues key) { > return Config.<T> GetValue(key); > } > > And then in code (for example, in a query test) - > > doReturn(5).when(query).<Integer> > getConfig(any(ConfigValues.SomeIntegerConstant)); > > > This effort can be done in small steps - > a. Define getConfig method in base classes (i.e AuditLoggalbeBase). > b. Rewrite parts (i.e Commands, and their tests) step by step (small > commits) > > Thoughts and ideas are more than welcome, Searching for the static mocked classes brought the following list: 1 AuditLogDirector.class 1 CommandsFactory.class 1 FileUtil.class 1 LoggerFactory.class 1 QuotaHelper.class 1 QuotaManager.class 1 SchedulerUtilQuartzImpl.class 1 StopVdsCommand.class 1 TransactionSupport.class 1 VmTemplateCommand.class 2 CpuFlagsManagerHandler.class 2 StorageDomainSpaceChecker.class 2 VdsGroupDAO.class 3 EjbUtils.class 3 LogFactory.class 3 MultiLevelAdministrationHandler.class 3 StorageHelperDirector.class 3 VmTemplateHandler.class 6 ImagesHandler.class 6 VmHandler.class 10 Backend.class 28 Config.class 33 DbFacade.class So handling the Config and DbFacade will be a major step toward powermock elimination. (list retrieved by running the next command from ovirt-engine head: find . -name "*Test.java" -exec grep -r "@PrepareForTest" {} \; | tr "{)}" " " | cut -d \( -f2 | tr "," "\n" | tr -d " " | sort | uniq -c | sort -n) > > Yair > _______________________________________________ > Engine-devel mailing list > Engine-devel(a)ovirt.org > http://lists.ovirt.org/mailman/listinfo/engine-devel _______________________________________________ Engine-devel mailing list Engine-devel(a)ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel

> On 03/27/2012 07:33 PM, Yair Zaslavsky wrote: >> Hi all, >> As (almost) all of us can see, >> Running BLL tests both takes considerable time, and also we have to >> take >> class loading dependencies between classes with static methods when >> we >> use mockStatic - IMHO, this is kinda frustrating. >> Maybe we should start get rid of PowerMock. >> Here is what I'm thinking of - >> Currently, as we use no IoC for DAOs , for tested class we do not >> use >> DbFacade.getInstance().getXXXDao() >> >> instead we define: >> >> protected XXXDao getXXXDao() { >> return DBFacade.getInstance().getXXXDao(); >> } >> > +1 for this. IMO all DAOs should be reachable from the CommandBase as > this is a repeated code scattered in the various commands. > > I'd suggest to create an interface that declares all of available > DAOs. > The new interface should be implemented by both DbFacade (which > already > contains that implementation) and CommandBase to assure new DAOs > added > to DbFacade will be added to the CommandBase as well. I'm not sure an interface is needed, since you won't be replacing it with different implementations. However, maybe a good approach is to put this method in bases class: protected DbFacade getDbFacade() { return DbFacade.getInstance(); }

This of course can be spied on to return a mock of the facade, and this mock can in turn return whatever mock DAOs you need. >> And then in our tests we use Mockito to define how getXXDao acts in >> the >> test. >> >> The following can be achieved also for config values , the >> following way - >> >> protected<T> T getConfig(ConfigValues key) { >> return Config.<T> GetValue(key); >> } >> >> And then in code (for example, in a query test) - >> >> doReturn(5).when(query).<Integer> >> getConfig(any(ConfigValues.SomeIntegerConstant)); >> >> >> This effort can be done in small steps - >> a. Define getConfig method in base classes (i.e AuditLoggalbeBase). >> b. Rewrite parts (i.e Commands, and their tests) step by step >> (small >> commits) >> >> Thoughts and ideas are more than welcome, > Searching for the static mocked classes brought the following list: > > 1 AuditLogDirector.class > 1 CommandsFactory.class > 1 FileUtil.class > 1 LoggerFactory.class > 1 QuotaHelper.class > 1 QuotaManager.class > 1 SchedulerUtilQuartzImpl.class > 1 StopVdsCommand.class > 1 TransactionSupport.class > 1 VmTemplateCommand.class > 2 CpuFlagsManagerHandler.class > 2 StorageDomainSpaceChecker.class > 2 VdsGroupDAO.class > 3 EjbUtils.class > 3 LogFactory.class > 3 MultiLevelAdministrationHandler.class > 3 StorageHelperDirector.class > 3 VmTemplateHandler.class > 6 ImagesHandler.class > 6 VmHandler.class > 10 Backend.class > 28 Config.class > 33 DbFacade.class > > So handling the Config and DbFacade will be a major step toward > powermock elimination. > (list retrieved by running the next command from ovirt-engine head: > find > . -name "*Test.java" -exec grep -r "@PrepareForTest" {} \; | tr "{)}" > " > " | cut -d \( -f2 | tr "," "\n" | tr -d " " | sort | uniq -c | sort > -n) > > >> Yair >> _______________________________________________ >> Engine-devel mailing list >> Engine-devel(a)ovirt.org >> http://lists.ovirt.org/mailman/listinfo/engine-devel > _______________________________________________ > Engine-devel mailing list > Engine-devel(a)ovirt.org > http://lists.ovirt.org/mailman/listinfo/engine-devel > _______________________________________________ Engine-devel mailing list Engine-devel(a)ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel