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

[MCLEAN-124] Leverage Files.delete(Path) API to provide more accurate reason in case of failure by peterdemaeyer · Pull Request #60 · apache/maven-clean-plugin

peterdemaeyer changed the base branch from master to maven-clean-plugin-3.x

November 7, 2024 08:00

…eason in case of failure

		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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can simply add Mockito to dependencies

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might want to do this in a separate pr first

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do the tests work now on windows?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 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

		*/
		private int delete(File file, boolean failOnError, boolean retryOnError) throws IOException {
		if (!file.delete()) {
		IOException error = delete(file);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not a Java error; please rename the variable

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I renamed it to failure.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might want to do this in a separate pr first

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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."

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why reset?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my other answer.

		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', '-');

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

		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', '-');

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

		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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in a --> in an