slachiewicz commented on code in PR #1677:
URL:
https://github.com/apache/maven-dependency-plugin/pull/1677#discussion_r3745674217
##########
src/test/java/org/apache/maven/plugins/dependency/TestGetMojo.java:
##########
@@ -119,13 +164,10 @@ void testTransitive(GetMojo mojo) throws Exception {
@InjectMojo(goal = "get")
@MojoParameter(
name = "remoteRepositories",
- value =
-
"central::default::https://repo.maven.apache.org/maven2,central::::https://repo.maven.apache.org/maven2,https://repo.maven.apache.org/maven2";)
+ value = "central::default::https://repo.maven.apache.org/maven2,";
+ + "central::::https://repo.maven.apache.org/maven2,";
+ + "https://repo.maven.apache.org/maven2";)
Review Comment:
Fair — the test did rely on an implicit argument (a single element
containing commas would split on `::` into five tokens and throw), which is not
obvious to a reader. Fixed both ways you suggested, minus reverting the type:
* `testRemoteRepositories` now asserts the injected field itself, so the
binding is checked directly rather than inferred:
```java
List<String> repositories = getVariableValueFromObject(mojo,
"remoteRepositories");
assertEquals(
Arrays.asList(
"central::default::https://repo.maven.apache.org/maven2";,
"central::::https://repo.maven.apache.org/maven2";,
"https://repo.maven.apache.org/maven2";),
repositories);
```
That confirms the harness splits on commas, so the surrounding test was
meaningful — but it only proves it for the harness.
* For production behaviour there are now two integration tests through real
Maven, one per binding path, described in the sibling thread. That is the
coverage for comma-separated input you asked for.
Unit suite 422, ITs 101, all green.
##########
src/test/java/org/apache/maven/plugins/dependency/TestGetMojo.java:
##########
@@ -218,20 +285,8 @@ void testRemoteRepositoriesNonProxyHosts(GetMojo mojo)
throws Exception {
}
}
- /**
- * Points the mojo at an empty local repository, so that the tests above
depend on the transfer actually
- * happening rather than on what an earlier run left behind in the shared
one.
- */
- private void useIsolatedLocalRepository() {
- DefaultRepositorySystemSession repositorySession =
- new
DefaultRepositorySystemSession(session.getRepositorySession());
-
repositorySession.setLocalRepositoryManager(repositorySystem.newLocalRepositoryManager(
- repositorySession, new
LocalRepository(isolatedLocalRepository.toFile())));
- when(session.getRepositorySession()).thenReturn(repositorySession);
-
- DefaultProjectBuildingRequest pbr = new
DefaultProjectBuildingRequest();
- pbr.setRepositorySession(repositorySession);
- when(session.getProjectBuildingRequest()).thenReturn(pbr);
+ private void setRemoteRepositories(GetMojo mojo, String... repositories)
throws Exception {
+ setVariableValueToObject(mojo, "remoteRepositories",
Arrays.asList(repositories));
}
Review Comment:
Fair — the test did rely on an implicit argument (a single element
containing commas would split on `::` into five tokens and throw), which is not
obvious to a reader. Fixed both ways you suggested, minus reverting the type:
* `testRemoteRepositories` now asserts the injected field itself, so the
binding is checked directly rather than inferred:
```java
List<String> repositories = getVariableValueFromObject(mojo,
"remoteRepositories");
assertEquals(
Arrays.asList(
"central::default::https://repo.maven.apache.org/maven2";,
"central::::https://repo.maven.apache.org/maven2";,
"https://repo.maven.apache.org/maven2";),
repositories);
```
That confirms the harness splits on commas, so the surrounding test was
meaningful — but it only proves it for the harness.
* For production behaviour there are now two integration tests through real
Maven, one per binding path, described in the sibling thread. That is the
coverage for comma-separated input you asked for.
Unit suite 422, ITs 101, all green.
--
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]
