gnodet-bot commented on code in PR #2186:
URL: https://github.com/apache/maven-resolver/pull/2186#discussion_r4219701536
##########
maven-resolver-api/src/test/java/org/eclipse/aether/RepositoryExceptionTest.java:
##########
@@ -111,6 +114,39 @@ void testArtifactResolutionException_Serializable() {
assertSerializable(new
ArtifactResolutionException(Arrays.asList(result)));
}
+ @Test
+ void
testArtifactResolutionExceptionMessageIncludesFailuresFromAllRepositories() {
+ RemoteRepository central =
+ new RemoteRepository.Builder("central", "default",
"https://repo.maven.apache.org/maven2";).build();
+ RemoteRepository corporate = new RemoteRepository.Builder(
+ "corporate", "default",
"https://repo.internal.example/repository/maven";)
+ .build();
+ Artifact artifact = newArtifact();
+ ArtifactRequest request = new ArtifactRequest()
+ .setArtifact(artifact)
+ .addRepository(central)
+ .addRepository(corporate);
+ ArtifactResult result = new ArtifactResult(request);
+ result.addException(central, new ArtifactNotFoundException(artifact,
central, "not found in central"));
+ result.addException(corporate, new ArtifactTransferException(artifact,
corporate, "DNS lookup failed"));
+
+ String message = new
ArtifactResolutionException(Collections.singletonList(result)).getMessage();
+
+ assertTrue(message.contains("not found in central"), message);
+ assertTrue(message.contains("DNS lookup failed"), message);
Review Comment:
💡 **Suggestion:** `assertTrue(message.contains(...))` doesn't verify the
full message format or confirm that the primary cause isn't duplicated in the
additional-failures section. Consider asserting the exact message string
instead — it documents the contract precisely and will catch unintended
formatting changes:
```suggestion
assertTrue(message.contains("not found in central"), message);
assertTrue(message.contains("DNS lookup failed"), message);
// Stronger: verify exact format and no duplication
// assertEquals("The following artifacts could not be resolved:
gid:aid:ext:1: not found in central " +
// "(additional failures: gid:aid:ext:1 from corporate: DNS
lookup failed)", message);
```
Using `assertEquals` on the full expected string makes the test
self-documenting and acts as a stronger non-duplication guard.
##########
maven-resolver-api/src/main/java/org/eclipse/aether/resolution/ArtifactResolutionException.java:
##########
@@ -133,9 +133,37 @@ private static String getSmartMessage(List<? extends
ArtifactResult> results) {
buffer.append(": ").append(cause.getMessage());
}
+ String additionalFailures = getAdditionalFailures(results, cause);
+ if (!additionalFailures.isEmpty()) {
+ buffer.append(" (additional failures:
").append(additionalFailures).append(")");
+ }
+
return buffer.toString();
}
+ private static String getAdditionalFailures(List<? extends ArtifactResult>
results, Throwable primaryCause) {
+ ArrayList<String> failures = new ArrayList<>();
Review Comment:
💡 **Nit:** Prefer `List<String>` over `ArrayList<String>` — program to the
interface rather than the implementation. Since `Collections.sort` accepts any
`List`, this works as-is:
```suggestion
List<String> failures = new ArrayList<>();
```
##########
maven-resolver-api/src/main/java/org/eclipse/aether/resolution/ArtifactResolutionException.java:
##########
@@ -133,9 +133,37 @@ private static String getSmartMessage(List<? extends
ArtifactResult> results) {
buffer.append(": ").append(cause.getMessage());
}
+ String additionalFailures = getAdditionalFailures(results, cause);
+ if (!additionalFailures.isEmpty()) {
+ buffer.append(" (additional failures:
").append(additionalFailures).append(")");
+ }
+
return buffer.toString();
}
+ private static String getAdditionalFailures(List<? extends ArtifactResult>
results, Throwable primaryCause) {
+ ArrayList<String> failures = new ArrayList<>();
Review Comment:
💡 **Nit:** Prefer `List<String>` over `ArrayList<String>` — program to the
interface rather than the implementation. Since `Collections.sort` accepts any
`List`, this works as-is:
```suggestion
List<String> failures = new ArrayList<>();
```
##########
maven-resolver-api/src/test/java/org/eclipse/aether/RepositoryExceptionTest.java:
##########
@@ -111,6 +114,39 @@ void testArtifactResolutionException_Serializable() {
assertSerializable(new
ArtifactResolutionException(Arrays.asList(result)));
}
+ @Test
+ void
testArtifactResolutionExceptionMessageIncludesFailuresFromAllRepositories() {
+ RemoteRepository central =
+ new RemoteRepository.Builder("central", "default",
"https://repo.maven.apache.org/maven2";).build();
+ RemoteRepository corporate = new RemoteRepository.Builder(
+ "corporate", "default",
"https://repo.internal.example/repository/maven";)
+ .build();
+ Artifact artifact = newArtifact();
+ ArtifactRequest request = new ArtifactRequest()
+ .setArtifact(artifact)
+ .addRepository(central)
+ .addRepository(corporate);
+ ArtifactResult result = new ArtifactResult(request);
+ result.addException(central, new ArtifactNotFoundException(artifact,
central, "not found in central"));
+ result.addException(corporate, new ArtifactTransferException(artifact,
corporate, "DNS lookup failed"));
+
+ String message = new
ArtifactResolutionException(Collections.singletonList(result)).getMessage();
+
+ assertTrue(message.contains("not found in central"), message);
+ assertTrue(message.contains("DNS lookup failed"), message);
Review Comment:
💡 **Suggestion:** `assertTrue(message.contains(...))` doesn't verify the
full message format or confirm that the primary cause isn't duplicated in the
additional-failures section. Consider asserting the exact message string
instead — it documents the contract precisely and will catch unintended
formatting changes:
```suggestion
assertTrue(message.contains("not found in central"), message);
assertTrue(message.contains("DNS lookup failed"), message);
// Stronger: verify exact format and no duplication
// assertEquals("The following artifacts could not be resolved:
gid:aid:ext:1: not found in central " +
// "(additional failures: gid:aid:ext:1 from corporate: DNS
lookup failed)", message);
```
Using `assertEquals` on the full expected string makes the test
self-documenting and acts as a stronger non-duplication guard.
--
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]
