From 0f1885c9ae55a3e3780c914e2f62a596692ecd27 Mon Sep 17 00:00:00 2001 From: Peter De Maeyer Date: Thu, 7 Nov 2024 08:39:37 +0100 Subject: [PATCH 1/6] MCLEAN-124 Leverage Files.delete(Path) API to provide more accurate reason in case of failure --- .../apache/maven/plugins/clean/Cleaner.java | 18 +- .../maven/plugins/clean/CleanerTest.java | 183 ++++++++++++++++++ 2 files changed, 197 insertions(+), 4 deletions(-) create mode 100644 src/test/java/org/apache/maven/plugins/clean/CleanerTest.java diff --git a/src/main/java/org/apache/maven/plugins/clean/Cleaner.java b/src/main/java/org/apache/maven/plugins/clean/Cleaner.java index f5c25d2..b43ebdc 100644 --- a/src/main/java/org/apache/maven/plugins/clean/Cleaner.java +++ b/src/main/java/org/apache/maven/plugins/clean/Cleaner.java @@ -273,7 +273,8 @@ private boolean isSymbolicLink(Path path) throws IOException { * @throws IOException If a file/directory could not be deleted and failOnError is true. */ private int delete(File file, boolean failOnError, boolean retryOnError) throws IOException { - if (!file.delete()) { + IOException error = delete(file); + if (error != null) { boolean deleted = false; if (retryOnError) { @@ -289,7 +290,7 @@ private int delete(File file, boolean failOnError, boolean retryOnError) throws } catch (InterruptedException e) { // ignore } - deleted = file.delete() || !file.exists(); + deleted = delete(file) == null || !file.exists(); } } else { deleted = !file.exists(); @@ -297,10 +298,10 @@ private int delete(File file, boolean failOnError, boolean retryOnError) throws if (!deleted) { if (failOnError) { - throw new IOException("Failed to delete " + file); + throw new IOException("Failed to delete " + file, error); } else { if (log.isWarnEnabled()) { - log.warn("Failed to delete " + file); + log.warn("Failed to delete " + file, error); } return 1; } @@ -310,6 +311,15 @@ private int delete(File file, boolean failOnError, boolean retryOnError) throws return 0; } + private static IOException delete(File file) { + try { + Files.delete(file.toPath()); + } catch (IOException e) { + return e; + } + return null; + } + private static class Result { private int failures; diff --git a/src/test/java/org/apache/maven/plugins/clean/CleanerTest.java b/src/test/java/org/apache/maven/plugins/clean/CleanerTest.java new file mode 100644 index 0000000..10aa7e1 --- /dev/null +++ b/src/test/java/org/apache/maven/plugins/clean/CleanerTest.java @@ -0,0 +1,183 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.maven.plugins.clean; + +import java.io.IOException; +import java.nio.file.AccessDeniedException; +import java.nio.file.DirectoryNotEmptyException; +import java.nio.file.Path; +import java.nio.file.attribute.PosixFilePermission; +import java.nio.file.attribute.PosixFilePermissions; +import java.util.Iterator; +import java.util.LinkedHashMap; +import java.util.Map; +import java.util.Map.Entry; +import java.util.Set; + +import org.apache.maven.plugin.logging.Log; +import org.apache.maven.plugin.testing.SilentLog; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import static java.nio.file.Files.createDirectory; +import static java.nio.file.Files.createFile; +import static java.nio.file.Files.exists; +import static java.nio.file.Files.getPosixFilePermissions; +import static java.nio.file.Files.setPosixFilePermissions; +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +class CleanerTest { + + private boolean warnEnabled; + + /** + * Use a {@code LinkedHashMap} to preserve the order of the logged warnings. + */ + private final Map warnings = new LinkedHashMap<>(); + + /** + * Ideally we should use a mocking framework such as Mockito for this, but alas, this project has no such dependency. + */ + private final Log log = new SilentLog() { + + @Override + public boolean isWarnEnabled() { + return warnEnabled; + } + + @Override + public void warn(CharSequence content, Throwable error) { + warnings.put(content, error); + } + }; + + @Test + void deleteSucceedsDeeply(@TempDir Path tempDir) throws Exception { + final Path basedir = createDirectory(tempDir.resolve("target")); + final Path file = createFile(basedir.resolve("file")); + final Cleaner cleaner = new Cleaner(null, log, false, null, null); + cleaner.delete(basedir.toFile(), null, false, true, false); + assertFalse(exists(basedir)); + assertFalse(exists(file)); + } + + @Test + void deleteFailsWithoutRetryWhenNoPermission(@TempDir Path tempDir) throws Exception { + warnEnabled = true; + final Path basedir = createDirectory(tempDir.resolve("target")); + createFile(basedir.resolve("file")); + final Set initialPermissions = getPosixFilePermissions(basedir); + final String rwxrwxr_x = PosixFilePermissions.toString(initialPermissions); + // Prevent directory listing, which will result in a DirectoryNotEmptyException. + final String rw_rw_r__ = rwxrwxr_x.replace('x', '-'); + final Set permissions = PosixFilePermissions.fromString(rw_rw_r__); + setPosixFilePermissions(basedir, permissions); + try { + final Cleaner cleaner = new Cleaner(null, log, false, null, null); + final IOException exception = + assertThrows(IOException.class, () -> cleaner.delete(basedir.toFile(), null, false, true, false)); + assertTrue(warnings.isEmpty()); + assertEquals("Failed to delete " + basedir, exception.getMessage()); + final DirectoryNotEmptyException cause = + assertInstanceOf(DirectoryNotEmptyException.class, exception.getCause()); + assertEquals(basedir.toString(), cause.getMessage()); + } finally { + // Allow the tempDir to be cleared by the @TempDir extension. + setPosixFilePermissions(basedir, initialPermissions); + } + } + + @Test + void deleteFailsAfterRetryWhenNoPermission(@TempDir Path tempDir) throws Exception { + final Path basedir = createDirectory(tempDir.resolve("target")); + createFile(basedir.resolve("file")); + final Set initialPermissions = getPosixFilePermissions(basedir); + final String rwxrwxr_x = PosixFilePermissions.toString(initialPermissions); + // Prevent directory listing, which will result in a DirectoryNotEmptyException. + final String rw_rw_r__ = rwxrwxr_x.replace('x', '-'); + final Set permissions = PosixFilePermissions.fromString(rw_rw_r__); + setPosixFilePermissions(basedir, permissions); + try { + final Cleaner cleaner = new Cleaner(null, log, false, null, null); + final IOException exception = + assertThrows(IOException.class, () -> cleaner.delete(basedir.toFile(), null, false, true, true)); + assertEquals("Failed to delete " + basedir, exception.getMessage()); + final DirectoryNotEmptyException cause = + assertInstanceOf(DirectoryNotEmptyException.class, exception.getCause()); + assertEquals(basedir.toString(), cause.getMessage()); + } finally { + // Allow the tempDir to be cleared by the @TempDir extension. + setPosixFilePermissions(basedir, initialPermissions); + } + } + + @Test + void deleteLogsWarningWithoutRetryWhenNoPermission(@TempDir Path tempDir) throws Exception { + warnEnabled = true; + final Path basedir = createDirectory(tempDir.resolve("target")); + final Path file = createFile(basedir.resolve("file")); + final Set initialPermissions = getPosixFilePermissions(basedir); + final String rwxrwxr_x = PosixFilePermissions.toString(initialPermissions); + final String r_xr_xr_x = rwxrwxr_x.replace('w', '-'); + final Set permissions = PosixFilePermissions.fromString(r_xr_xr_x); + setPosixFilePermissions(basedir, permissions); + try { + final Cleaner cleaner = new Cleaner(null, log, false, null, null); + assertDoesNotThrow(() -> cleaner.delete(basedir.toFile(), null, false, false, false)); + assertEquals(2, warnings.size()); + final Iterator> it = + warnings.entrySet().iterator(); + final Entry warning1 = it.next(); + assertEquals("Failed to delete " + file, warning1.getKey()); + final AccessDeniedException cause1 = assertInstanceOf(AccessDeniedException.class, warning1.getValue()); + assertEquals(file.toString(), cause1.getMessage()); + final Entry warning2 = it.next(); + assertEquals("Failed to delete " + basedir, warning2.getKey()); + final DirectoryNotEmptyException cause2 = + assertInstanceOf(DirectoryNotEmptyException.class, warning2.getValue()); + assertEquals(basedir.toString(), cause2.getMessage()); + } finally { + setPosixFilePermissions(basedir, initialPermissions); + } + } + + @Test + void deleteDoesNotLogAnythingWhenNoPermissionAndWarnDisabled(@TempDir Path tempDir) throws Exception { + warnEnabled = false; + final Path basedir = createDirectory(tempDir.resolve("target")); + createFile(basedir.resolve("file")); + final Set initialPermissions = getPosixFilePermissions(basedir); + final String rwxrwxr_x = PosixFilePermissions.toString(initialPermissions); + final String r_xr_xr_x = rwxrwxr_x.replace('w', '-'); + final Set permissions = PosixFilePermissions.fromString(r_xr_xr_x); + setPosixFilePermissions(basedir, permissions); + try { + final Cleaner cleaner = new Cleaner(null, log, false, null, null); + assertDoesNotThrow(() -> cleaner.delete(basedir.toFile(), null, false, false, false)); + assertTrue(warnings.isEmpty()); + } finally { + setPosixFilePermissions(basedir, initialPermissions); + } + } +} From e951a26ab492e52ae842266e485f1fb6056e2fe6 Mon Sep 17 00:00:00 2001 From: Peter De Maeyer Date: Thu, 7 Nov 2024 11:07:41 +0100 Subject: [PATCH 2/6] Use Mockito + fix unit tests for Windows --- pom.xml | 6 ++ .../maven/plugins/clean/CleanerTest.java | 88 ++++++++----------- 2 files changed, 45 insertions(+), 49 deletions(-) diff --git a/pom.xml b/pom.xml index 73c3c72..40bb82e 100644 --- a/pom.xml +++ b/pom.xml @@ -108,6 +108,12 @@ under the License. junit-jupiter-api test + + org.mockito + mockito-core + 4.11.0 + test + org.codehaus.plexus plexus-testing diff --git a/src/test/java/org/apache/maven/plugins/clean/CleanerTest.java b/src/test/java/org/apache/maven/plugins/clean/CleanerTest.java index 10aa7e1..68eeec4 100644 --- a/src/test/java/org/apache/maven/plugins/clean/CleanerTest.java +++ b/src/test/java/org/apache/maven/plugins/clean/CleanerTest.java @@ -21,19 +21,17 @@ import java.io.IOException; import java.nio.file.AccessDeniedException; import java.nio.file.DirectoryNotEmptyException; +import java.nio.file.FileSystems; import java.nio.file.Path; import java.nio.file.attribute.PosixFilePermission; import java.nio.file.attribute.PosixFilePermissions; -import java.util.Iterator; -import java.util.LinkedHashMap; -import java.util.Map; -import java.util.Map.Entry; import java.util.Set; import org.apache.maven.plugin.logging.Log; -import org.apache.maven.plugin.testing.SilentLog; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; +import org.mockito.ArgumentCaptor; +import org.mockito.InOrder; import static java.nio.file.Files.createDirectory; import static java.nio.file.Files.createFile; @@ -45,36 +43,27 @@ import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertInstanceOf; import static org.junit.jupiter.api.Assertions.assertThrows; -import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assumptions.assumeTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.inOrder; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; class CleanerTest { - private boolean warnEnabled; + private static final boolean POSIX_COMPLIANT = + FileSystems.getDefault().supportedFileAttributeViews().contains("posix"); - /** - * Use a {@code LinkedHashMap} to preserve the order of the logged warnings. - */ - private final Map warnings = new LinkedHashMap<>(); - - /** - * Ideally we should use a mocking framework such as Mockito for this, but alas, this project has no such dependency. - */ - private final Log log = new SilentLog() { - - @Override - public boolean isWarnEnabled() { - return warnEnabled; - } - - @Override - public void warn(CharSequence content, Throwable error) { - warnings.put(content, error); - } - }; + private final Log log = mock(); @Test void deleteSucceedsDeeply(@TempDir Path tempDir) throws Exception { - final Path basedir = createDirectory(tempDir.resolve("target")); + assumeTrue(POSIX_COMPLIANT); + final Path basedir = createDirectory(tempDir.resolve("target")).toRealPath(); final Path file = createFile(basedir.resolve("file")); final Cleaner cleaner = new Cleaner(null, log, false, null, null); cleaner.delete(basedir.toFile(), null, false, true, false); @@ -84,8 +73,9 @@ void deleteSucceedsDeeply(@TempDir Path tempDir) throws Exception { @Test void deleteFailsWithoutRetryWhenNoPermission(@TempDir Path tempDir) throws Exception { - warnEnabled = true; - final Path basedir = createDirectory(tempDir.resolve("target")); + assumeTrue(POSIX_COMPLIANT); + when(log.isWarnEnabled()).thenReturn(true); + final Path basedir = createDirectory(tempDir.resolve("target")).toRealPath(); createFile(basedir.resolve("file")); final Set initialPermissions = getPosixFilePermissions(basedir); final String rwxrwxr_x = PosixFilePermissions.toString(initialPermissions); @@ -97,7 +87,7 @@ void deleteFailsWithoutRetryWhenNoPermission(@TempDir Path tempDir) throws Excep final Cleaner cleaner = new Cleaner(null, log, false, null, null); final IOException exception = assertThrows(IOException.class, () -> cleaner.delete(basedir.toFile(), null, false, true, false)); - assertTrue(warnings.isEmpty()); + verify(log, never()).warn(any(CharSequence.class), any(Throwable.class)); assertEquals("Failed to delete " + basedir, exception.getMessage()); final DirectoryNotEmptyException cause = assertInstanceOf(DirectoryNotEmptyException.class, exception.getCause()); @@ -110,7 +100,8 @@ void deleteFailsWithoutRetryWhenNoPermission(@TempDir Path tempDir) throws Excep @Test void deleteFailsAfterRetryWhenNoPermission(@TempDir Path tempDir) throws Exception { - final Path basedir = createDirectory(tempDir.resolve("target")); + assumeTrue(POSIX_COMPLIANT); + final Path basedir = createDirectory(tempDir.resolve("target")).toRealPath(); createFile(basedir.resolve("file")); final Set initialPermissions = getPosixFilePermissions(basedir); final String rwxrwxr_x = PosixFilePermissions.toString(initialPermissions); @@ -134,8 +125,9 @@ void deleteFailsAfterRetryWhenNoPermission(@TempDir Path tempDir) throws Excepti @Test void deleteLogsWarningWithoutRetryWhenNoPermission(@TempDir Path tempDir) throws Exception { - warnEnabled = true; - final Path basedir = createDirectory(tempDir.resolve("target")); + assumeTrue(POSIX_COMPLIANT); + when(log.isWarnEnabled()).thenReturn(true); + final Path basedir = createDirectory(tempDir.resolve("target")).toRealPath(); final Path file = createFile(basedir.resolve("file")); final Set initialPermissions = getPosixFilePermissions(basedir); final String rwxrwxr_x = PosixFilePermissions.toString(initialPermissions); @@ -145,18 +137,15 @@ void deleteLogsWarningWithoutRetryWhenNoPermission(@TempDir Path tempDir) throws try { final Cleaner cleaner = new Cleaner(null, log, false, null, null); assertDoesNotThrow(() -> cleaner.delete(basedir.toFile(), null, false, false, false)); - assertEquals(2, warnings.size()); - final Iterator> it = - warnings.entrySet().iterator(); - final Entry warning1 = it.next(); - assertEquals("Failed to delete " + file, warning1.getKey()); - final AccessDeniedException cause1 = assertInstanceOf(AccessDeniedException.class, warning1.getValue()); - assertEquals(file.toString(), cause1.getMessage()); - final Entry warning2 = it.next(); - assertEquals("Failed to delete " + basedir, warning2.getKey()); - final DirectoryNotEmptyException cause2 = - assertInstanceOf(DirectoryNotEmptyException.class, warning2.getValue()); - assertEquals(basedir.toString(), cause2.getMessage()); + verify(log, times(2)).warn(any(CharSequence.class), any(Throwable.class)); + InOrder inOrder = inOrder(log); + ArgumentCaptor cause1 = ArgumentCaptor.forClass(AccessDeniedException.class); + inOrder.verify(log).warn(eq("Failed to delete " + file), cause1.capture()); + assertEquals(file.toString(), cause1.getValue().getMessage()); + ArgumentCaptor cause2 = + ArgumentCaptor.forClass(DirectoryNotEmptyException.class); + inOrder.verify(log).warn(eq("Failed to delete " + basedir), cause2.capture()); + assertEquals(basedir.toString(), cause2.getValue().getMessage()); } finally { setPosixFilePermissions(basedir, initialPermissions); } @@ -164,8 +153,9 @@ void deleteLogsWarningWithoutRetryWhenNoPermission(@TempDir Path tempDir) throws @Test void deleteDoesNotLogAnythingWhenNoPermissionAndWarnDisabled(@TempDir Path tempDir) throws Exception { - warnEnabled = false; - final Path basedir = createDirectory(tempDir.resolve("target")); + assumeTrue(POSIX_COMPLIANT); + when(log.isWarnEnabled()).thenReturn(false); + final Path basedir = createDirectory(tempDir.resolve("target")).toRealPath(); createFile(basedir.resolve("file")); final Set initialPermissions = getPosixFilePermissions(basedir); final String rwxrwxr_x = PosixFilePermissions.toString(initialPermissions); @@ -175,7 +165,7 @@ void deleteDoesNotLogAnythingWhenNoPermissionAndWarnDisabled(@TempDir Path tempD try { final Cleaner cleaner = new Cleaner(null, log, false, null, null); assertDoesNotThrow(() -> cleaner.delete(basedir.toFile(), null, false, false, false)); - assertTrue(warnings.isEmpty()); + verify(log, never()).warn(any(CharSequence.class), any(Throwable.class)); } finally { setPosixFilePermissions(basedir, initialPermissions); } From 5f801d4e0988a689ae92c1af075bc83463b4881e Mon Sep 17 00:00:00 2001 From: Peter De Maeyer Date: Thu, 7 Nov 2024 19:39:59 +0100 Subject: [PATCH 3/6] Renamed variable error -> failure --- src/main/java/org/apache/maven/plugins/clean/Cleaner.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/main/java/org/apache/maven/plugins/clean/Cleaner.java b/src/main/java/org/apache/maven/plugins/clean/Cleaner.java index b43ebdc..52f1e25 100644 --- a/src/main/java/org/apache/maven/plugins/clean/Cleaner.java +++ b/src/main/java/org/apache/maven/plugins/clean/Cleaner.java @@ -273,8 +273,8 @@ private boolean isSymbolicLink(Path path) throws IOException { * @throws IOException If a file/directory could not be deleted and failOnError is true. */ private int delete(File file, boolean failOnError, boolean retryOnError) throws IOException { - IOException error = delete(file); - if (error != null) { + IOException failure = delete(file); + if (failure != null) { boolean deleted = false; if (retryOnError) { @@ -298,10 +298,10 @@ private int delete(File file, boolean failOnError, boolean retryOnError) throws if (!deleted) { if (failOnError) { - throw new IOException("Failed to delete " + file, error); + throw new IOException("Failed to delete " + file, failure); } else { if (log.isWarnEnabled()) { - log.warn("Failed to delete " + file, error); + log.warn("Failed to delete " + file, failure); } return 1; } From 1616a1bfab0d3aff83c9752a1898aec52ec77c3c Mon Sep 17 00:00:00 2001 From: Peter De Maeyer Date: Fri, 8 Nov 2024 18:31:25 +0100 Subject: [PATCH 4/6] Removed the resetting of permissions in tests, relying on JUnit @TempDir to take care of permissions issues when clearing the temporary directory --- .../maven/plugins/clean/CleanerTest.java | 81 ++++++++----------- 1 file changed, 32 insertions(+), 49 deletions(-) diff --git a/src/test/java/org/apache/maven/plugins/clean/CleanerTest.java b/src/test/java/org/apache/maven/plugins/clean/CleanerTest.java index 68eeec4..5cbb038 100644 --- a/src/test/java/org/apache/maven/plugins/clean/CleanerTest.java +++ b/src/test/java/org/apache/maven/plugins/clean/CleanerTest.java @@ -79,23 +79,18 @@ void deleteFailsWithoutRetryWhenNoPermission(@TempDir Path tempDir) throws Excep createFile(basedir.resolve("file")); final Set initialPermissions = getPosixFilePermissions(basedir); final String rwxrwxr_x = PosixFilePermissions.toString(initialPermissions); - // Prevent directory listing, which will result in a DirectoryNotEmptyException. + // Remove the executable flag to prevent directory listing, which will result in a DirectoryNotEmptyException. final String rw_rw_r__ = rwxrwxr_x.replace('x', '-'); final Set permissions = PosixFilePermissions.fromString(rw_rw_r__); setPosixFilePermissions(basedir, permissions); - try { - final Cleaner cleaner = new Cleaner(null, log, false, null, null); - final IOException exception = - assertThrows(IOException.class, () -> cleaner.delete(basedir.toFile(), null, false, true, false)); - verify(log, never()).warn(any(CharSequence.class), any(Throwable.class)); - assertEquals("Failed to delete " + basedir, exception.getMessage()); - final DirectoryNotEmptyException cause = - assertInstanceOf(DirectoryNotEmptyException.class, exception.getCause()); - assertEquals(basedir.toString(), cause.getMessage()); - } finally { - // Allow the tempDir to be cleared by the @TempDir extension. - setPosixFilePermissions(basedir, initialPermissions); - } + final Cleaner cleaner = new Cleaner(null, log, false, null, null); + final IOException exception = + assertThrows(IOException.class, () -> cleaner.delete(basedir.toFile(), null, false, true, false)); + verify(log, never()).warn(any(CharSequence.class), any(Throwable.class)); + assertEquals("Failed to delete " + basedir, exception.getMessage()); + final DirectoryNotEmptyException cause = + assertInstanceOf(DirectoryNotEmptyException.class, exception.getCause()); + assertEquals(basedir.toString(), cause.getMessage()); } @Test @@ -105,22 +100,17 @@ void deleteFailsAfterRetryWhenNoPermission(@TempDir Path tempDir) throws Excepti createFile(basedir.resolve("file")); final Set initialPermissions = getPosixFilePermissions(basedir); final String rwxrwxr_x = PosixFilePermissions.toString(initialPermissions); - // Prevent directory listing, which will result in a DirectoryNotEmptyException. + // Remove the executable flag to prevent directory listing, which will result in a DirectoryNotEmptyException. final String rw_rw_r__ = rwxrwxr_x.replace('x', '-'); final Set permissions = PosixFilePermissions.fromString(rw_rw_r__); setPosixFilePermissions(basedir, permissions); - try { - final Cleaner cleaner = new Cleaner(null, log, false, null, null); - final IOException exception = - assertThrows(IOException.class, () -> cleaner.delete(basedir.toFile(), null, false, true, true)); - assertEquals("Failed to delete " + basedir, exception.getMessage()); - final DirectoryNotEmptyException cause = - assertInstanceOf(DirectoryNotEmptyException.class, exception.getCause()); - assertEquals(basedir.toString(), cause.getMessage()); - } finally { - // Allow the tempDir to be cleared by the @TempDir extension. - setPosixFilePermissions(basedir, initialPermissions); - } + final Cleaner cleaner = new Cleaner(null, log, false, null, null); + final IOException exception = + assertThrows(IOException.class, () -> cleaner.delete(basedir.toFile(), null, false, true, true)); + assertEquals("Failed to delete " + basedir, exception.getMessage()); + final DirectoryNotEmptyException cause = + assertInstanceOf(DirectoryNotEmptyException.class, exception.getCause()); + assertEquals(basedir.toString(), cause.getMessage()); } @Test @@ -131,24 +121,20 @@ void deleteLogsWarningWithoutRetryWhenNoPermission(@TempDir Path tempDir) throws final Path file = createFile(basedir.resolve("file")); final Set initialPermissions = getPosixFilePermissions(basedir); final String rwxrwxr_x = PosixFilePermissions.toString(initialPermissions); + // Remove the writable flag to prevent deletion of the file, which will result in a AccessDeniedException. final String r_xr_xr_x = rwxrwxr_x.replace('w', '-'); final Set permissions = PosixFilePermissions.fromString(r_xr_xr_x); setPosixFilePermissions(basedir, permissions); - try { - final Cleaner cleaner = new Cleaner(null, log, false, null, null); - assertDoesNotThrow(() -> cleaner.delete(basedir.toFile(), null, false, false, false)); - verify(log, times(2)).warn(any(CharSequence.class), any(Throwable.class)); - InOrder inOrder = inOrder(log); - ArgumentCaptor cause1 = ArgumentCaptor.forClass(AccessDeniedException.class); - inOrder.verify(log).warn(eq("Failed to delete " + file), cause1.capture()); - assertEquals(file.toString(), cause1.getValue().getMessage()); - ArgumentCaptor cause2 = - ArgumentCaptor.forClass(DirectoryNotEmptyException.class); - inOrder.verify(log).warn(eq("Failed to delete " + basedir), cause2.capture()); - assertEquals(basedir.toString(), cause2.getValue().getMessage()); - } finally { - setPosixFilePermissions(basedir, initialPermissions); - } + final Cleaner cleaner = new Cleaner(null, log, false, null, null); + assertDoesNotThrow(() -> cleaner.delete(basedir.toFile(), null, false, false, false)); + verify(log, times(2)).warn(any(CharSequence.class), any(Throwable.class)); + InOrder inOrder = inOrder(log); + ArgumentCaptor cause1 = ArgumentCaptor.forClass(AccessDeniedException.class); + inOrder.verify(log).warn(eq("Failed to delete " + file), cause1.capture()); + assertEquals(file.toString(), cause1.getValue().getMessage()); + ArgumentCaptor cause2 = ArgumentCaptor.forClass(DirectoryNotEmptyException.class); + inOrder.verify(log).warn(eq("Failed to delete " + basedir), cause2.capture()); + assertEquals(basedir.toString(), cause2.getValue().getMessage()); } @Test @@ -159,15 +145,12 @@ void deleteDoesNotLogAnythingWhenNoPermissionAndWarnDisabled(@TempDir Path tempD createFile(basedir.resolve("file")); final Set initialPermissions = getPosixFilePermissions(basedir); final String rwxrwxr_x = PosixFilePermissions.toString(initialPermissions); + // Remove the writable flag to prevent deletion of the file, which will result in a AccessDeniedException. final String r_xr_xr_x = rwxrwxr_x.replace('w', '-'); final Set permissions = PosixFilePermissions.fromString(r_xr_xr_x); setPosixFilePermissions(basedir, permissions); - try { - final Cleaner cleaner = new Cleaner(null, log, false, null, null); - assertDoesNotThrow(() -> cleaner.delete(basedir.toFile(), null, false, false, false)); - verify(log, never()).warn(any(CharSequence.class), any(Throwable.class)); - } finally { - setPosixFilePermissions(basedir, initialPermissions); - } + final Cleaner cleaner = new Cleaner(null, log, false, null, null); + assertDoesNotThrow(() -> cleaner.delete(basedir.toFile(), null, false, false, false)); + verify(log, never()).warn(any(CharSequence.class), any(Throwable.class)); } } From 813acd613168af6bc82214cca95f08844dfdc028 Mon Sep 17 00:00:00 2001 From: Peter De Maeyer Date: Sat, 9 Nov 2024 12:25:37 +0100 Subject: [PATCH 5/6] Fixed typo a -> an --- src/test/java/org/apache/maven/plugins/clean/CleanerTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/java/org/apache/maven/plugins/clean/CleanerTest.java b/src/test/java/org/apache/maven/plugins/clean/CleanerTest.java index 5cbb038..d76c479 100644 --- a/src/test/java/org/apache/maven/plugins/clean/CleanerTest.java +++ b/src/test/java/org/apache/maven/plugins/clean/CleanerTest.java @@ -121,7 +121,7 @@ void deleteLogsWarningWithoutRetryWhenNoPermission(@TempDir Path tempDir) throws final Path file = createFile(basedir.resolve("file")); final Set initialPermissions = getPosixFilePermissions(basedir); final String rwxrwxr_x = PosixFilePermissions.toString(initialPermissions); - // Remove the writable flag to prevent deletion of the file, which will result in a AccessDeniedException. + // Remove the writable flag to prevent deletion of the file, which will result in an AccessDeniedException. final String r_xr_xr_x = rwxrwxr_x.replace('w', '-'); final Set permissions = PosixFilePermissions.fromString(r_xr_xr_x); setPosixFilePermissions(basedir, permissions); @@ -145,7 +145,7 @@ void deleteDoesNotLogAnythingWhenNoPermissionAndWarnDisabled(@TempDir Path tempD createFile(basedir.resolve("file")); final Set initialPermissions = getPosixFilePermissions(basedir); final String rwxrwxr_x = PosixFilePermissions.toString(initialPermissions); - // Remove the writable flag to prevent deletion of the file, which will result in a AccessDeniedException. + // Remove the writable flag to prevent deletion of the file, which will result in an AccessDeniedException. final String r_xr_xr_x = rwxrwxr_x.replace('w', '-'); final Set permissions = PosixFilePermissions.fromString(r_xr_xr_x); setPosixFilePermissions(basedir, permissions); From f35036f6d3d15c9e28173d3710f392a80d097bc5 Mon Sep 17 00:00:00 2001 From: Peter De Maeyer Date: Sat, 9 Nov 2024 12:32:14 +0100 Subject: [PATCH 6/6] Simplified the setting of permissions in tests --- .../maven/plugins/clean/CleanerTest.java | 21 ++++--------------- 1 file changed, 4 insertions(+), 17 deletions(-) diff --git a/src/test/java/org/apache/maven/plugins/clean/CleanerTest.java b/src/test/java/org/apache/maven/plugins/clean/CleanerTest.java index d76c479..e958664 100644 --- a/src/test/java/org/apache/maven/plugins/clean/CleanerTest.java +++ b/src/test/java/org/apache/maven/plugins/clean/CleanerTest.java @@ -36,7 +36,6 @@ import static java.nio.file.Files.createDirectory; import static java.nio.file.Files.createFile; import static java.nio.file.Files.exists; -import static java.nio.file.Files.getPosixFilePermissions; import static java.nio.file.Files.setPosixFilePermissions; import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -77,11 +76,8 @@ void deleteFailsWithoutRetryWhenNoPermission(@TempDir Path tempDir) throws Excep when(log.isWarnEnabled()).thenReturn(true); final Path basedir = createDirectory(tempDir.resolve("target")).toRealPath(); createFile(basedir.resolve("file")); - final Set initialPermissions = getPosixFilePermissions(basedir); - final String rwxrwxr_x = PosixFilePermissions.toString(initialPermissions); // Remove the executable flag to prevent directory listing, which will result in a DirectoryNotEmptyException. - final String rw_rw_r__ = rwxrwxr_x.replace('x', '-'); - final Set permissions = PosixFilePermissions.fromString(rw_rw_r__); + final Set permissions = PosixFilePermissions.fromString("rw-rw-r--"); setPosixFilePermissions(basedir, permissions); final Cleaner cleaner = new Cleaner(null, log, false, null, null); final IOException exception = @@ -98,11 +94,8 @@ void deleteFailsAfterRetryWhenNoPermission(@TempDir Path tempDir) throws Excepti assumeTrue(POSIX_COMPLIANT); final Path basedir = createDirectory(tempDir.resolve("target")).toRealPath(); createFile(basedir.resolve("file")); - final Set initialPermissions = getPosixFilePermissions(basedir); - final String rwxrwxr_x = PosixFilePermissions.toString(initialPermissions); // Remove the executable flag to prevent directory listing, which will result in a DirectoryNotEmptyException. - final String rw_rw_r__ = rwxrwxr_x.replace('x', '-'); - final Set permissions = PosixFilePermissions.fromString(rw_rw_r__); + final Set permissions = PosixFilePermissions.fromString("rw-rw-r--"); setPosixFilePermissions(basedir, permissions); final Cleaner cleaner = new Cleaner(null, log, false, null, null); final IOException exception = @@ -119,11 +112,8 @@ void deleteLogsWarningWithoutRetryWhenNoPermission(@TempDir Path tempDir) throws when(log.isWarnEnabled()).thenReturn(true); final Path basedir = createDirectory(tempDir.resolve("target")).toRealPath(); final Path file = createFile(basedir.resolve("file")); - final Set initialPermissions = getPosixFilePermissions(basedir); - final String rwxrwxr_x = PosixFilePermissions.toString(initialPermissions); // Remove the writable flag to prevent deletion of the file, which will result in an AccessDeniedException. - final String r_xr_xr_x = rwxrwxr_x.replace('w', '-'); - final Set permissions = PosixFilePermissions.fromString(r_xr_xr_x); + final Set permissions = PosixFilePermissions.fromString("r-xr-xr-x"); setPosixFilePermissions(basedir, permissions); final Cleaner cleaner = new Cleaner(null, log, false, null, null); assertDoesNotThrow(() -> cleaner.delete(basedir.toFile(), null, false, false, false)); @@ -143,11 +133,8 @@ void deleteDoesNotLogAnythingWhenNoPermissionAndWarnDisabled(@TempDir Path tempD when(log.isWarnEnabled()).thenReturn(false); final Path basedir = createDirectory(tempDir.resolve("target")).toRealPath(); createFile(basedir.resolve("file")); - final Set initialPermissions = getPosixFilePermissions(basedir); - final String rwxrwxr_x = PosixFilePermissions.toString(initialPermissions); // Remove the writable flag to prevent deletion of the file, which will result in an AccessDeniedException. - final String r_xr_xr_x = rwxrwxr_x.replace('w', '-'); - final Set permissions = PosixFilePermissions.fromString(r_xr_xr_x); + final Set permissions = PosixFilePermissions.fromString("r-xr-xr-x"); setPosixFilePermissions(basedir, permissions); final Cleaner cleaner = new Cleaner(null, log, false, null, null); assertDoesNotThrow(() -> cleaner.delete(basedir.toFile(), null, false, false, false));