Closed GoogleCodeExporter closed 9 years ago
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
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
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
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
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
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
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
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:
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
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
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
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
Original issue reported on code.google.com by
mag...@jungsbluth.de
on 22 Nov 2011 at 9:02