[MCLEAN-124] Leverage Files.delete(Path) API to provide more accurate reason in case of failure #60

peterdemaeyer · 2024-11-07T07:47:12Z

No description provided.

…eason in case of failure

slawekjaranowski · 2024-11-07T08:39:42Z

src/test/java/org/apache/maven/plugins/clean/CleanerTest.java

+    private final Map<CharSequence, Throwable> warnings = new LinkedHashMap<>();
+
+    /**
+     * Ideally we should use a mocking framework such as Mockito for this, but alas, this project has no such dependency.


you can simply add Mockito to dependencies

Ok, good to know. I had to settle for Mockito 4.x/Java 8 instead of 5.x/Java 11, but it's an improvement nonetheless.
I also needed to fix the tests for Windows paths.

peterdemaeyer · 2024-11-07T10:11:08Z

src/test/java/org/apache/maven/plugins/clean/CleanerTest.java


    @Test
    void deleteSucceedsDeeply(@TempDir Path tempDir) throws Exception {
-        final Path basedir = createDirectory(tempDir.resolve("target"));
+        final Path basedir = createDirectory(tempDir.resolve("target")).toRealPath();


This is an attempt to fix the tests so that they also work on Windows.

might want to do this in a separate pr first

Hmm, I can't. This code doesn't exist outside of this PR, so I can't create a separate PR for it.

do the tests work now on windows?

No, I disabled the tests on non-POSIX-compliant OSes using JUnit 5 Assumptions. I deemed it acceptable, because this is not a test for specific features and failures on all OSes, it's a test for Cleaner providing it with a variety of failures. It doesn't matter if these failure are exactly the same on all OSes, as long as there is at least one POSIX-compliant OS in the test suite, which is the case, it's enough to cover the feature.

elharo · 2024-11-07T18:26:59Z

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 <code>failOnError</code> is <code>true</code>.
     */
    private int delete(File file, boolean failOnError, boolean retryOnError) throws IOException {
-        if (!file.delete()) {
+        IOException error = delete(file);


this is not a Java error; please rename the variable

Will do.
To justify why I named it like this: it was to be consistent with the context of the existing terminology failOnError - it is the error in that context.

I renamed it to failure.

elharo · 2024-11-07T18:28:20Z

src/test/java/org/apache/maven/plugins/clean/CleanerTest.java


    @Test
    void deleteSucceedsDeeply(@TempDir Path tempDir) throws Exception {
-        final Path basedir = createDirectory(tempDir.resolve("target"));
+        final Path basedir = createDirectory(tempDir.resolve("target")).toRealPath();


might want to do this in a separate pr first

elharo · 2024-11-07T19:50:48Z

src/test/java/org/apache/maven/plugins/clean/CleanerTest.java

+            inOrder.verify(log).warn(eq("Failed to delete " + basedir), cause2.capture());
+            assertEquals(basedir.toString(), cause2.getValue().getMessage());
+        } finally {
+            setPosixFilePermissions(basedir, initialPermissions);


why do you need to reset this here? Feels like the object shouldn't be used again

To avoid resource exhaustion. It's good practice to reset persistent state which is modified in a test. The files I make in this test are persistent, and I rely on the @TempDir extension in the JUnit framework to delete them. If I leave permissions as they are, i.e. deliberately restricted so that files cannot be deleted, I imagine it could be that deleting the directory fails, resulting not per se in an exception but in lingering temporary directories. Over time, lingering temporary directories can cause "no space left on device" on test systems. By resetting the permissions, I reset the persistent state, allowing the JUnit framework to clear the temporary directory, avoiding resource exhaustion.

Something's off with that. Test resources including files should not be shared between test methods. Per Junit docs "The temporary directory will be shared by all tests in a class when the annotation is present on a static field or on a parameter of a @BeforeAll method. Otherwise — for example, when @tempdir is only used on instance fields or on parameters in test, @BeforeEach, or @AfterEach methods — each test will use its own temporary directory."

I think there is a misunderstanding. I agree that test resources including files should not be shared between test methods. That's exactly why I did not share them between tests. According to you quote of the JUnit docs, they fall in the category "@TempDir is used on parameters in tests", in which case "each test will use its own temporary directory", which is exactly what we want, no?

So I don't think these are what I would call "persistent'. Either way, let's start with the assumption you don't need this. Take out the finally block and see if everything still passes.

elharo · 2024-11-07T19:51:11Z

src/test/java/org/apache/maven/plugins/clean/CleanerTest.java

+            assertDoesNotThrow(() -> cleaner.delete(basedir.toFile(), null, false, false, false));
+            verify(log, never()).warn(any(CharSequence.class), any(Throwable.class));
+        } finally {
+            setPosixFilePermissions(basedir, initialPermissions);


See my other answer.

elharo · 2024-11-08T13:44:24Z

src/test/java/org/apache/maven/plugins/clean/CleanerTest.java

+        final Set<PosixFilePermission> 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', '-');


Is replace needed here? Could this just be a string literal?

Replace is not strictly needed, it could be a string literal, but the replace reflects better the intention. I just want to remove the executable flag, and leave any other "rw" settings as they were. That's exactly what this replace expresses. If a make it a string literal, it is not clear to the reader that in fact I only mean to change the executable flag. The name of the variable rw_rw_r__ is merely suggestive, to reflect the sort of value the string can have.

This feels too clever by half. Let's just make it a string literal.

elharo · 2024-11-08T13:45:24Z

src/test/java/org/apache/maven/plugins/clean/CleanerTest.java

+                    assertInstanceOf(DirectoryNotEmptyException.class, exception.getCause());
+            assertEquals(basedir.toString(), cause.getMessage());
+        } finally {
+            // Allow the tempDir to be cleared by the @TempDir extension.


This might be what I'm missing. The annotation doesn't work with the permissions you set?

Maybe it does, maybe it doesn't. I didn't check. I deem it good practice to reset any modified persistent state so that I don't have to make assumptions about how "powerful" the @TempDir extension really is.

I checked, the @TempDir framework is smart enough to fix the permissions to that the tmp dir can be deleted. So I removed all the try-catches.

elharo · 2024-11-08T13:47:33Z

src/test/java/org/apache/maven/plugins/clean/CleanerTest.java

+            inOrder.verify(log).warn(eq("Failed to delete " + basedir), cause2.capture());
+            assertEquals(basedir.toString(), cause2.getValue().getMessage());
+        } finally {
+            setPosixFilePermissions(basedir, initialPermissions);


So I don't think these are what I would call "persistent'. Either way, let's start with the assumption you don't need this. Take out the finally block and see if everything still passes.

to take care of permissions issues when clearing the temporary directory

elharo · 2024-11-08T18:26:47Z

src/test/java/org/apache/maven/plugins/clean/CleanerTest.java

+        final Set<PosixFilePermission> 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', '-');


This feels too clever by half. Let's just make it a string literal.

elharo · 2024-11-08T18:27:49Z

src/test/java/org/apache/maven/plugins/clean/CleanerTest.java

+        final Path file = createFile(basedir.resolve("file"));
+        final Set<PosixFilePermission> 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.


in a --> in an

peterdemaeyer changed the base branch from master to maven-clean-plugin-3.x November 7, 2024 08:00

MCLEAN-124 Leverage Files.delete(Path) API to provide more accurate r…

0f1885c

…eason in case of failure

peterdemaeyer force-pushed the MCLEAN-124 branch from 32f973d to 0f1885c Compare November 7, 2024 08:06

slawekjaranowski reviewed Nov 7, 2024

View reviewed changes

peterdemaeyer commented Nov 7, 2024

View reviewed changes

peterdemaeyer force-pushed the MCLEAN-124 branch from 8a7dc7b to 20a6fca Compare November 7, 2024 12:32

Use Mockito + fix unit tests for Windows

e951a26

peterdemaeyer force-pushed the MCLEAN-124 branch from 20a6fca to e951a26 Compare November 7, 2024 13:18

elharo changed the title ~~MCLEAN-124 Leverage Files.delete(Path) API to provide more accurate reason in case of failure~~ [MCLEAN-124] Leverage Files.delete(Path) API to provide more accurate reason in case of failure Nov 7, 2024

elharo reviewed Nov 7, 2024

View reviewed changes

Renamed variable error -> failure

5f801d4

elharo reviewed Nov 7, 2024

View reviewed changes

elharo reviewed Nov 8, 2024

View reviewed changes

Removed the resetting of permissions in tests, relying on JUnit @tempdir

1616a1b

to take care of permissions issues when clearing the temporary directory

elharo reviewed Nov 8, 2024

View reviewed changes

peterdemaeyer added 2 commits November 9, 2024 12:25

Fixed typo a -> an

813acd6

Simplified the setting of permissions in tests

f35036f

elharo approved these changes Nov 9, 2024

View reviewed changes

elharo merged commit cb2127f into apache:maven-clean-plugin-3.x Nov 9, 2024
26 checks passed

peterdemaeyer deleted the MCLEAN-124 branch November 10, 2024 18:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MCLEAN-124] Leverage Files.delete(Path) API to provide more accurate reason in case of failure #60

[MCLEAN-124] Leverage Files.delete(Path) API to provide more accurate reason in case of failure #60

peterdemaeyer commented Nov 7, 2024

slawekjaranowski Nov 7, 2024

peterdemaeyer Nov 7, 2024

peterdemaeyer Nov 7, 2024 •

edited

Loading

elharo Nov 7, 2024

peterdemaeyer Nov 7, 2024

elharo Nov 7, 2024

peterdemaeyer Nov 7, 2024

elharo Nov 7, 2024

peterdemaeyer Nov 7, 2024

peterdemaeyer Nov 7, 2024

elharo Nov 7, 2024

elharo Nov 7, 2024

peterdemaeyer Nov 7, 2024

elharo Nov 7, 2024

peterdemaeyer Nov 8, 2024 •

edited

Loading

elharo Nov 8, 2024

elharo Nov 7, 2024

peterdemaeyer Nov 7, 2024

elharo Nov 8, 2024

peterdemaeyer Nov 8, 2024

elharo Nov 8, 2024

elharo Nov 8, 2024

peterdemaeyer Nov 8, 2024

peterdemaeyer Nov 9, 2024

elharo Nov 8, 2024

elharo Nov 8, 2024

elharo Nov 8, 2024

[MCLEAN-124] Leverage Files.delete(Path) API to provide more accurate reason in case of failure #60

[MCLEAN-124] Leverage Files.delete(Path) API to provide more accurate reason in case of failure #60

Conversation

peterdemaeyer commented Nov 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterdemaeyer Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterdemaeyer Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterdemaeyer Nov 7, 2024 •

edited

Loading

peterdemaeyer Nov 8, 2024 •

edited

Loading