Conversation
c0c6ab5 to
e53bc5b
Compare
23a9861 to
23baa39
Compare
There was a problem hiding this comment.
Pull request overview
This pull request adds an automated mechanism to track and clean up orphaned approved files that remain after test methods are renamed or deleted. The implementation introduces an inventory system that records the mapping between approved files and their originating test methods during test runs, and provides build tool plugins (Gradle and Maven) to identify and remove orphaned files.
Changes:
- Added
ApprovedFileInventorysystem to track approved file-to-test mappings in.approvej/inventory.properties - Created Gradle and Maven plugins with tasks/mojos to find and cleanup orphaned approved files
- Added
inventoryEnabledconfiguration option that defaults to enabled locally but disabled in CI environments
Reviewed changes
Copilot reviewed 25 out of 26 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| settings.gradle.kts | Added plugin modules to the build |
| plugins/gradle/src/main/java/org/approvej/gradle/ApproveJPlugin.java | Gradle plugin that registers approvejFindOrphans and approvejCleanup tasks |
| plugins/gradle/src/test/java/org/approvej/gradle/ApproveJPluginTest.java | Test coverage for Gradle plugin functionality |
| plugins/gradle/build.gradle.kts | Build configuration for Gradle plugin with plugin-publish support |
| plugins/maven/src/main/java/org/approvej/maven/FindOrphansMojo.java | Maven mojo to list orphaned files |
| plugins/maven/src/main/java/org/approvej/maven/CleanupMojo.java | Maven mojo to remove orphaned files |
| plugins/maven/src/main/java/org/approvej/maven/MojoHelper.java | Shared helper to fork JVM and execute inventory operations |
| plugins/maven/src/test/java/org/approvej/maven/MojoHelperTest.java | Test coverage for Maven helper functionality |
| plugins/maven/build.gradle.kts | Build configuration for Maven plugin |
| plugins/maven/gradle.properties | Maven plugin metadata |
| modules/core/src/main/java/org/approvej/approve/ApprovedFileInventory.java | Core inventory implementation with find/remove operations |
| modules/core/src/main/java/org/approvej/approve/InventoryEntry.java | Data structure for inventory entries |
| modules/core/src/main/java/org/approvej/configuration/Configuration.java | Added inventoryEnabled configuration field and CI detection logic |
| modules/core/src/main/java/org/approvej/configuration/ConfigurationLoader.java | Added getenv() method for raw environment variable access |
| modules/core/src/main/java/org/approvej/ApprovalBuilder.java | Integration point to register approved files in inventory |
| modules/core/src/test/java/org/approvej/approve/ApprovedFileInventoryTest.java | Comprehensive test coverage for inventory operations |
| modules/core/src/test/java/org/approvej/configuration/ConfigurationTest.java | Test coverage for inventory configuration |
| modules/core/src/test/resources/approvej.properties | Test configuration with inventoryEnabled |
| gradle/libs.versions.toml | Added Maven and plugin-publish dependencies |
| bom/build.gradle.kts | Excluded plugin modules from BOM |
| .gitignore | Added .approvej directory to ignore inventory files |
| .github/workflows/publish.yml | Added Gradle plugin publishing step |
| .github/workflows/build-pr.yml | Updated test report paths to include plugins |
| .github/workflows/build-main.yml | Updated test report paths to include plugins |
| CONTRIBUTING.md | Updated documentation to describe plugin modules |
| CLAUDE.md | Updated project structure documentation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
modules/core/src/main/java/org/approvej/approve/ApprovedFileInventory.java
Outdated
Show resolved
Hide resolved
modules/core/src/main/java/org/approvej/approve/ApprovedFileInventory.java
Outdated
Show resolved
Hide resolved
modules/core/src/main/java/org/approvej/configuration/Configuration.java
Show resolved
Hide resolved
modules/core/src/main/java/org/approvej/approve/ApprovedFileInventory.java
Outdated
Show resolved
Hide resolved
modules/core/src/main/java/org/approvej/approve/ApprovedFileInventory.java
Outdated
Show resolved
Hide resolved
modules/core/src/main/java/org/approvej/approve/ApprovedFileInventory.java
Outdated
Show resolved
Hide resolved
modules/core/src/main/java/org/approvej/approve/ApprovedFileInventory.java
Show resolved
Hide resolved
modules/core/src/test/java/org/approvej/configuration/ConfigurationTest.java
Show resolved
Hide resolved
modules/core/src/main/java/org/approvej/approve/ApprovedFileInventory.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 30 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
modules/core/src/main/java/org/approvej/approve/ApprovedFileInventory.java
Outdated
Show resolved
Hide resolved
modules/core/src/main/java/org/approvej/approve/ApprovedFileInventory.java
Outdated
Show resolved
Hide resolved
modules/core/src/main/java/org/approvej/approve/ApprovedFileInventory.java
Show resolved
Hide resolved
Track approved files in .approvej/inventory.properties so that orphaned files from renamed or deleted tests can be detected and cleaned up. The inventory is collected in-memory during a test run and written at JVM shutdown, merging with existing entries. Adds inventoryEnabled configuration (auto-disabled in CI), Gradle tasks approvejFindOrphans and approvejCleanup, and reflection-based orphan detection via a main() entry point.
Add maven-plugin module with find-orphans and cleanup goals that fork a JVM to run ApprovedFileInventory, mirroring the existing Gradle plugin functionality for Maven users.
Move maven-* library entries after junit-* entries in
libs.versions.toml to restore alphabetical order.
Route CI environment variable lookup through ConfigurationLoader
so tests can control it. Previously resolveInventoryEnabled called
System.getenv("CI") directly, causing the defaults test to fail
in CI where CI=true.
Add in-process unit tests using ProjectBuilder alongside the existing GradleRunner functional tests. These run in the same JVM so coverage tools can track them, and they verify task registration, group, description, main class, and args. Also tests that plugin order does not matter (Java before or after ApproveJ).
Enable the jacoco plugin and XML report generation so SonarCloud can pick up test coverage for both plugin modules.
Enable inventoryEnabled in test properties so the ApprovalBuilder.byFile() inventory recording line is covered in CI where the CI env var would otherwise disable it. Add executeInventory test that exercises the process forking and non-zero exit code path in MojoHelper.
Rename record() to registerApprovedFile() to avoid restricted identifier. Join assertion chains in ApprovedFileInventoryTest where multiple assertions share the same subject.
Separate build tool plugins from library modules by moving them from modules/ to a dedicated plugins/ directory: - modules/gradle-plugin/ → plugins/gradle/ - modules/maven-plugin/ → plugins/maven/ Update all references in settings.gradle.kts, CI workflows, bom/build.gradle.kts, CLAUDE.md, and CONTRIBUTING.md.
InventoryEntry now validates the className#methodName format in its constructor and stores className and methodName as record components, replacing the inline parsing in ApprovedFileInventory.
Exclude Gradle plugin from Maven Central deployment since it is published to the Gradle Plugin Portal only. Set Maven plugin artifact ID to approvej-maven-plugin via gradle.properties to match Maven's plugin naming convention.
Name shutdown hook thread for easier debugging. Only remove inventory entry when file deletion succeeds. Read process stdout and stderr concurrently to prevent potential deadlock. Add test for inventory disabled in CI environments.
Rename plugin directories from plugins/gradle and plugins/maven to plugins/approvej-gradle-plugin and plugins/approvej-maven-plugin so the Gradle project names produce correct Maven artifact IDs. Exclude the Gradle plugin from Maven Central publishing since it is published to the Gradle Plugin Portal separately. Replace "orphan" terminology with "leftover" throughout code, tests, plugin tasks/goals, and documentation for sensitivity. Print file:// URIs in CLI output so paths are clickable in IDEs.
9882317 to
911f45e
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 31 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
CONTRIBUTING.md
Outdated
| - [gradle](plugins/gradle) contains the Gradle plugin for managing approved files | ||
| - [maven](plugins/maven) contains the Maven plugin for managing approved files |
There was a problem hiding this comment.
The paths to the plugin directories are incorrect. The actual directory names are plugins/approvej-gradle-plugin and plugins/approvej-maven-plugin, not plugins/gradle and plugins/maven. The markdown links should be updated to match the actual directory structure.
Rename test methods to start with the method under test: - registerApprovedFile/record_* → writeInventory_* in ApprovedFileInventoryTest - approvejFindLeftovers/approvejCleanup_* → apply_* in ApproveJPluginTest Reorder test methods to match production code order: - Group loadConfiguration_inventoryEnabled_* with other loadConfiguration tests - Move toEnvironmentVariableName after get tests in ConfigurationTest - Move executeInventory before buildCommand in MojoHelperTest
Update plugin paths to match actual directory names, add missing modules (json-jackson3, yaml-jackson3, http-wiremock), and replace leftover "Shakespeare" reference with "ApproveJ".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 31 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| saveInventory(inventory); | ||
|
|
||
| return leftovers; | ||
| } |
There was a problem hiding this comment.
removeLeftovers() returns the full leftovers list even when deleting a file fails (IOException). This makes the --remove CLI output claim files were removed when they may still exist, and callers can’t distinguish partial failures. Consider collecting and returning only successfully deleted entries (and/or failing the command with a non-zero exit / exception when any deletion fails).
There was a problem hiding this comment.
Fixed in 9be849a — removeLeftovers() now collects and returns only successfully deleted entries.
| static void reset(Path testInventoryFile) { | ||
| entries.clear(); | ||
| executedMethods.clear(); | ||
| shutdownHookRegistered.set(false); |
There was a problem hiding this comment.
reset(Path) sets shutdownHookRegistered back to false, but any previously registered shutdown hook can’t be automatically unregistered. If registerApprovedFile() was called before reset(), subsequent calls can register additional shutdown hooks, leading to multiple inventory-writer threads running at JVM shutdown. Consider keeping a reference to the hook thread and removing it in reset() via Runtime.removeShutdownHook(...), or avoid resetting shutdownHookRegistered in reset().
| shutdownHookRegistered.set(false); |
There was a problem hiding this comment.
Fixed in 9be849a — replaced AtomicBoolean with AtomicReference<Thread>. The hook is now properly removed via Runtime.removeShutdownHook() in reset().
plugins/approvej-gradle-plugin/src/main/java/org/approvej/gradle/ApproveJPlugin.java
Show resolved
Hide resolved
plugins/approvej-maven-plugin/src/main/java/org/approvej/maven/FindLeftoversMojo.java
Show resolved
Hide resolved
Remove writeInventory_merges_with_existing (duplicate of writeInventory_preserves_entries_for_unexecuted_methods) and configurationLoader_emptyBuilder_returnsLoaderWithNoSources (duplicate of configurationLoader_get_returnsDefaultWhenNoSourcesConfigured). Collapse buildCommand_find and buildCommand_remove into a single parameterized test.
- Use removeIf for clearer intent when removing executed method entries - Return only successfully deleted entries from removeLeftovers - Make inventoryFile volatile for thread safety - Track shutdown hook via AtomicReference to avoid duplicate registration - Handle path relativization failure across filesystem roots
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 31 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .register( | ||
| "approvejFindLeftovers", | ||
| JavaExec.class, |
There was a problem hiding this comment.
PR description mentions Gradle task approvejFindOrphans (and Maven goal find-orphans), but the implementation registers approvejFindLeftovers (and Maven uses find-leftovers). Please align the PR description (or rename the task/goal) so users get consistent command names.
| return leftovers; | ||
| } | ||
|
|
||
| TreeMap<String, String> inventory = loadInventory(); | ||
| List<InventoryEntry> removed = new ArrayList<>(); | ||
| for (InventoryEntry leftover : leftovers) { | ||
| try { | ||
| Files.deleteIfExists(Path.of(leftover.relativePath())); | ||
| inventory.remove(leftover.relativePath()); | ||
| removed.add(leftover); | ||
| } catch (IOException e) { | ||
| System.err.printf("Failed to delete leftover file: %s%n", leftover.relativePath()); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
removeLeftovers() returns an empty removed list both when there are no leftovers and when deletions fail for all leftovers. That makes it hard for the CLI/plugins to report accurate results (and currently leads to misleading output in the --remove command). Consider tracking deletion failures separately (or returning both leftovers+removed) so callers can distinguish “nothing to do” from “failed to delete”.
| return leftovers; | |
| } | |
| TreeMap<String, String> inventory = loadInventory(); | |
| List<InventoryEntry> removed = new ArrayList<>(); | |
| for (InventoryEntry leftover : leftovers) { | |
| try { | |
| Files.deleteIfExists(Path.of(leftover.relativePath())); | |
| inventory.remove(leftover.relativePath()); | |
| removed.add(leftover); | |
| } catch (IOException e) { | |
| System.err.printf("Failed to delete leftover file: %s%n", leftover.relativePath()); | |
| } | |
| } | |
| // Nothing to do: no leftovers were found. | |
| return leftovers; | |
| } | |
| TreeMap<String, String> inventory = loadInventory(); | |
| List<InventoryEntry> removed = new ArrayList<>(); | |
| List<InventoryEntry> failed = new ArrayList<>(); | |
| for (InventoryEntry leftover : leftovers) { | |
| Path path = Path.of(leftover.relativePath()); | |
| try { | |
| boolean deleted = Files.deleteIfExists(path); | |
| if (deleted) { | |
| inventory.remove(leftover.relativePath()); | |
| removed.add(leftover); | |
| } else { | |
| // File was not deleted (for example, it did not exist); treat as a failure so callers | |
| // and users can distinguish between "no leftovers" and "could not delete leftovers". | |
| failed.add(leftover); | |
| System.err.printf( | |
| "Failed to delete leftover file (file was not deleted): %s%n", | |
| leftover.relativePath()); | |
| } | |
| } catch (IOException e) { | |
| failed.add(leftover); | |
| System.err.printf("Failed to delete leftover file: %s (%s)%n", | |
| leftover.relativePath(), e.getMessage()); | |
| } | |
| } | |
| if (!failed.isEmpty()) { | |
| System.err.printf( | |
| "Failed to delete %d leftover approved file(s). First failed file: %s%n", | |
| failed.size(), failed.get(0).relativePath()); | |
| } |
| List<InventoryEntry> removed = removeLeftovers(); | ||
| if (removed.isEmpty()) { | ||
| System.out.println("No leftover approved files found."); | ||
| } else { | ||
| System.out.println("Removed leftover approved files:"); | ||
| removed.forEach( | ||
| leftover -> System.out.printf(" %s%n", Path.of(leftover.relativePath()).toUri())); |
There was a problem hiding this comment.
In the --remove command, printing "No leftover approved files found." based solely on removed.isEmpty() can be incorrect when leftovers exist but deletion fails (e.g., due to permissions) and removeLeftovers() returns an empty list. Consider basing this message on the pre-removal leftovers list and/or exiting non-zero when any deletion fails so build-tool plugins can surface the failure reliably.
| List<InventoryEntry> removed = removeLeftovers(); | |
| if (removed.isEmpty()) { | |
| System.out.println("No leftover approved files found."); | |
| } else { | |
| System.out.println("Removed leftover approved files:"); | |
| removed.forEach( | |
| leftover -> System.out.printf(" %s%n", Path.of(leftover.relativePath()).toUri())); | |
| // Determine leftovers before attempting removal so we can distinguish | |
| // "nothing to remove" from "failed to remove existing leftovers". | |
| List<InventoryEntry> beforeRemoval = findLeftovers(); | |
| if (beforeRemoval.isEmpty()) { | |
| System.out.println("No leftover approved files found."); | |
| return; | |
| } | |
| List<InventoryEntry> removed = removeLeftovers(); | |
| if (removed.isEmpty()) { | |
| System.err.println("Failed to remove leftover approved files:"); | |
| beforeRemoval.forEach( | |
| leftover -> | |
| System.err.printf( | |
| " %s%n from %s%n", | |
| Path.of(leftover.relativePath()).toUri(), leftover.testReference())); | |
| System.exit(1); | |
| } else { | |
| System.out.println("Removed leftover approved files:"); | |
| removed.forEach( | |
| leftover -> System.out.printf(" %s%n", Path.of(leftover.relativePath()).toUri())); | |
| // If we could not remove all leftovers, signal a partial failure. | |
| int remainingCount = beforeRemoval.size() - removed.size(); | |
| if (remainingCount > 0) { | |
| System.err.printf( | |
| "Failed to remove %d leftover approved file(s).%n", remainingCount); | |
| System.exit(1); | |
| } |
PathProvider and StackTraceTestFinderUtil now return normalized relative paths instead of absolute ones. This eliminates the unnecessary absolutize-then-relativize round-trip in ApprovedFileInventory. InventoryEntry uses Path instead of String for relativePath, giving proper path comparison semantics.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 37 out of 39 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Thread hook = new Thread(ApprovedFileInventory::writeInventory, "ApproveJ-Inventory-Writer"); | ||
| if (shutdownHook.compareAndSet(null, hook)) { | ||
| Runtime.getRuntime().addShutdownHook(hook); |
There was a problem hiding this comment.
A new Thread object is created on every call to registerApprovedFile(), but only the first thread is ever registered as a shutdown hook. For subsequent calls, the newly created Thread is immediately discarded. While this is functionally correct, it creates unnecessary object allocations on every approval during a test run.
Consider checking if the shutdown hook is already registered before creating the Thread object to avoid these allocations. For example: if (shutdownHook.get() == null) { Thread hook = ...; if (shutdownHook.compareAndSet(null, hook)) { ... } }
| Thread hook = new Thread(ApprovedFileInventory::writeInventory, "ApproveJ-Inventory-Writer"); | |
| if (shutdownHook.compareAndSet(null, hook)) { | |
| Runtime.getRuntime().addShutdownHook(hook); | |
| if (shutdownHook.get() == null) { | |
| Thread hook = | |
| new Thread(ApprovedFileInventory::writeInventory, "ApproveJ-Inventory-Writer"); | |
| if (shutdownHook.compareAndSet(null, hook)) { | |
| Runtime.getRuntime().addShutdownHook(hook); | |
| } |
| } | ||
|
|
||
| private static String parseMethodName(String testReference) { | ||
| return testReference.substring(testReference.indexOf('#') + 1); |
There was a problem hiding this comment.
The parseMethodName method doesn't validate that the method name is non-empty. If a test reference ends with '#' (e.g., "MyClass#"), this would return an empty string for the method name. While this is unlikely to occur in normal usage since test references are generated by the framework, it could happen if the inventory file is manually edited or corrupted.
Consider adding validation after line 28 to throw an IllegalArgumentException if the method name is empty, similar to how parseClassName validates the presence of the '#' character.
| return testReference.substring(testReference.indexOf('#') + 1); | |
| int hashIndex = testReference.indexOf('#'); | |
| if (hashIndex < 0 || hashIndex == testReference.length() - 1) { | |
| throw new IllegalArgumentException( | |
| "Invalid test reference (expected 'className#methodName'): %s".formatted(testReference)); | |
| } | |
| return testReference.substring(hashIndex + 1); |
|


Summary
byFile()calls are recorded in.approvej/inventory.propertiesorg.approvej) withapprovejFindLeftoversandapprovejCleanuptasksapprovej-maven-plugin) withfind-leftoversandcleanupgoalsBefore merging
GRADLE_PUBLISH_KEYsecret to the repo (Settings > Secrets)GRADLE_PUBLISH_SECRETsecret to the repo (Settings > Secrets)