Brett Porter wrote:
Sorry, read these out of order with my other response :)
On 01/06/2007, at 2:29 AM, Kenney Westerhof wrote:
I'll have to see what the issues are, but there are some usecases for
flat classloading
apps, where surefire needs everything in the system classloader. I'm
sure you understand
the need for this feature - i've tried to convince people otherwise
but finally saw that
it was a valid usecase.
Ok, makes sense. It's more consistent with how IDEs run it too.
However, I'm rethinking setting it to default (which I did) - seems best
to retain it as false. I did it because of another regression where
clases in the system classloader weren't found in tests by default any
more. I think that's just an omission in creating the isolated
classloader that can be fixed differently.
Hm, that shouldn't have anything to do with this approach. Surefire and
the test classes are (IIRC) all in the system classloader - at least according
to the manifest. I think there are no extra classloaders created when running
with useSystemClassLoader=true; at least that should be the case.
The regressions may have to do with the issue I described in the other mail,
that the
directories need to end in /; test-classes are typically located in
target/test-classes
so this may be the issue there.
Anyway, I think useSystemClassLoader should be false by default, as it's not really safe.
See below.
On 25/05/2007, at 5:01 PM, Brett Porter wrote:
I'm starting to find a number of bugs related to
useSystemClassLoader, by virtue of it being a forked JAR call. I'd
be interested to get more input on why this route was taken and how
the classloading in general works now (would be a handy document to
have).
See above for the reason of this solution.
Which bugs exactly?
There were a couple I fixed up already (not working on windows because
of the urls in the manifest, seem to be fine now which is the urlutils
thing I used - hope it still works for you). There are 334 (easy to fix)
and 335 (doesn't look critical).
334: indeed, if you run all in the system classloader, surefire and it's
dependencies
are present there too. There's no real easy way around this. I can think of two
solutions:
1) reorder the manifest classpath to put surefire last, perhaps the booter
first. This may
introduce conflicts when running this on surefire itself, or when the tests
include a dep
on anything surefire uses. Since 2 classpaths are merged, there are duplicate
entries. If you
filter them out, which one should you take - the one surefire mentions, so
surefire works, but the
tests are invalid, or the one the project declares, where surefire may no
longer work.
2) Add a single class without dependencies to the test classpath as the Main
(so not surefire-booter),
that reads the surefire*.tmp files and constructs a new classloader (child of
system), containing
surefire and surefirebooter.
Also put no surefire deps in the test classpath. Either create a new project
(with the single class and no deps)
and add that as a dep to the jar classpath, or copy the class into the
test-classes directory (from
the surefire-booter project, using
getResourceAsstream("org/apache/maven/surefire/Main.class") or something.
I think the 2nd option is best - the jar itself should not contain any surefire
classes, except the main one.
wdyt? We can also just say 'dont use this except you're dealing with legacy
apps that don't understand classloading',
which rules out all maven code. The usesystemclasspath
issue 335: must be the '/' bug i fixed (which was not ported to trunk).
Do you have a testcase; can you reproduce this?
I'd definitely be interested in getting together a doc on how the
classloading works though. I can certainly do it by investigation, but
if you have any thoughts you could jot down that'd be great :)
Sure, but it may require some more changes..
The idea is that there are 2 classloaders; the parent containing the tests, the
child containing surefire,
as to avoid polution. The child should have 'childFirst' set to true (so the
surefire loader is consulted first,
then the test classloader.
For testing surefire itself, you'll have the same setup and it should work fine, as the tests themselves
are only present in the parent classloader. When those tests load surefire
classes, they come from the parent,
so the correct classes are tested. The classes in the surefire child
classloader are invisible to the parent
and will not pollute the parent. That's the idea.
There are currently 3 operating modes:
- forkmode none
parent loader: isolated (no parent, not even system) classloader with tests
child loader: parent=parent loader, configured by plugin/booter, containing
surefire.
- forkmode once/pertest, usesystemclassloader=false
have to check the code for this one, but i think it's something like this:
system classloader: contains surefire booter+deps
parent loader: isolated (no parent) loader with test classes
child loader: parent=parent loader, contains surefire provider (junit etc)
- forkmode once/pertest, usesystemcl=true
system classloader: contains tests and surefire booter + deps:
surefire-booter
surefire-api
plexus-utils
commons-lang
plexus-archiver
child loader: parent is system loader, contains surefire provider+deps
That's how it is right now, but the code may be different atm - it's been a
while since I worked on this.
If you have any more questions or need some help, don't hesitate to ask.
Cheers,
Kenney
Also, can you check whether
http://jira.codehaus.org/browse/SUREFIRE-117?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
should be closed as fixed as it was originally, and a new issue
created for their problems, or if it wasn't in fact fixed?
It was fixed; i asked for feedback there. No idea why it doesn't work
for them.
I do however know there were issues with the surefire plugin, but
haven't seen those problems since.
Cool, I'll take another look.
Cheers,
Brett
