Capture all classloaders loading mutant for roboelectric and quarkus

README.md

@@ -8,6 +8,12 @@ Read all about it at http://pitest.org
 
 ## Releases
 
+## 1.10.0 (unreleased)
+
+* #1067 Improved Quarkus and Roboelectric support
+
+As a result of #1067 it is important that mutations are only created for a single class for each JVM. The `MutationGrouper` extension point has therefore been removed as this allowed this constraint to be violated. Any third party plugins using this extension are no longer supported.  
+
 ### 1.9.11
 
 * #1105 Aggregator resolves wrong file for out of package kotlin files with same name

pitest-entry/src/main/java/org/pitest/mutationtest/build/DefaultGrouper.java

@@ -24,8 +24,8 @@ public List<List<MutationDetails>> groupMutations(
     final Map<ClassName, Collection<MutationDetails>> bucketed = FCollection
         .bucket(mutations, MutationDetails::getClassName);
     final List<List<MutationDetails>> chunked = new ArrayList<>();
-    for (final Collection<MutationDetails> each : bucketed.values()) {
-      shrinkToMaximumUnitSize(chunked, each);
+    for (final Map.Entry<ClassName,Collection<MutationDetails>> each : bucketed.entrySet()) {
+      shrinkToMaximumUnitSize(chunked, each.getValue());
     }
 
     return chunked;

pitest-entry/src/main/java/org/pitest/mutationtest/config/SettingsFactory.java

@@ -87,9 +87,11 @@ public JavaExecutableLocator getJavaExecutable() {
   }
 
   public MutationGrouperFactory getMutationGrouper() {
-    final Collection<? extends MutationGrouperFactory> groupers = this.plugins
-        .findGroupers();
-    return firstOrDefault(groupers, new DefaultMutationGrouperFactory());
+    // Grouping behaviour is important. We cannot have more than 1 class mutated within
+    // a JVM or else the last mutation will poison the next. This restriction can only
+    // be removed if the hotswap functionality is reworked.
+    // Grouping behaviour is therefore hard coded for now.
+    return new DefaultMutationGrouperFactory();
   }
 
   public void describeFeatures(Consumer<Feature> enabled, Consumer<Feature> disabled) {

pitest-entry/src/test/java/com/example/classloaders/MuteeInOtherClassloader.java

@@ -0,0 +1,17 @@
+package com.example.classloaders;
+
+import java.util.function.IntSupplier;
+
+public class MuteeInOtherClassloader implements IntSupplier {
+
+    @Override
+    public int getAsInt() {
+        pointless();
+        System.out.println("Can't kill me");
+        return 42;
+    }
+
+    public void pointless() {
+        System.out.println("Can't kill me either");
+    }
+}

pitest-entry/src/test/java/com/example/classloaders/MuteeInOtherClassloaderPooledTest.java

@@ -0,0 +1,22 @@
+package com.example.classloaders;
+
+import org.junit.Test;
+import org.pitest.classpath.ClassPath;
+import org.pitest.mutationtest.execute.DefaultPITClassloader;
+
+import java.util.function.IntSupplier;
+
+import static org.junit.Assert.assertEquals;
+
+public class MuteeInOtherClassloaderPooledTest {
+
+    // will persist between mutants
+    static DefaultPITClassloader otherLoader = new DefaultPITClassloader(new ClassPath(), null);
+
+    @Test
+    public void returns42() throws Exception {
+        Class<?> clz = otherLoader.loadClass(MuteeInOtherClassloader.class.getName());
+        IntSupplier underTest = (IntSupplier) clz.getConstructor().newInstance();
+        assertEquals(42, underTest.getAsInt());
+    }
+}

pitest-entry/src/test/java/com/example/classloaders/MuteeInOtherClassloaderTest.java

@@ -0,0 +1,20 @@
+package com.example.classloaders;
+
+import org.junit.Test;
+import org.pitest.classpath.ClassPath;
+import org.pitest.mutationtest.execute.DefaultPITClassloader;
+
+import java.util.function.IntSupplier;
+
+import static org.junit.Assert.assertEquals;
+
+public class MuteeInOtherClassloaderTest {
+
+    @Test
+    public void returns42() throws Exception {
+        DefaultPITClassloader otherLoader = new DefaultPITClassloader(new ClassPath(), null);
+        Class<?> clz = otherLoader.loadClass(MuteeInOtherClassloader.class.getName());
+        IntSupplier underTest = (IntSupplier) clz.getConstructor().newInstance();
+        assertEquals(42, underTest.getAsInt());
+    }
+}

pitest-entry/src/test/java/org/pitest/mutationtest/MutationCoverageReportSystemTest.java

@@ -33,6 +33,9 @@
 import java.util.Collections;
 import java.util.List;
 
+import com.example.classloaders.MuteeInOtherClassloader;
+import com.example.classloaders.MuteeInOtherClassloaderPooledTest;
+import com.example.classloaders.MuteeInOtherClassloaderTest;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.experimental.categories.Category;
@@ -427,6 +430,34 @@ public void shouldRecoverFromMinionThatDoesNotStart() {
     // just running without a hang then erroring is success
   }
 
+  @Test
+  public void handlesMutantsInOtherClassLoaders() {
+    setMutators("PRIMITIVE_RETURNS", "VOID_METHOD_CALLS");
+
+    this.data
+            .setTargetClasses(asGlobs(MuteeInOtherClassloader.class));
+    this.data
+            .setTargetTests(predicateFor(MuteeInOtherClassloaderTest.class));
+
+    createAndRun();
+
+    verifyResults(KILLED, SURVIVED, SURVIVED, SURVIVED);
+  }
+
+  @Test
+  public void handlesMutantsInPooledClassLoaders() {
+    setMutators("PRIMITIVE_RETURNS", "VOID_METHOD_CALLS");
+
+    this.data
+            .setTargetClasses(asGlobs(MuteeInOtherClassloader.class));
+    this.data
+            .setTargetTests(predicateFor(MuteeInOtherClassloaderPooledTest.class));
+
+    createAndRun();
+
+    verifyResults(KILLED, SURVIVED, SURVIVED, SURVIVED);
+  }
+
   @Generated
   public static class AnnotatedToAvoidAtClassLevel {
     public int mutateMe() {

pitest-entry/src/test/java/org/pitest/mutationtest/ReportTestBase.java

@@ -142,7 +142,7 @@ private CoverageOptions createCoverageOptions(TestPluginArguments configuration)
         configuration, this.data.getVerbosity());
   }
 
-  protected void setMutators(final String mutator) {
+  protected void setMutators(final String... mutator) {
     this.data.setMutators(Arrays.asList(mutator));
   }
 

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

@@ -403,7 +403,7 @@ public void shouldWorkWithQuarkus() throws Exception {
     //assertThat(actual)
     //        .contains(
     //                "<mutation detected='false' status='SURVIVED' numberOfTestsRun='2'>" +
-     //                       "<sourceFile>ExampleController.java</sourceFile>");
+    //                        "<sourceFile>ExampleController.java</sourceFile>");
 
     assertThat(actual)
             .contains(

pitest/pom.xml

@@ -206,6 +206,8 @@
 			<artifactId>equalsverifier</artifactId>
 			<scope>test</scope>
 		</dependency>
+
+
 	</dependencies>
 
 </project>

pitest/src/main/java/org/pitest/mutationtest/execute/CatchNewClassLoadersTransformer.java

@@ -5,9 +5,9 @@
 
 import java.lang.instrument.ClassFileTransformer;
 import java.security.ProtectionDomain;
-import java.util.Arrays;
+import java.util.Collections;
 import java.util.Map;
-import java.util.concurrent.ConcurrentHashMap;
+import java.util.WeakHashMap;
 
 /**
  * Pitest mainly inserts mutants by calling Instrumentation.redefineClasses using
@@ -22,83 +22,41 @@
  * the byte array (modifying by rerunning the mutator might result in a different mutant for
  * the same id if the input has changed).
  *
- * So, sometimes we want to transform the class, sometimes we don't.
- *
- * Not found a way to reliably work out which we need to do, so need to maintain
- * a list. As mutants unexpectedly surviving are far more likely to be reported than
- * ones unexpectedly killed, we list the loaders we should mutate for (currently just
- * quarkus, but others likely exist).
+ * This transformer identifies loaders that try to load the target class, and stores
+ * a weak reference to them to ensure that all copies of the class are mutated.
  *
  */
 public class CatchNewClassLoadersTransformer implements ClassFileTransformer {
 
+    private static String targetClass;
     private static byte[] currentMutant;
-    private static String currentClass;
-
-    // The context classloader at the point pitest started.
-    // we do not want to transform classes from this as they are already handled
-    // by the primary mechanism
-    private static ClassLoader ignore;
-
-    // Map of loaders we have transformed the current class in, so we can restore them
-    static final Map<ClassLoader, byte[]> ORIGINAL_LOADER_CLASSES = new ConcurrentHashMap<>();
 
-    public static synchronized void setLoader(ClassLoader loader) {
-        ignore = loader;
-    }
+    // What we want is a ConcurrentWeakHasSet, since that doesn't exist without writing one ourselves
+    // we'll abuse a WeakHashMap and live with the synchronization
+    static final Map<ClassLoader, Object> CLASS_LOADERS = Collections.synchronizedMap(new WeakHashMap<>());
 
     public static synchronized void setMutant(String className, byte[] mutant) {
-        String toRestore = currentClass;
+        targetClass = className;
         currentMutant = mutant;
-
-        // prevent transforming again when we restore if the same class is mutated twice
-        currentClass = null;
-
-        restoreClasses(toRestore);
-
-        currentClass = className;
-    }
-
-    private static void restoreClasses(String toRestore) {
-        for (Map.Entry<ClassLoader, byte[]> each : ORIGINAL_LOADER_CLASSES.entrySet()) {
-            final Class<?> clazz = checkClassForLoader(each.getKey(), toRestore);
+        for (ClassLoader each : CLASS_LOADERS.keySet()) {
+            final Class<?> clazz = checkClassForLoader(each, className);
             if (clazz != null) {
-                HotSwapAgent.hotSwap(clazz, each.getValue());
+                HotSwapAgent.hotSwap(clazz, mutant);
             }
         }
-        ORIGINAL_LOADER_CLASSES.clear();
     }
 
     @Override
     public byte[] transform(final ClassLoader loader, final String className,
                             final Class<?> classBeingRedefined,
                             final ProtectionDomain protectionDomain, final byte[] classfileBuffer) {
-        if (loader == null || ignore == loader) {
-            return null;
-        }
-
-        // Only loader identified so far where mutants must be inserted is Quarkus, but
-        // others likely exist. At least one loader (gwtmockito) results incorrectly
-        // killed mutants if we insert.
-        if (!loader.getClass().getName().startsWith("io.quarkus.bootstrap.classloading")) {
-            return null;
-        }
-
-        if (className.equals(currentClass)) {
-            // skip if class already loaded
-            if (classBeingRedefined != null) {
-                return null;
-            }
-
-            // avoid restoring an already mutated class
-            // Not clear if this situation is possible, but check left in
-            // out of fear.
-            if (!Arrays.equals(classfileBuffer, currentMutant)) {
-                ORIGINAL_LOADER_CLASSES.put(loader, classfileBuffer);
-            }
 
+        if (className.equals(targetClass) && shouldTransform(loader)) {
+            CLASS_LOADERS.put(loader, null);
+            // we might be mid-mutation so return the mutated bytes
             return currentMutant;
         }
+
         return null;
     }
 
@@ -119,4 +77,10 @@ private static Class<?> checkClassForLoader(ClassLoader loader, String className
 
     }
 
+    private boolean shouldTransform(ClassLoader loader) {
+        // Only gwtmockito has been identified so far as a loader not to transform
+        // but there will be others
+        return !loader.getClass().getName().startsWith("com.google.gwtmockito.");
+    }
+
 }

pitest/src/main/java/org/pitest/mutationtest/execute/HotSwap.java

@@ -1,62 +1,32 @@
 package org.pitest.mutationtest.execute;
 
 import org.pitest.boot.HotSwapAgent;
-import org.pitest.classinfo.ClassByteArraySource;
 import org.pitest.classinfo.ClassName;
-import org.pitest.functional.F3;
 import org.pitest.util.Unchecked;
 
-class HotSwap implements F3<ClassName, ClassLoader, byte[], Boolean> {
+/**
+ * Since pitest 1.9.4 there is an implicit assumption that pitest will never mutate
+ * more than once class within the same jvm. If this assumption is ever broken, mutants
+ * from the previous class would remain active and invalidate results.
+ */
+class HotSwap {
 
-  private final ClassByteArraySource byteSource;
-  private byte[]                     lastClassPreMutation;
-  private ClassName                  lastMutatedClass;
-  private ClassLoader                lastUsedLoader;
-
-  HotSwap(final ClassByteArraySource byteSource) {
-    this.byteSource = byteSource;
-  }
-
-  @Override
-  public Boolean apply(final ClassName clazzName, final ClassLoader loader,
-      final byte[] b) {
+  public Boolean insertClass(final ClassName clazzName, ClassLoader loader, final byte[] mutantBytes) {
     try {
+      // Some frameworks (eg quarkus) run tests in non delegating
+      // classloaders. Need to make sure these are transformed too
+      CatchNewClassLoadersTransformer.setMutant(clazzName.asInternalName(), mutantBytes);
 
-      System.out.println("Hotswap loader " + loader);
-
-      restoreLastClass(this.byteSource, clazzName, loader);
-      this.lastUsedLoader = loader;
+      // trigger loading for the current loader
       Class<?> clazz = Class.forName(clazzName.asJavaName(), false, loader);
-      return HotSwapAgent.hotSwap(clazz, b);
-    } catch (final ClassNotFoundException e) {
-      throw Unchecked.translateCheckedException(e);
-    }
 
-  }
-
-  private void restoreLastClass(final ClassByteArraySource byteSource,
-      final ClassName clazzName, final ClassLoader loader)
-          throws ClassNotFoundException {
-    if ((this.lastMutatedClass != null)
-        && !this.lastMutatedClass.equals(clazzName)) {
-      restoreForLoader(this.lastUsedLoader);
-      restoreForLoader(loader);
-    }
+      // will still need to explicitly swap it... not clear why the transformed does not do this
+      return HotSwapAgent.hotSwap(clazz, mutantBytes);
 
-    if ((this.lastMutatedClass == null)
-        || !this.lastMutatedClass.equals(clazzName)) {
-      this.lastClassPreMutation = byteSource.getBytes(clazzName.asJavaName())
-          .get();
+    } catch (final ClassNotFoundException e) {
+      throw Unchecked.translateCheckedException(e);
     }
 
-    this.lastMutatedClass = clazzName;
-  }
-
-  private void restoreForLoader(ClassLoader loader)
-      throws ClassNotFoundException {
-    final Class<?> clazz = Class.forName(this.lastMutatedClass.asJavaName(), false,
-        loader);
-    HotSwapAgent.hotSwap(clazz, this.lastClassPreMutation);
   }
 
 }