Maven Surefire

Improve the design of the class Classpath

Details

  • Type: Improvement Improvement
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Fixed
  • Affects Version/s: 2.7.2
  • Fix Version/s: 2.8
  • Component/s: Maven Surefire Plugin
  • Labels:
    None
  • Complexity:
    Intermediate
  • Testcase included:
    yes
  • Patch Submitted:
    Yes
  • Number of attachments :
    2

Description

This patch contains some improvements for the class Classpath:

  • Only use a set for storing the elements.
  • append(Classpath) has been changed to Classpath.join(Classpath, Classpath), because the classpath doesn't get appended. Instead a new one is created.
  • Renamed setForkProperties to writeToForkProperties.
  • Renamed setAsSystemProperty to writeToSystemProperty.
  • Moved getClassPathAsString() into writeToSystemProperty and only call writeToSystemProperty from outside.
  • Added readFromForkProperties in order to have serialization and deseralization being part of the same class.

From my point of view these changes improve the readability of the Surefire code and are another step on the way to clean code.

  1. Text File
    SUREFIRE-696.patch
    09/Feb/11 2:00 AM
    25 kB
    Stefan Birkner
  2. Text File
    SUREFIRE-Classpath.patch
    06/Feb/11 11:35 AM
    24 kB
    Stefan Birkner

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Kristian Rosenvold added a comment -

LinkedHashSet is a jdk1.4 class and the booter is actually (believe it or not!) a java 1.3 module.

I would also prefer using a private copy-constructor in the join method to retain immutability.

Personally I see no point in using an interator when the code also has to maintain a counter, at least not when you can't use a linkedhashset

The other changes are nice, would you mind updating the patch?

Show
Kristian Rosenvold added a comment - LinkedHashSet is a jdk1.4 class and the booter is actually (believe it or not!) a java 1.3 module. I would also prefer using a private copy-constructor in the join method to retain immutability. Personally I see no point in using an interator when the code also has to maintain a counter, at least not when you can't use a linkedhashset The other changes are nice, would you mind updating the patch?
Hide
Permalink
Stefan Birkner added a comment -

Thanks for your advice.

Unfortunately there are two other problems with Java 1.3. PropertiesWrapper, SurefireStarter and SurefireReflectorTest are using RuntimeException constructors, which have an Exception parameter. Foo.java uses Boolean.valueOf(...). Maybe we should make surefire-booter a Java 1.4 project.

I don't understand in which way a private copy-constructor retains immutability. Classpath objects are not immutable, because there is a method addClassPathElementUrl(...). We could replace this method by a static method
Classpath add( Classpath classpath, String element )
in order to get an immutable class. But for me there's no reason for having an immutable class.

Show
Stefan Birkner added a comment - Thanks for your advice. Unfortunately there are two other problems with Java 1.3. PropertiesWrapper, SurefireStarter and SurefireReflectorTest are using RuntimeException constructors, which have an Exception parameter. Foo.java uses Boolean.valueOf(...). Maybe we should make surefire-booter a Java 1.4 project. I don't understand in which way a private copy-constructor retains immutability. Classpath objects are not immutable, because there is a method addClassPathElementUrl(...). We could replace this method by a static method Classpath add( Classpath classpath, String element ) in order to get an immutable class. But for me there's no reason for having an immutable class.
Hide
Permalink
Kristian Rosenvold added a comment -

Sorry I was wrong about the immutability, my bad. I was just looking at the diff and it must have been wishful thinking on my behalf. I would like to make it immutable since it IMO generally improves construction logic and forces you to create objects in a given order - as required by the problem domain - it usually just drives towards better code. But as you'll see making it immutable involves a fair amount of change.

The current situation wrt jdk 1.3 is that the forked side of the booter and the junit 3.x provider can actually fork down to 1.3. I am no particular fan of this feature, but that'd have to be a ML discussion of sorts. We will not be getting rid of 1.4 yet and I don't personally see the big difference in supporting 1.3.

But the booter module is basically split in 2, the "remote" part, which is 1.3 and the "plugin" side which is 1.4. I have tried to increase this separation by moving the classes that belong only on the "plugin" side out of the booter project and into org.apache.maven.plugin.surefire.booterclient.

So the issues you found in PropertiesWrapper and SurefireStarter are real, since that's 1.3 code. The others questionable, since we somehow need 1.4 to build I will look at your patch tonight and fix the constructors too.

I don't like the 1.3 situation since it's really not sustainable; 1.3 compatibility was broken in from 2.4.2->2.6 and I restored it in 2.7. I have to run in virtualbox to even test this stuff, since there is no 1.3 available for my platform. But I suppose I've just decided to look at it as a challenge since I never have to deal with this in my day job.

Show
Kristian Rosenvold added a comment - Sorry I was wrong about the immutability, my bad. I was just looking at the diff and it must have been wishful thinking on my behalf. I would like to make it immutable since it IMO generally improves construction logic and forces you to create objects in a given order - as required by the problem domain - it usually just drives towards better code. But as you'll see making it immutable involves a fair amount of change. The current situation wrt jdk 1.3 is that the forked side of the booter and the junit 3.x provider can actually fork down to 1.3. I am no particular fan of this feature, but that'd have to be a ML discussion of sorts. We will not be getting rid of 1.4 yet and I don't personally see the big difference in supporting 1.3. But the booter module is basically split in 2, the "remote" part, which is 1.3 and the "plugin" side which is 1.4. I have tried to increase this separation by moving the classes that belong only on the "plugin" side out of the booter project and into org.apache.maven.plugin.surefire.booterclient. So the issues you found in PropertiesWrapper and SurefireStarter are real, since that's 1.3 code. The others questionable, since we somehow need 1.4 to build I will look at your patch tonight and fix the constructors too. I don't like the 1.3 situation since it's really not sustainable; 1.3 compatibility was broken in from 2.4.2->2.6 and I restored it in 2.7. I have to run in virtualbox to even test this stuff, since there is no 1.3 available for my platform. But I suppose I've just decided to look at it as a challenge since I never have to deal with this in my day job.
Hide
Permalink
Kristian Rosenvold added a comment -

Thanks for the patch & followup!

Fixed in r1069058 and r1069062.

Show
Kristian Rosenvold added a comment - Thanks for the patch & followup! Fixed in r1069058 and r1069062.

People

  • Assignee:
    Unassigned
    Reporter:
    Stefan Birkner
Vote (0)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved: