PowerMock sometimes causes other TestNG tests to fail

[GoogleCodeExporter]

Please see the attached small demo project with two test classes that 
demonstrate the problem.

My environment is the following:
  Windows 7 Enterprise 64-bit
  JDK 1.6.0_22
  Maven 2.2.1
  PowerMock 1.4.6
  TestNG 5.14.1

ServiceTest uses PowerMock to mock class with final modifier (@PrepareForTest 
and @ObjectFactory are there). XmlReaderTest just creates new instance of 
SAXParser. Each of the tests succeed when they run independently. However 
XmlReaderTest fails when running the whole suite with the following exception:

java.lang.ClassCastException: 
com.sun.org.apache.xerces.internal.parsers.SAXParser cannot be cast to 
org.xml.sax.XMLReader

FAQ describes this issue and advises to solve it by adding 
@PowerMockIgnore({"javax.xml.*", "org.xml.sax.*"}) to XmlReaderTest.
And this fix works. BUT why does PowerMock alter behavior of other tests? It is 
definetely not OK.

There are other classloading issues that are not demoed in this sample. The 
overall idea is that tests that do not use PowerMock must not be affected by it.

Partially the problem is that there is only one ObjectFactory per suite and 
this ObjectFactory is responsible for creating all test class instances. 
I have solved the problem by writing and applying custom ObjectFactory that 
inspects test class and delegates creation of test class instance to 
PowerMockObjectFactory ONLY if test class has PowerMock annotations. Otherwise 
ObjectFactory delegates creation to default ObjectFactoryImpl.

It seems to me that similar ObjectFactory logic should be the part of the 
library. Classloader that PowerMock uses causes issues for other tests 
frequently and sometimes it is VERY HARD to understand what is going wrong.

Original issue reported on code.google.com by flush...@gmail.com on 16 Dec 2010 at 6:55

Attachments:

• affecting-other-tests.zip

[GoogleCodeExporter]

Thanks a lot for the good example and explanation. Would it be possible for you 
to share your ObjectFactory with us by e.g. sending us a patch? 

Original comment by johan.ha...@gmail.com on 26 Dec 2010 at 8:39

• Changed state: Accepted
• Added labels: Milestone-Release1.5

[GoogleCodeExporter]

Thank you for the response. Please see the attachment with custom ObjectFactory 
and annotation that controls usage of PowerMockObjectFactory.

I have to note that there is a significant problem with my implementation of 
ObjectFactory: no cleanup call is made for normal non-PowerMock tests 
(MockRepository.clear()).

Original comment by flush...@gmail.com on 28 Dec 2010 at 5:57

Attachments:

• ConditionalPowerMockObjectFactory.java
• EnablePowerMockObjectFactory.java

[GoogleCodeExporter]

Thanks! Do you think it would be possible to somehow integrate this in the 
standard PowerMockObjectFactory instead of having an additional one?

Original comment by johan.ha...@gmail.com on 28 Dec 2010 at 7:02

[GoogleCodeExporter]

Yes, it is possible BUT we have to resolve the problem of proper test cleanup 
first.

Ordinary tests that do not declare PowerMock annotations may still use 
PowerMock.createMock, etc. thus updating the MockRepository. If MockRepository 
will not be cleared after such test, other tests may be affects if verifyAll is 
executed. Do you understand what I mean?

Original comment by flush...@gmail.com on 28 Dec 2010 at 7:09

[GoogleCodeExporter]

Yes I understand what you mean. But I tend regard this as a user error. If 
you're using any PowerMock functionality (excluding Whitebox) you _must_ use 
the PowerMockRunner or PowerMockRule (in JUnit) or PowerMockObjectFactory (in 
TestNG). The JUnit runner cleans up previous state on startup, not sure if 
PowerMockObjectFactory does the same, probably not. But it should do so.

Original comment by johan.ha...@gmail.com on 6 Jan 2011 at 12:08

[GoogleCodeExporter]

With PowerMockObjectFactory all test classes within the suite will go through 
'mock classloader' provided by PowerMock. This will cause issues in existing 
unit tests and have nothing to do with PowerMock (e.g. ClassCastExceptions). 
So, it breaks isolation.

Original description of the issue stated that "the overall idea is that tests 
that do not use PowerMock must not be affected by it".

Original comment by flush...@gmail.com on 6 Jan 2011 at 12:19

[GoogleCodeExporter]

Related TestNG JIRA

http://jira.opensymphony.com/browse/TESTNG-422

Original comment by caoi...@gmail.com on 5 Apr 2011 at 11:37

[GoogleCodeExporter]

Great, thanks.

Original comment by johan.ha...@gmail.com on 5 Apr 2011 at 12:28

[GoogleCodeExporter]

The TestNG issue is closed so it looks there is no action item on my part for 
this issue? (just making sure)

Original comment by cbe...@gmail.com on 5 Apr 2011 at 2:21

[GoogleCodeExporter]

cbe: I suppose not.

Flush: Could you please verify if things works as you expect?

Original comment by johan.ha...@gmail.com on 5 Apr 2011 at 2:51

[GoogleCodeExporter]

My understanding was that the TestNG guys don't think that a fix is needed in 
their code, but it is still very difficult to use Powermock in TestNG so I 
think either some sort of accommodation will have to be reached between the two 
projects or some sort of work around will be needed in Powermock.

Possibly the PowerMockObjectFactory could be adapted to only be used when tests 
use @PrepareForTest? 

Original comment by caoi...@gmail.com on 5 Apr 2011 at 2:59

[GoogleCodeExporter]

Ah sorry I didn't read the TestNG issue throughly enough, I thought TestNG had 
implemented a fix already.

Have I understood it correctly that the problem is when you have a PowerMock 
ObjectFactory declared anywhere in your suite then all tests will be using 
PowerMock (i.e. the PowerMock classloader)? If this is the case I suppose we 
could patch the PowerMockObjectFactory along the lines of the 
ConditionalPowerMockObjectFactory attached above but triggering on the 
@PrepareForTest annotation. Is this a nice way to solve it?

Original comment by johan.ha...@gmail.com on 5 Apr 2011 at 6:27

[GoogleCodeExporter]

Possibly, but I still haven't got my head around the cleanup issues described 
above (although they don't seem to be affecting me yet).

Original comment by caoi...@gmail.com on 6 Apr 2011 at 8:16

[GoogleCodeExporter]

I think the problem that flush is talking about is that PowerMock keeps some 
state in something called the MockRepository which is cleared after each test 
method to avoid taking up unnecessary memory. It's the ObjectFactory that takes 
care of cleaning out the state so if the ObjectFactory is not used but you 
still use PowerMock functionality (such as PowerMock.createMock(..)) there will 
be unused state in the MockRepository which is not cleared. For large suites it 
may lead to OOM issues in the end. How ever you should NOT use 
PowerMock.createMock(..) unless you use the ObjectFactory (or JUnit Runner).

I think we could go ahead and implement this. Perhaps it should be made clearer 
in the Javadoc that you're not suppose to use PowerMock without using the 
ObjectFactory/Runner. (Whitebox is OK though).

Original comment by johan.ha...@gmail.com on 7 Apr 2011 at 6:33

[GoogleCodeExporter]

Hi there, sorry for not responding for a while.

@johan It is fine that tests which use PowerMock functionality should leverage 
ObjectFactory. HOWEVER the problem is that tests which DO NOT USE PowerMock are 
still affected by ObjectFactory. It causes issues. Have a look at the project 
I've posted in the bug.

Original comment by flush...@gmail.com on 7 Apr 2011 at 8:55

[GoogleCodeExporter]

But wouldn't it work if we use your approach and makes PowerMockObjectFactory 
"conditional"? I.e. we only use the "PowerMockObjectFactory" (as it is today) 
when we find a @PrepareForTest annotation in the test class and the default 
ObjectFactory for test classes not having this annotation.

So the new PowerMockObjectFactory should be pretty much like your 
ConditionalPowerMockObjectFactory but checking @PrepareForTest instead of 
@EnablePowerMockObjectFactory and delegating to the current 
"PowerMockObjectFactory" (i.e. we need to copy the current implementation to a 
new name).

Original comment by johan.ha...@gmail.com on 7 Apr 2011 at 9:10

[GoogleCodeExporter]

@johan Yes, it makes sense.

Original comment by flush...@gmail.com on 7 Apr 2011 at 9:23

[GoogleCodeExporter]

Ok good. Then I'll implement this asap. Thanks everyone for your help so far.

Original comment by johan.ha...@gmail.com on 7 Apr 2011 at 9:33

[GoogleCodeExporter]

I've committed the changes to trunk. Please help me verify whether it helps or 
not. 

An issue that I encountered with the new implementation is that if you've 
annotated fields to make them eligible for mock injection (e.g. using @Mock) 
you now _must_ use the @PrepareForTest annotation on the test class (or a 
method in the test class) for this to work. I think this is a reasonable trade 
off but it's a non-backward compatible change. 

Original comment by johan.ha...@gmail.com on 7 Apr 2011 at 8:08

[GoogleCodeExporter]

I've tested changes in trunk against sample project (attached to original bug 
report) and found ClassLoader issues.

The problem is that even if you use TestNG's ObjectFactoryImpl for test 
instance creation PowerMockClassloaderObjectFactory may override current 
thread's ClassLoader (mockLoader). It may lead to ClassCastException(s) and 
similar errors in test which doesn't use PowerMock but reside in the same test 
suite.

BTW I've tried to change to source of PowerMockObjectFactory and 
PowerMockClassloaderObjectFactory so that current thread's ClassLoader is set 
to a normal ClassLoader before test instance creation is delegated to 
ObjectFactoryImpl. This fix resolved the issue.

Original comment by flush...@gmail.com on 9 Apr 2011 at 8:00

[GoogleCodeExporter]

You're right, I overlooked the thread's classloader. By "normal" classloader do 
you mean the system classloader or the classloader that loads the 
PowerMockObjectFactory? I suspect we may need to use the latter since otherwise 
I have a feel we can run into issues when tests are run from e.g. OSGi. Do you 
think you could you create a patch?

Thanks a lot for your help!

Original comment by johan.ha...@gmail.com on 9 Apr 2011 at 8:20

[GoogleCodeExporter]

You are right about OSGi and usage of classloader that loads the 
PowerMockObjectFactory. I think I can create a patch, but not earlier than 
Monday (have limited access to my dev env at the moment).

Original comment by flush...@gmail.com on 9 Apr 2011 at 9:42

[GoogleCodeExporter]

It would be nice if you could create a patch. 

I would also be really glad if someone could try out the new Javaagent based 
bootstrapper I've been playing around with during the weekend. If you build 
PowerMock and add powermock-module-testng-agent instead powermock-module-testng 
to your pom PowerMock should bootstrap from the javaagent instead of using the 
custom classloader. This means that you shouldn't have any problems with 
ClassCastException's and other classloading related issues (I don't think it'll 
work in OSGi though).

Original comment by johan.ha...@gmail.com on 10 Apr 2011 at 5:17

[GoogleCodeExporter]

I think I've fixed it in trunk. Please try it out.

Original comment by johan.ha...@gmail.com on 18 Apr 2011 at 9:41

[GoogleCodeExporter]

Tried out the fix, works fine.

Thank you!

Original comment by flush...@gmail.com on 20 May 2011 at 4:48

[GoogleCodeExporter]

Great. Is it alright to close the issue?

Original comment by johan.ha...@gmail.com on 21 May 2011 at 3:44

[GoogleCodeExporter]

Yes!

Original comment by flush...@gmail.com on 21 May 2011 at 10:02

[GoogleCodeExporter]

Original comment by johan.ha...@gmail.com on 22 May 2011 at 7:52

• Changed state: Fixed