Junit5: Test with @TempDir fails with NoSuchFileException if the tempDir is deleted inside the test

[gastaldi]

JUnit 5.4.0

Steps to reproduce

    @Test
    void deleteDirectory(@TempDir Path tempDirectory) throws IOException {
        Files.delete(tempDirectory);
        assertThat(tempDirectory).doesNotExist();
    }

Context

• Used versions (Jupiter/Vintage/Platform): 5.4.0
• Build Tool/IDE: IntelliJ / Maven 3.6.0

[sbrannen]

Good catch!

We'll look improving this.

[sbrannen]

In case I don't get around to pushing the fix to master and documenting it in the release notes, here's the fix for org.junit.jupiter.engine.extension.TempDirectory.CloseablePath.deleteAllFilesAndDirectories():

        private SortedMap<Path, IOException> deleteAllFilesAndDirectories() throws IOException {
            SortedMap<Path, IOException> failures = new TreeMap<>();
            if (Files.notExists(dir)) {
                return failures;
            }
            // rest of method

[gastaldi]

@sbrannen I think a better fix would be ?

        private SortedMap<Path, IOException> deleteAllFilesAndDirectories() throws IOException {
            SortedMap<Path, IOException> failures = new TreeMap<>();
            Files.walkFileTree(dir, new SimpleFileVisitor<Path>() {

                @Override
                public FileVisitResult visitFile(Path file, BasicFileAttributes attributes) {
                    return deleteAndContinue(file);
                }

                @Override
                public FileVisitResult postVisitDirectory(Path dir, IOException exc) {
                    return deleteAndContinue(dir);
                }

                private FileVisitResult deleteAndContinue(Path path) {
                    try {
                        if (Files.exists(path)) {
                            Files.delete(path);
                        }
                    }
                    catch (IOException ex) {
                        failures.put(path, ex);
                    }
                    return CONTINUE;
                }
            });
            return failures;
        }

[gastaldi]

Ah no,you're right, ignore my last comment

[gastaldi]

But it wouldn't hurt to check if the file exists before deleting anyway :wink:

[gastaldi]

For the memory-concerned ones:

private SortedMap<Path, IOException> deleteAllFilesAndDirectories() throws IOException {
    if (Files.notExists(dir)) {
            return Collections.emptySortedMap();
    }
        SortedMap<Path, IOException> failures = new TreeMap<>();
        ...
}

[gastaldi]

Any chance to have this fix in a 5.4.x release?

[sormuras]

What about this implementation?

    static void deleteAllFilesAndDirectories(Path root) throws Exception {
      // trivial case: delete existing empty directory or single file
      try {
        Files.deleteIfExists(root);
        return;
      } catch (DirectoryNotEmptyException ignored) {
        // fall-through
      }

      // default case: walk the tree...
      try (var stream = Files.walk(root)) {
        var selected = stream.sorted((p, q) -> -p.compareTo(q));
        for (var path : selected.collect(Collectors.toList())) {
          Files.deleteIfExists(path);
        }
      }
    }

[gastaldi]

@sormuras no objections, but I'd rather avoid throwing exceptions when possible.

[sormuras]

Sure thing. They would be handled by TempDirectory.CloseablePath.deleteAllFilesAndDirectories().

[sbrannen]

Any chance to have this fix in a 5.4.x release?

Yes, but "it depends". 😉

I don't consider this a show-stopper personally. If it were, that would warrant a quick 5.4.1 release containing the fix.

What we typically do for bugs of this magnitude is wait for 2-3 bugs to pop up before we put out a point release.

[sbrannen]

@junit-team/junit-lambda, anyone consider this a show-stopper?

[sbrannen]

Any chance to have this fix in a 5.4.x release?

We have decided to publish a 5.4.1 release, and this fix will be included in that.