Copilot commented on code in PR #2197:
URL: https://github.com/apache/maven-resolver/pull/2197#discussion_r4250738656
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultUpdateCheckManager.java:
##########
@@ -455,6 +463,25 @@ private void setUpdated(RepositorySystemSession session,
String updateKey) {
((Map<String, Boolean>) checkedFiles).put(updateKey, Boolean.TRUE);
}
+ private String getNotFoundKey(Path touchPath, String dataKey) {
+ return touchPath.toAbsolutePath() + "|" + dataKey;
+ }
+
+ /**
+ * Records not-found markers written during this session so that an
ordinary repository fall-through is not
+ * logged as a warning when a sibling repository later provides the
artifact.
+ */
+ @SuppressWarnings("unchecked")
+ private void setNotFound(RepositorySystemSession session, String
notFoundKey) {
+ Object notFounds =
session.getData().computeIfAbsent(SESSION_NOT_FOUNDS, () -> new
ConcurrentHashMap<>(64));
+ ((Map<String, Boolean>) notFounds).put(notFoundKey, Boolean.TRUE);
+ }
+
+ private boolean isNotFoundInSession(RepositorySystemSession session,
String notFoundKey) {
+ Object notFounds = session.getData().get(SESSION_NOT_FOUNDS);
Review Comment:
This uses a `Map<String, Boolean>` as a set, which forces unchecked casts
and adds unnecessary Boolean values. Consider storing a `Set<String>` instead
(e.g., a concurrent key set) to remove the casts/suppressions and make intent
clearer (`add` / `contains`). This also reduces the chance of accidental misuse
elsewhere and keeps the session data type more explicit.
##########
maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/DefaultUpdateCheckManagerTest.java:
##########
@@ -167,6 +186,44 @@ void
testTouchArtifactKeepsSiblingTransferErrorOnSuccessfulDownload() throws Exc
assertEquals("some error",
props.getProperty("file:///other-repo/.error"));
}
+ @Test
+ void testTouchArtifactDoesNotWarnAboutSiblingNotFoundFromSameSession()
throws Exception {
+ File touchFile = new File(artifact.getFile().getPath() +
".lastUpdated");
+
+ RemoteRepository other = new RemoteRepository.Builder("other",
"default", "file:///other-repo").build();
+ UpdateCheck<Artifact, ArtifactTransferException> notFound =
newArtifactCheck();
+ notFound.setRepository(other);
+ notFound.setAuthoritativeRepository(other);
+ notFound.setException(new ArtifactNotFoundException(artifact, other));
+ manager.touchArtifact(session, notFound);
+ assertTrue(touchFile.exists());
+
+ UpdateCheck<Artifact, ArtifactTransferException> success =
newArtifactCheck();
+ String log = captureStdErr(() -> manager.touchArtifact(session,
success));
+ assertFalse(log.contains("WARN"), log);
Review Comment:
Asserting on the literal substring `\"WARN\"` is brittle: it assumes a
particular logging backend/format (the test comment mentions slf4j-simple) and
could produce false positives/negatives if formatting changes or other warnings
are emitted to stderr during capture. A more robust assertion is to check for
presence/absence of the specific message text you expect (e.g., the warning
message fragment), and/or to capture the logger events via the active logging
backend rather than parsing stderr.
##########
maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/DefaultUpdateCheckManagerTest.java:
##########
@@ -98,6 +101,22 @@ void teardown() throws Exception {
static void resetSessionData(RepositorySystemSession session) {
session.getData().set(DefaultUpdateCheckManager.SESSION_CHECKS, null);
+ session.getData().set(DefaultUpdateCheckManager.SESSION_NOT_FOUNDS,
null);
+ }
+
+ /**
+ * Runs the action while capturing the slf4j-simple output written to
{@code System.err}.
+ */
+ private static String captureStdErr(Runnable action) throws Exception {
+ PrintStream original = System.err;
+ ByteArrayOutputStream buffer = new ByteArrayOutputStream();
+ try (PrintStream capture = new PrintStream(buffer, true,
StandardCharsets.UTF_8.name())) {
+ System.setErr(capture);
+ action.run();
+ } finally {
+ System.setErr(original);
+ }
+ return buffer.toString(StandardCharsets.UTF_8.name());
}
Review Comment:
Reassigning `System.err` is a JVM-global change and can make the test suite
flaky when tests run concurrently (within the same JVM) or when other code logs
to stderr during the capture window. Consider protecting these tests with a
JUnit 5 resource lock for `SYSTEM_ERR` (or otherwise forcing
same-thread/serialized execution for these test methods), or switch to a
logger-specific capture approach (e.g., appender-based capture for the
configured backend) to avoid global state.
--
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]
