gnodet-bot commented on code in PR #328: URL: https://github.com/apache/maven-clean-plugin/pull/328#discussion_r4083414386
########## src/test/java/org/apache/maven/plugins/clean/BackgroundCleanerTest.java: ########## @@ -0,0 +1,300 @@ +/* + * 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.Files; +import java.nio.file.Path; +import java.nio.file.attribute.PosixFilePermissions; +import java.util.HashMap; +import java.util.Map; +import java.util.function.Supplier; + +import org.apache.maven.api.Event; +import org.apache.maven.api.EventType; +import org.apache.maven.api.Listener; +import org.apache.maven.api.Session; +import org.apache.maven.api.SessionData; +import org.apache.maven.api.plugin.Log; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.condition.DisabledOnOs; +import org.junit.jupiter.api.condition.OS; +import org.junit.jupiter.api.io.TempDir; +import org.mockito.ArgumentCaptor; + +import static java.nio.file.Files.createDirectory; +import static java.nio.file.Files.createFile; +import static java.nio.file.Files.exists; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.atLeastOnce; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +/** + * Unit tests for {@link BackgroundCleaner}'s new batch-retry, session-scoping, + * and leftover-scan logic introduced in the fast-clean refactor. + */ +@DisabledOnOs(OS.WINDOWS) Review Comment: ⚠️ **`@DisabledOnOs(OS.WINDOWS)` is class-level but only one test method requires POSIX APIs** `batchRetryWithForceDeletesReadOnlyFile` is the only test that calls `Files.setPosixFilePermissions` — a POSIX-only API. The other six tests use only `Files.createDirectory`, `Files.createFile`, and standard Mockito; they have no OS dependency and should run on Windows. Applying the annotation at class level means the six OS-agnostic tests — including the session-scoping, leftover-scan, and basic background-deletion tests — are silently skipped on Windows CI. This is particularly unfortunate given that MCLEAN-102 is a Windows-specific regression. Move the annotation to the single method that actually needs it: ```suggestion class BackgroundCleanerTest { ``` And add `@DisabledOnOs(OS.WINDOWS)` directly above `void batchRetryWithForceDeletesReadOnlyFile`. ########## src/test/java/org/apache/maven/plugins/clean/BackgroundCleanerTest.java: ########## @@ -0,0 +1,300 @@ +/* + * 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.Files; +import java.nio.file.Path; +import java.nio.file.attribute.PosixFilePermissions; +import java.util.HashMap; +import java.util.Map; +import java.util.function.Supplier; + +import org.apache.maven.api.Event; +import org.apache.maven.api.EventType; +import org.apache.maven.api.Listener; +import org.apache.maven.api.Session; +import org.apache.maven.api.SessionData; +import org.apache.maven.api.plugin.Log; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.condition.DisabledOnOs; +import org.junit.jupiter.api.condition.OS; +import org.junit.jupiter.api.io.TempDir; +import org.mockito.ArgumentCaptor; + +import static java.nio.file.Files.createDirectory; +import static java.nio.file.Files.createFile; +import static java.nio.file.Files.exists; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.atLeastOnce; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +/** + * Unit tests for {@link BackgroundCleaner}'s new batch-retry, session-scoping, + * and leftover-scan logic introduced in the fast-clean refactor. + */ +@DisabledOnOs(OS.WINDOWS) +class BackgroundCleanerTest { + + /** + * Minimal in-memory {@link SessionData} that supports {@link #computeIfAbsent}. + */ + private static class FakeSessionData implements SessionData { + private final Map<Key<?>, Object> store = new HashMap<>(); + + @SuppressWarnings("unchecked") + @Override + public <T> T get(Key<T> key) { + return (T) store.get(key); + } + + @Override + public <T> void set(Key<T> key, T value) { + store.put(key, value); + } + + @SuppressWarnings("unchecked") + @Override + public <T> T computeIfAbsent(Key<T> key, Supplier<T> supplier) { + return (T) store.computeIfAbsent(key, k -> supplier.get()); + } + + @SuppressWarnings("unchecked") + @Override + public <T> boolean replace(Key<T> key, T expected, T value) { + T current = (T) store.get(key); + if (current == expected) { + store.put(key, value); + return true; + } + return false; + } + } + + /** + * Creates a mock {@link Session} backed by a real {@link FakeSessionData} so that + * {@code computeIfAbsent} works correctly across multiple calls to {@code getOrCreate}. + * + * <p>The returned captor can be used to retrieve the registered {@link Listener} and + * fire synthetic events at it.</p> + */ + private Session mockSession(ArgumentCaptor<Listener> listenerCaptor) { + Session session = mock(Session.class); + FakeSessionData data = new FakeSessionData(); + when(session.getData()).thenReturn(data); + if (listenerCaptor != null) { + // Capture any listener registered during BackgroundCleaner construction. + org.mockito.Mockito.doNothing().when(session).registerListener(listenerCaptor.capture()); + } + return session; + } + + // ----------------------------------------------------------------------- + // getOrCreate — session-scoping + // ----------------------------------------------------------------------- + + /** + * Two calls to {@link BackgroundCleaner#getOrCreate} with the same parameters must + * return the same instance (session-scoped singleton). + */ + @Test + void getOrCreateReturnsSameInstance(@TempDir Path tempDir) throws IOException { + Path fastDir = tempDir.resolve(".clean"); + Log log = mock(Log.class); + ArgumentCaptor<Listener> captor = ArgumentCaptor.forClass(Listener.class); + Session session = mockSession(captor); + + BackgroundCleaner bc1 = BackgroundCleaner.getOrCreate(session, log, fastDir, FastMode.BACKGROUND); + BackgroundCleaner bc2 = BackgroundCleaner.getOrCreate(session, log, fastDir, FastMode.BACKGROUND); + + assertSame(bc1, bc2, "getOrCreate must return the same instance for the same session"); + // Constructor registers exactly one listener + verify(session, atLeastOnce()).registerListener(any(Listener.class)); + } + + /** + * When a second subproject calls {@link BackgroundCleaner#getOrCreate} with a different + * {@code fastMode}, a debug-level warning must be emitted (first-wins semantics). + */ + @Test + void getOrCreateLogsDebugOnConfigMismatch(@TempDir Path tempDir) throws IOException { + Path fastDir = tempDir.resolve(".clean"); + Log log = mock(Log.class); + Session session = mockSession(null); + + BackgroundCleaner.getOrCreate(session, log, fastDir, FastMode.BACKGROUND); + // Second call with different fastMode — should log a debug message. + BackgroundCleaner.getOrCreate(session, log, fastDir, FastMode.AT_END); + + verify(log, atLeastOnce()).debug(any(CharSequence.class)); + } + + /** + * When a second subproject calls {@link BackgroundCleaner#getOrCreate} with the same + * parameters, no debug warning about a mismatch must be emitted. + */ + @Test + void getOrCreateNoDebugWhenSameConfig(@TempDir Path tempDir) throws IOException { + Path fastDir = tempDir.resolve(".clean"); + Log log = mock(Log.class); + Session session = mockSession(null); + + BackgroundCleaner.getOrCreate(session, log, fastDir, FastMode.BACKGROUND); + BackgroundCleaner.getOrCreate(session, log, fastDir, FastMode.BACKGROUND); + + verify(log, never()).debug(any(CharSequence.class)); + } + + // ----------------------------------------------------------------------- + // deleteInBackground — basic deletion + // ----------------------------------------------------------------------- + + /** + * Files placed in the staging area via {@link BackgroundCleaner#fastDelete} must be + * deleted in the background before the session ends. + * + * <p><b>Note on assertion strategy:</b> {@code fastDelete} moves {@code target} to a staging + * directory under {@code fastDir} <em>synchronously</em>, so {@code exists(target)} becomes + * {@code false} immediately — before any background deletion runs. The meaningful assertion is + * that the staging area inside {@code fastDir} is empty after {@code onEvent} has drained the + * executor (i.e. background deletion actually completed).</p> + */ + @Test + void fastDeleteRemovesDirectoryInBackground(@TempDir Path tempDir) throws Exception { + Path fastDir = tempDir.resolve(".clean"); + Path target = createDirectory(tempDir.resolve("target")); + createFile(target.resolve("file.txt")); + + Log log = mock(Log.class); + ArgumentCaptor<Listener> captor = ArgumentCaptor.forClass(Listener.class); + Session session = mockSession(captor); + + BackgroundCleaner bc = BackgroundCleaner.getOrCreate(session, log, fastDir, FastMode.BACKGROUND); + assertTrue(bc.fastDelete(target, false, true)); + + // After fastDelete(), target has been moved to fastDir — the staging area exists. + assertTrue(exists(fastDir), "staging directory must exist after fastDelete"); + + // Fire SESSION_ENDED: onEvent shuts down the executor and waits for completion. + Event event = mock(Event.class); + when(event.getType()).thenReturn(EventType.SESSION_ENDED); + captor.getValue().onEvent(event); + + // After onEvent returns, the background thread has finished and run() has cleaned up. + // fastDir itself is deleted by run() when it is empty (directoriesToDeleteIfEmpty). + assertFalse(exists(fastDir), "staging directory must be deleted after background clean completes"); + } + + // ----------------------------------------------------------------------- + // deleteInBackground — batch retry with force + // ----------------------------------------------------------------------- + + /** + * With {@code force=true}, a read-only file that would fail a plain + * {@link Files#deleteIfExists} must still be deleted: the batch-retry path + * must call {@code tryDeleteOnce(path, force)} (which makes the file writable) + * rather than raw {@code Files.deleteIfExists}. + * + * <p><b>Note on assertion strategy:</b> {@code fastDelete} moves the entire {@code target} + * tree (including the read-only file) to a staging directory under {@code fastDir} + * <em>synchronously</em>. After that move, neither {@code target} nor {@code readOnly} + * exist at their original paths — the assertions would pass trivially. The meaningful + * check is that the staging area itself is empty after {@code onEvent} completes, which + * proves that {@code tryDeleteOnce(path, true)} successfully handled the read-only file + * inside the staging tree.</p> + */ + @Test + void batchRetryWithForceDeletesReadOnlyFile(@TempDir Path tempDir) throws Exception { + Path fastDir = tempDir.resolve(".clean"); + Path target = createDirectory(tempDir.resolve("target")); + Path readOnly = createFile(target.resolve("ro.txt")); + // Make the file read-only so the first pass fails. + Files.setPosixFilePermissions(readOnly, PosixFilePermissions.fromString("r--r--r--")); + + Log log = mock(Log.class); + ArgumentCaptor<Listener> captor = ArgumentCaptor.forClass(Listener.class); + Session session = mockSession(captor); + + BackgroundCleaner bc = BackgroundCleaner.getOrCreate(session, log, fastDir, FastMode.BACKGROUND); Review Comment: 💬 **`batchRetryWithForceDeletesReadOnlyFile` doesn't exercise the batch retry path** The test name implies the second-pass sleep-and-retry loop is being tested. It isn't. `tryDeleteOnce(file, force=true)` handles `AccessDeniedException` in the *first pass*: it calls `Cleaner.setWritable()` and retries immediately. The file succeeds on the first attempt, so it never enters the `failures` list, meaning `Thread.sleep(BATCH_RETRY_DELAY_MS)` is never reached. What the test actually verifies is that `tryDeleteOnce` correctly handles a read-only file when `force=true` — which is still valuable coverage, just not what the name says. The actual batch retry path (sleep + second pass) is only reachable when a file fails even with `force=true` (e.g. a file held open by another process on Windows). That scenario is hard to test portably, but the test name should at minimum reflect what it actually does: ```suggestion void forceDeleteHandlesReadOnlyFile(@TempDir Path tempDir) throws Exception { ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
