Merge pull request #1309 from hcoles/bug/surefire_unreplaced_properties

replace standard maven properties imported from surefire

pitest-maven-verification/src/test/java/org/pitest/PitMojoIT.java

@@ -321,6 +321,8 @@ public void shouldCorrectlyHandleOverrides() throws Exception {
 
   @Test
   public void shouldReadExclusionsFromSurefireConfig() throws Exception {
+    // Note this test also tests the argline parsing concern
+
     File testDir = prepare("/pit-surefire-excludes");
     verifier.executeGoal("test");
     verifier.executeGoal("org.pitest:pitest-maven:mutationCoverage");

pitest-maven-verification/src/test/resources/pit-surefire-excludes/pom.xml

@@ -15,6 +15,8 @@
             </dependency>
 	</dependencies>
 	<properties>
+		<a.value>isSet</a.value>
+		<b.value>alsoSet</b.value>
 		<argLine/>
 	</properties>
 	<build>
@@ -37,7 +39,7 @@
 			    <exclude>**/FailingTest.java</exclude>
 			    <exclude>**/BadTest.java</exclude>
 			  </excludes>
-				<argLine>@{argLine} -DnotNeeded=avalue</argLine>
+				<argLine>${argLine} -DMUST_BE_SET=${a.value} -DMUST_ALSO_BE_SET=@{b.value}</argLine>
 			</configuration>
 		        </plugin>
 			<plugin>

pitest-maven-verification/src/test/resources/pit-surefire-excludes/src/test/java/com/example/AnotherTest.java

@@ -18,4 +18,10 @@ public void anotherTest() {
     assertEquals(1, Covered.someCode(1));
   }
 
+  @Test
+  public void dependsOnArgLine() {
+    assertEquals("isSet", System.getProperty("MUST_BE_SET"));
+    assertEquals("alsoSet", System.getProperty("MUST_ALSO_BE_SET"));
+  }
+
 }

pitest-maven/src/main/java/org/pitest/maven/AbstractPitMojo.java

@@ -319,6 +319,13 @@ public class AbstractPitMojo extends AbstractMojo {
   @Parameter(defaultValue = "true")
   private boolean                     parseSurefireConfig;
 
+  /**
+   * When set will try and set the argLine based on surefire configuration. This
+   * may not give the desired result in some circumstances
+   */
+  @Parameter(defaultValue = "true")
+  private boolean                     parseSurefireArgLine;
+
   /**
    * honours common skipTests flag in a maven run
    */
@@ -515,7 +522,7 @@ private void throwErrorIfMoreThanMaximumSurvivors(final MutationStatistics resul
 
   protected Optional<CombinedStatistics> analyse() throws MojoExecutionException {
     final ReportOptions data = new MojoToReportOptionsConverter(this,
-        new SurefireConfigConverter(), this.filter).convert();
+        new SurefireConfigConverter(this.isParseSurefireArgLine()), this.filter).convert();
     return Optional.ofNullable(this.goalStrategy.execute(detectBaseDir(), data,
         this.plugins, this.environmentVariables));
   }
@@ -726,6 +733,10 @@ public boolean isParseSurefireConfig() {
     return this.parseSurefireConfig;
   }
 
+  public boolean isParseSurefireArgLine() {
+    return this.parseSurefireArgLine;
+  }
+
   public boolean skipFailingTests() {
     return this.skipFailingTests;
   }

pitest-maven/src/main/java/org/pitest/maven/MojoToReportOptionsConverter.java

@@ -95,7 +95,8 @@ public ReportOptions convert() {
 
     // argline may contain surefire style properties that require expanding
     if (effective.getArgLine() != null) {
-      effective.setArgLine(this.replacePropertyExpressions(option.getArgLine()));
+      log.info("Replacing properties in argLine " + effective.getArgLine());
+      effective.setArgLine(this.replacePropertyExpressions(effective.getArgLine()));
     }
     return effective;
 
@@ -488,14 +489,26 @@ private Properties createPluginProperties() {
    */
   private String replacePropertyExpressions(String argLine) {
     for (Enumeration<?> e = mojo.getProject().getProperties().propertyNames(); e.hasMoreElements();) {
+
       String key = e.nextElement().toString();
-      String field = "@{" + key + "}";
-      if (argLine.contains(field))  {
-        argLine = argLine.replace(field, mojo.getProject().getProperties().getProperty(key, ""));
-      }
+
+      // Replace surefire late evaluation syntax properties
+      argLine = replaceFieldForSymbol('@', key, argLine);
+
+      // Normally properties will already have been expanded by maven, but this is
+      // bypassed for argLines pulled from surefire, se we must handle them here
+      argLine = replaceFieldForSymbol('$', key, argLine);
     }
 
     return argLine;
   }
 
+  private String replaceFieldForSymbol(char symbol, String key, String argLine) {
+    String field = symbol + "{" + key + "}";
+    if (argLine.contains(field))  {
+      return argLine.replace(field, mojo.getProject().getProperties().getProperty(key, ""));
+    }
+    return argLine;
+  }
+
 }

pitest-maven/src/main/java/org/pitest/maven/ScmMojo.java

@@ -135,7 +135,7 @@ protected Optional<CombinedStatistics> analyse() throws MojoExecutionException {
     logClassNames();
     defaultTargetTestsIfNoValueSet();
     final ReportOptions data = new MojoToReportOptionsConverter(this,
-        new SurefireConfigConverter(), getFilter()).convert();
+        new SurefireConfigConverter(this.isParseSurefireArgLine()), getFilter()).convert();
     data.setFailWhenNoMutations(false);
 
     return Optional.ofNullable(this.getGoalStrategy().execute(detectBaseDir(), data,

pitest-maven/src/main/java/org/pitest/maven/SurefireConfigConverter.java

@@ -22,14 +22,22 @@
  */
 public class SurefireConfigConverter {
 
-  public ReportOptions update(ReportOptions option, Xpp3Dom configuration) {
+  private final boolean parseArgLine;
+
+    public SurefireConfigConverter(boolean parseArgLine) {
+        this.parseArgLine = parseArgLine;
+    }
+
+    public ReportOptions update(ReportOptions option, Xpp3Dom configuration) {
     if (configuration == null) {
       return option;
     }
     convertExcludes(option, configuration);
     convertGroups(option, configuration);
     convertTestFailureIgnore(option, configuration);
-    convertArgLine(option, configuration);
+    if (parseArgLine) {
+      convertArgLine(option, configuration);
+    }
     return option;
   }
 
@@ -70,13 +78,10 @@ private void convertExcludes(ReportOptions option, Xpp3Dom configuration) {
   }
 
   private void convertArgLine(ReportOptions option, Xpp3Dom configuration) {
-    if (option.getArgLine() != null) {
-      return;
-    }
-
     Xpp3Dom argLine = configuration.getChild("argLine");
     if (argLine != null) {
-      option.setArgLine(argLine.getValue());
+      String existing = option.getArgLine() != null ? option.getArgLine() + " " : "";
+      option.setArgLine(existing + argLine.getValue());
     }
   }
 

pitest-maven/src/test/java/org/pitest/maven/MojoToReportOptionsConverterTest.java

@@ -441,14 +441,24 @@ public void testParsesArgline() {
     assertThat(actual.getArgLine()).isEqualTo("foo");
   }
 
-  public void testEvaluatesArgLineProperties() {
+  public void testEvaluatesSureFireLateEvalArgLineProperties() {
     properties.setProperty("FOO", "fooValue");
     properties.setProperty("BAR", "barValue");
     properties.setProperty("UNUSED", "unusedValue");
     ReportOptions actual = parseConfig("<argLine>@{FOO} @{BAR}</argLine>");
     assertThat(actual.getArgLine()).isEqualTo("fooValue barValue");
   }
 
+  public void testEvaluatesNormalPropertiesInArgLines() {
+    properties.setProperty("FOO", "fooValue");
+    properties.setProperty("BAR", "barValue");
+    properties.setProperty("UNUSED", "unusedValue");
+    // these are normally auto resolved by maven, but if we pull
+    // in an argline from surefire it will not have been escaped.
+    ReportOptions actual = parseConfig("<argLine>${FOO} ${BAR}</argLine>");
+    assertThat(actual.getArgLine()).isEqualTo("fooValue barValue");
+  }
+
   public void testAutoAddsKotlinSourceDirsWhenPresent() throws IOException {
     // we're stuck in junit 3 land but can
     // use junit 4's temporary folder rule programatically

pitest-maven/src/test/java/org/pitest/maven/SurefireConfigConverterTest.java

@@ -17,7 +17,7 @@
 
 public class SurefireConfigConverterTest {
 
-  SurefireConfigConverter testee  = new SurefireConfigConverter();
+  SurefireConfigConverter testee  = new SurefireConfigConverter(true);
   ReportOptions           options = new ReportOptions();
   Xpp3Dom                 surefireConfig;
 
@@ -166,16 +166,28 @@ public void shouldConvertTestFailureIgnoreWhenAbsent() throws Exception {
 
   @Test
   public void convertsArgline() throws Exception {
-    this.surefireConfig = makeConfig("<argLine>-Xmx1024m</argLine>");
+    this.surefireConfig = makeConfig("<argLine>-Xmx1024m -Dfoo=${BAR} -Dfoo=$@BAR}</argLine>");
 
     ReportOptions actual = this.testee
         .update(this.options, this.surefireConfig);
 
-    assertThat(actual.getArgLine()).isEqualTo("-Xmx1024m");
+    assertThat(actual.getArgLine()).isEqualTo("-Xmx1024m -Dfoo=${BAR} -Dfoo=$@BAR}");
+  }
+
+  @Test
+  public void appendsToExistingArgLine() throws Exception {
+    this.surefireConfig = makeConfig("<argLine>-Xmx1024m -Dfoo=${BAR} -Dfoo=$@BAR}</argLine>");
+    this.options.setArgLine("alreadyHere");
+
+    ReportOptions actual = this.testee
+            .update(this.options, this.surefireConfig);
+
+    assertThat(actual.getArgLine()).isEqualTo("alreadyHere -Xmx1024m -Dfoo=${BAR} -Dfoo=$@BAR}");
   }
 
   @Test
-  public void doesNotConvertArglineWhenAlreadySetInPitestConfig() throws Exception {
+  public void doesNotConvertArglineWhenFlagNotSet() throws Exception {
+    this.testee = new SurefireConfigConverter(false);
     this.surefireConfig = makeConfig("<argLine>-Xmx1024m</argLine>");
 
     this.options.setArgLine("-foo");