Match maven-shared-archive-resources on the last path segment - #317
Match maven-shared-archive-resources on the last path segment#317kratos0718 wants to merge 3 commits into
Conversation
createArchiver() identified the shared archive resources directory with String.endsWith on the whole path, so any resource directory whose path merely ends with those characters matched too - for example /var/lib/tmp-maven-shared-archive-resources-extra would be treated as the shared directory and its contents added to the source archive. Compare the last path segment instead, via Path.getFileName(), and reuse the resolved Path for addDirectory rather than parsing the string twice. getFileName() returns null for a root path, so that case is guarded. Fixes apache#307
| * Name of the directory produced by maven-remote-resources-plugin whose contents are added to | ||
| * the source archive. Matched against the last path segment, not the whole path. | ||
| */ | ||
| private static final String SHARED_ARCHIVE_RESOURCES = "maven-shared-archive-resources"; |
There was a problem hiding this comment.
inline this as it's only used in one place
Used in only one place, so the constant did not earn its keep.
|
Inlined, thanks — the constant wasn't earning its keep at one usage. The diff is now +4/-2 in the single method.
|
createArchiver() adds a resource directory to the source archive only when that directory is the one produced by maven-remote-resources-plugin. Nothing covered which directories it selects, so neither the previous suffix match nor the last-segment match was pinned by a test. Drive createArchiver() with a stubbed project and a mock JarArchiver and assert the file sets it receives: the directory named maven-shared-archive-resources is added, a directory whose name merely ends with that text is not, and when both are present only the former is added. The latter two fail against the suffix match.
|
Both points addressed: the literal is inlined, and The tests drive |
elharo
left a comment
elharo
left a comment
There was a problem hiding this comment.
The mocks means you're not actually testing all that much, and tests afre pretty tuightly coupled to implementation. Consider using an integration test that uses real files and real objects.
| void testSharedArchiveResourcesDirectoryIsAdded(@TempDir Path tempDir) throws Exception { | ||
| Path sharedArchiveResources = Files.createDirectory(tempDir.resolve(SHARED_ARCHIVE_RESOURCES)); | ||
|
|
||
| JarArchiver jarArchiver = mock(JarArchiver.class); |
There was a problem hiding this comment.
can you use a real object instead of a mock?
2 participants
Fixes #307
Problem
AbstractSourceJarMojo.createArchiver()identified the shared archive resources directory by callingString.endsWithon the entire path:Because that tests the whole path string rather than the final segment, any resource directory whose path merely ends with those characters also matches — for example
/var/lib/tmp-maven-shared-archive-resources-extra, or a directory a user happens to namemy-maven-shared-archive-resources. Its contents would then be added to the source archive as though it were the directory produced by maven-remote-resources-plugin.Fix
Compare the last path segment instead, using
Path.getFileName(), and reuse the already-resolvedPathforaddDirectoryrather than parsing the same string a second time.getFileName()returnsnullfor a root path, so that case is guarded.Testing
Which directories
createArchiver()selects was not covered, soAbstractSourceJarMojoTestadds three cases. Each drivescreateArchiver()with a stubbed project and a mockJarArchiver, then asserts the file sets the archiver receives:maven-shared-archive-resourcesis addedThe last two fail against the previous suffix match and pass with the fix.
mvn verify -DskipITs: 13 tests, all passing.