Javaagent falls if JaCoCo Agent is also turned in

[GoogleCodeExporter]

What steps will reproduce the problem?
1. Create a powermock test with Mockito and junit Javaagent rule. Passes...
2. Install Eclipse EclEmma Plugin 2.0 Beta which uses the JaCoCo Javaagent
3. Run the Test again with "coverage as"

What is the expected output? What do you see instead?
Expected: Passing Test 
Instead: Failed to redefine class

What version of the product are you using? On what operating system?
1.4.10 Win64
Eclipse Indigo 3.7.1
Latest EclEmma 2.0 Plugin with JaCoCo

Please provide any additional information below.

First of all, the Javaagent Support is awesome! Keep going with it, it is a lot 
more powerful than the classloader approach. I suspect that multiple Agens will 
cause this problem...

Original issue reported on code.google.com by mag...@jungsbluth.de on 22 Nov 2011 at 9:02

[GoogleCodeExporter]

Hi! I haven't experimented with multiple agents and I'm not entirely how it's 
supposed to work in Java even. Unfortunately I'm maintaining PowerMock totally 
in my spare time and it requires a great amount time which I don't have right 
now :/ Also I'm not even using Eclipse anymore so I would be really glad if you 
could help out with this in any way.

Original comment by johan.ha...@gmail.com on 25 Nov 2011 at 11:32

• Changed state: Accepted
• Added labels: Priority-Low
• Removed labels: Priority-Medium

[GoogleCodeExporter]

It is actually possible to test this without eclipse. Jacoco can be run from 
the command line using the -javaagent switch. I will at least try to produce a 
test case. Cheers and keep up the good work!

Original comment by mag...@jungsbluth.de on 3 Dec 2011 at 9:52

[GoogleCodeExporter]

I analyzed the problem a bit:
- The redefining actually fails for the Test class
- JaCoCo is first to instrument the Test class
- JaCoCo adds a private final method
- PowerMockClassRedefiner ignores the already loaded class in method redefine 
since Javassist loads the .class file directly from disk thus wiping out the 
previously added private final method
- At last the redefinition fails (I guess because the missing private final 
method)

I can't see any easy way to provide patch for this without restructuring a lot. 
I also do not understand the intention of not doing the actual instrumentation 
in the ClassFileTransformer but effectively while class loading of the rule 
happens. The easiest way would probably be to write a custom javassist 
ClassPool to cache the byte arrays that arrive in the ClassFileTransformer 
although I don't know how complicated this will be. Any ideas? 

Original comment by mag...@jungsbluth.de on 3 Dec 2011 at 11:52

[GoogleCodeExporter]

Great findings! Of course you're very welcome to "restructure a lot" if needed. 
I threw together the Javaagent stuff mainly as an experiment and I know there 
are bugs and limitations and I'm quite sure there can be many improvements (and 
maybe things in there is plain wrong?). If it really is possible to cache the 
byte arrays in the ClassFileTransformer and use them when making further 
changes to the class that would be awesome! This would enable us (I hope) to 
use PowerMock with other frameworks like Robolectric.

Original comment by johan.ha...@gmail.com on 4 Dec 2011 at 9:30

[GoogleCodeExporter]

ok, I managed to change the java agent  to work with JaCoCo (and hopefully 
other agents as well).

- The Redefiner is reduced to only trigger 
Instrumentation.retransform(classes), this triggers a re-transformation of the 
passed classes. 
- I moved the actual Javassist transformation to the ClassFileTransformer. The 
class pool is then made aware of the actual bytes by ClassPool.makeClass()
- I had to change the editor in the core transformer and explicitly filter 
JaCoCo meethods (one name). This is due to Javassist still loading auxiliary 
classes from disk (JaCoCo communicates via a modified UUID class). I consider 
this filtering ugly at best but it will be hard to control what javassist 
actual tries. This can IMO only be changed by switching completely to ASM 
(which only operates on one class at a time) or caching all classes, which 
would not be reliable if other javaagents run after this javaagent.

After this, only some testng-agent tests fail which are related to final 
classes and fields. In the ClassFileTransformer there is some ASM usage to 
strip final modifiert. Now that all the transformations are in the 
ClassFileTransformer it looks amusing to use two different byte code 
modification libraries in the same method. So to fix the failing tests I would 
need to understand what that asm code really does and why this is not handled 
in the core transformer since this means that the javaagent rule behaves 
different from the classloader one. Can you advise? 

Original comment by mag...@jungsbluth.de on 5 Dec 2011 at 9:26

[GoogleCodeExporter]

Great work!!!

As I think I said before it was a while ago since I looked into the code but if 
I remember it correctly ASM was only used to remove final modifiers from all 
classes (when they're loaded) and nothing else. The reason for using ASM was 
that it's (supposed to be) faster than Javassist and since the removal of the 
final modifier happens on _all classes_ (i.e. not only those specified by 
@PrepareForTest) I went for that approach. Then Javassist was used, if needed 
(i.e. if specified by the PrepareForTest annotation), to modify the rest of the 
byte-code. The final modifier, if I remember it correctly, could only be 
removed when the class was first loaded and not on demand. I guess that was the 
reason for it. But it's of course not written in stone and if there's not a 
great performance penalty for using Javassist all the way it would be the 
better option for sure (as long as the tests will run).  

Original comment by johan.ha...@gmail.com on 5 Dec 2011 at 6:01

[GoogleCodeExporter]

ok, that bit was the last information I needed. 

I split up the ClassfileTransformer in two:
- The Definalizer (ASM) is added with the canRetransform bit set to false
- The regular powermock transformer ist added with the canRetransform bit set 
to true

all tests pass now. I will check with my employer for clearance and will submit 
a patch.  

Original comment by mag...@jungsbluth.de on 5 Dec 2011 at 8:47

[GoogleCodeExporter]

Attached it a patch to fix the abovementioned issue. 

Please take special care when reviewing the core transformer. I was in the end 
able to remove hard-coded method names by just accepting that javassist 
NotFoundExceptions might occur when the java agent is used. 

Original comment by mag...@jungsbluth.de on 6 Dec 2011 at 10:18

Attachments:

• multi-agent.patch

[GoogleCodeExporter]

Thank you so much for your patch! I've applied it to trunk so please try it out 
and see if it works.

Original comment by johan.ha...@gmail.com on 8 Dec 2011 at 7:15

• Changed state: Started

[GoogleCodeExporter]

no worries, was fun to tap into bytecode manipulation once again :). I just 
checked out a clean trunk, ran a mvn clean install and tried JaCoCo with 
Powermock and it works as expected. From my point of view this issue is solved.

To a completely different topic: If I found the time to write a ASM based 
Transformer that mirrors the functionality of the Javassist based one and 
proves this by using exhausive tests, would you be willing to integrate it (as 
an alternative)? ASM is a lot faster and does only work on the bytes that it is 
provided, which means it would solve the root cause of this ticket. I am not 
sure I will find the time but just want to check if it would be worth the 
effort....

Original comment by mag...@jungsbluth.de on 8 Dec 2011 at 9:10

[GoogleCodeExporter]

Are you kidding? That would be totally awesome!! :) A long term goal for a 
veeery long time has been to change everything in PowerMock to ASM but instead 
of Javassist but I have never had the time to properly learn ASM. So what ever 
you want to do to help would be amazing.

Original comment by johan.ha...@gmail.com on 9 Dec 2011 at 6:28

• Changed state: Fixed

[GoogleCodeExporter]

Awesome, just ran into this problem myself, thanks for the fix!
Johan, in what version will this be released and when's the release date?
Thanks.

Original comment by agiann...@kioos.com on 27 Dec 2011 at 2:03