Exception copying to non-empty patched-test-runtime directory

[dbwiddis]

Originally reported in #92, but the problem can be reproduced without the complexity I originally saw. Any project with modular patched test runtime will cause the problem.

Running mvn test multiple times without a clean, on a modular project, fails.

Minimal reproducer project: https://github.com/dbwiddis/foo

1. Clone the project
2. mvn clean test test ---> fails
3. mvn clean test clean test ---> succeeds

Additional debugging info:

• The stack trace points to a problem copying the directory structure with the code in these lines

  if (mojo.getProjectModules().getMode() == TestMode.MODULAR_PATCHED_TEST_RUNTIME) {
    String patchedDirectory = targetPath.resolve("patched-test-runtime").toString();
    String mainDirectory = project.getBuild().getOutputDirectory();
  
    try {
      copyDirectoryStructure(mainDirectory, patchedDirectory, false);

• Deleting either the target/junit-runtime/patched-test-runtime directory, or the entire contents of the directory, in between runs of mvn test also fixes the problem, indicating something associated with that directory is failing in these lines:

      Path destPath = Paths.get(dstFldr, currentPath.toString().substring(srcFldr.length()));
          try {
            Files.copy(currentPath, destPath, COPY_ATTRIBUTES, REPLACE_EXISTING);

• The Documentation indicates this line is probably a bug: dstFldr here is patched-test-runtime and it is non-empty after the first run.

  REPLACE_EXISTING – Performs the copy even when the target file already exists. If the target is a symbolic link, the link itself is copied (and not the target of the link). If the target is a non-empty directory, the copy fails with the DirectoryNotEmptyException exception.

[dbwiddis]

Further analysis:

• The DirectoryNotEmptyException is anticipated in the catch block but only if the boolean ignoreNonEmpty is true.
• There are two copyDirectoryStructure() calls, and the ignoreNonEmpty value is set false for the first one (which is the one causing the problem here).
• A possible fix would be to delete the patched-test-runtime directory (if it exists) before the first call, but in that case, it seems the ignoreNonEmpty boolean is useless, so I'm assuming it's there for a reason; perhaps the first call is intended to throw an exception on a nonEmpty directory, exactly as it's doing.

So at present, this seems to be the intention, and successive tests without cleaning up in between are not permitted.

[sormuras]

Thank you for raising this issue including a minimal reproducer, Daniel.

I can't recall why I resorted to copying files back in the days, would be good if there's a "don't copy files" approach.

[hadrabap]

I found quite convenient workaround:

Define in your parent POM:

…
    <build>
        <pluginManagement>
            <plugins>
                <plugin>
                    <groupId>org.apache.maven.plugins</groupId>
                    <artifactId>maven-clean-plugin</artifactId>
                    <version>${maven-clean-plugin.version}</version>
                    <executions>
                        <execution>
                            <id>clean-junit-platform</id>
                            <phase>initialize</phase>
                            <goals>
                                <goal>clean</goal>
                            </goals>
                            <configuration>
                                <excludeDefaultDirectories>true</excludeDefaultDirectories>
                                <fast>true</fast>
                                <filesets>
                                    <fileset>
                                        <directory>${project.build.directory}/junit-platform</directory>
                                    </fileset>
                                </filesets>
                                <skip>false</skip>
                            </configuration>
                        </execution>
                    </executions>
                </plugin>
…

and instantiate it in the artifact's POM which has the junit-platform-maven-plugin in in already. Like this:

    <build>
        <plugins>
            <plugin>
                <groupId>org.apache.maven.plugins</groupId>
                <artifactId>maven-clean-plugin</artifactId>
                <executions>
                    <execution>
                        <id>clean-junit-platform</id>
                    </execution>
                </executions>
            </plugin>
            <plugin>
                <groupId>de.sormuras.junit</groupId>
                <artifactId>junit-platform-maven-plugin</artifactId>
            </plugin>
…

Goo luck!

[dbwiddis]

Yep. Already worked around it. :-)

[sormuras]

Fall is coming - might be a good time to tackle this bug within the plugin.

[dbwiddis]

I'd be happy to submit a fix, except it's really not clear what the fix is. I'd just always set ignoreNonEmpty to true and it would work.

But then why is that boolean there in the first place? :-)

[dbwiddis]

So my workaround works great for running tests, but I'm blocked on publishing a release due to a different bug in maven-javadoc-plugin that can't find my module descriptor in the case the output directory exists but my previously built jar has been "cleaned". :(

[dbwiddis]

Hey @sormuras as I hack around this for yet another release, I'm wondering if there's anything I can do to help get this fixed. I certainly know a way to fix it but I'm not sure if that will break some other intended behavior. To summarize:

• this bug requires running clean for modular builds all the time, or tests break
• I've hacked my POM to force clean in the initialize phase so users don't have to, which is less efficient but workable
• but that breaks release with javadoc plugin so I have to undo the hack every time I want to cut a release, and then put it back in

[sormuras]

Hi Daniel!

I certainly know a way to fix it but I'm not sure if that will break some other intended behavior.

If you have time to prepare a fix I am happy to merge it. As long as existing integration tests don't break, we're fine. If a few fail, there's always the option to name the next release 1.2.0 or even 2.0.0

[dbwiddis]

Sure, I'll take a crack at it this weekend. My plan is to toggle the ignoreNonEmpty boolean to true in this line: https://github.com/sormuras/junit-platform-maven-plugin/blob/be28792abb6ba9d6489713c407a58389c96da2d4/src/main/java/de/sormuras/junit/platform/maven/plugin/MavenDriver.java#L138

That'll take 5 seconds. Writing a failing test, proving this fixes it, and debugging any other failures that causes may require extra cups of coffee. :)

[dbwiddis]

Initial attempts toggling the boolean worked, but ended up keeping the old files there because the copy failed (as before) but the exception was caught. And old compiled files might not be current, which is bad.

So I just replicated the subset of clean that really is needed, deleting the target directory before copying.