Re: svn commit: r438999 - /maven/surefire/trunk/surefire-booter/src/main/java/org/apache/maven/surefire/booter/SurefireBooter.java

[Kenney Westerhof <ke...@apache.org>]

Brett Porter wrote:
> Could this break any existing tests?

Yes, if those tests were wrong. Effectively the difference is that
you no longer have $M2_HOME/core/*.jar in the classpath.

Two scenarios:
- the tests dependended on those libs but didn't declare them as test dependencies.
  This can only be a problem if they use Class.forName(), but then they should've declared
  them as dependencies.
- tests define a different version of those libs as dependencies, for instance plexus-utils-<something else than 1.1>.
  Now the tests will actually use the declared dependency instead of the one that maven uses.
  If the tests break: great! then the code was bad.

This commit will probably fix a bunch of JIRA issues for surefire or maven-surefire-plugin.

I found this problem because my unit tests in continuum gave a 'NoSuchMethodError: setPid(..) in Commandline'.
(available in plexus-utils-1.2).
When i asked where Commandline came from it was from plexus-utils-1.1 from the maven directory. The tests
ran fine in eclipse, which uses ONLY the dependencies declared in the POM.

So this seems the correct behaviour to me.

-- Kenney

> 
> - Brett
> 
> On 01/09/2006, at 4:21 AM, kenney@apache.org wrote:
> 
>> Author: kenney
>> Date: Thu Aug 31 11:21:53 2006
>> New Revision: 438999
>>
>> URL: http://svn.apache.org/viewvc?rev=438999&view=rev
>> Log:
>> Set parent classloader to null for the testsClassLoader.
>>
>> If this is not done, the System classloader is added, in this case
>> an AppClassloader containing everything in the root classpath.
>> For instance, in maven, everything in core/ is available.
>> This can cause clashes with the plexus-utils used in maven itself.
>>
>> Modified:
>>     
>> maven/surefire/trunk/surefire-booter/src/main/java/org/apache/maven/surefire/booter/SurefireBooter.java 
>>
>>
>> Modified: 
>> maven/surefire/trunk/surefire-booter/src/main/java/org/apache/maven/surefire/booter/SurefireBooter.java 
>>
>> URL: 
>> http://svn.apache.org/viewvc/maven/surefire/trunk/surefire-booter/src/main/java/org/apache/maven/surefire/booter/SurefireBooter.java?rev=438999&r1=438998&r2=438999&view=diff 
>>
>> ============================================================================== 
>>
>> --- 
>> maven/surefire/trunk/surefire-booter/src/main/java/org/apache/maven/surefire/booter/SurefireBooter.java 
>> (original)
>> +++ 
>> maven/surefire/trunk/surefire-booter/src/main/java/org/apache/maven/surefire/booter/SurefireBooter.java 
>> Thu Aug 31 11:21:53 2006
>> @@ -203,7 +203,7 @@
>>              ClassLoader surefireClassLoader =
>>                  createClassLoader( surefireClassPathUrls, 
>> getClass().getClassLoader(), true );
>>
>> -            ClassLoader testsClassLoader = createClassLoader( 
>> classPathUrls, childDelegation, true );
>> +            ClassLoader testsClassLoader = createClassLoader( 
>> classPathUrls, null, childDelegation, true );
>>
>>              Class surefireClass = surefireClassLoader.loadClass( 
>> Surefire.class.getName() );
>>
>> @@ -243,7 +243,7 @@
>>          {
>>              // The test classloader must be constructed first to 
>> avoid issues with commons-logging until we properly
>>              // separate the TestNG classloader
>> -            ClassLoader testsClassLoader = createClassLoader( 
>> classPathUrls, childDelegation, true );
>> +            ClassLoader testsClassLoader = createClassLoader( 
>> classPathUrls, null, childDelegation, true );
>>
>>              ClassLoader surefireClassLoader =
>>                  createClassLoader( surefireClassPathUrls, 
>> getClass().getClassLoader(), true );
>> @@ -289,7 +289,7 @@
>>          ClassLoader surefireClassLoader;
>>          try
>>          {
>> -            testsClassLoader = createClassLoader( classPathUrls, 
>> false, true );
>> +            testsClassLoader = createClassLoader( classPathUrls, 
>> null, false, true );
>>              // TODO: assertions = true shouldn't be required if we 
>> had proper separation (see TestNG)
>>              surefireClassLoader = createClassLoader( 
>> surefireClassPathUrls, false, true );
>>          }
>> @@ -586,7 +586,10 @@
>>              try
>>              {
>>                  Object[] args = new Object[]{assertionsEnabled ? 
>> Boolean.TRUE : Boolean.FALSE};
>> -                assertionStatusMethod.invoke( parent, args );
>> +                if ( parent != null )
>> +                {
>> +                    assertionStatusMethod.invoke( parent, args );
>> +                }
>>                  assertionStatusMethod.invoke( classLoader, args );
>>              }
>>              catch ( IllegalAccessException e )
>>