gnodet-bot commented on code in PR #13860: URL: https://github.com/apache/maven/pull/13860#discussion_r4487949067
########## compat/maven-embedder/src/test/java/org/apache/maven/cli/MavenCliLoggingTest.java: ########## @@ -0,0 +1,353 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.maven.cli; + +import java.io.ByteArrayOutputStream; +import java.io.InputStream; +import java.io.OutputStream; +import java.io.PrintStream; +import java.lang.reflect.Field; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.FutureTask; +import java.util.concurrent.atomic.AtomicBoolean; + +import org.apache.maven.api.Constants; +import org.apache.maven.cling.logging.Slf4jConfiguration; +import org.apache.maven.cling.logging.Slf4jConfigurationFactory; +import org.apache.maven.jline.FastTerminal; +import org.apache.maven.jline.MessageUtils; +import org.jline.terminal.Terminal; +import org.jline.terminal.impl.DumbTerminal; +import org.jline.terminal.spi.TerminalExt; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; +import org.junit.jupiter.api.parallel.Isolated; +import org.slf4j.LoggerFactory; + +import static java.nio.charset.StandardCharsets.UTF_8; +import static java.util.concurrent.TimeUnit.SECONDS; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.Mockito.RETURNS_DEEP_STUBS; +import static org.mockito.Mockito.mock; + +@Isolated("Changes JVM-wide streams, system properties, terminal state, and logging configuration") +class MavenCliLoggingTest { + /** + * When color is explicitly forced ({@code -color=always}), {@code logging()} must NOT call + * {@code awaitTerminalInitialization()} before activating the logging configuration. The + * logger must therefore cache the stream that was active at activation time (the "early" + * stream), not the stream installed by the terminal consumer afterwards. + * <p> + * Note: on master the logger reconfiguration ({@code MavenSimpleLogger.configure()}) renders + * ANSI-coloured level labels via {@code JLineMessageBuilderFactory}, which calls + * {@code terminal.getType()} and therefore always awaits terminal initialisation. The test + * accounts for this by completing the terminal build before running {@code logging()} and + * then swapping {@code System.out} afterwards, verifying that the logger sticks to the + * stream it cached at activation time. + */ + @Test + void withColorForcedLoggerCachesEarlyStream() throws Exception { + PrintStream originalOut = System.out; + Terminal originalTerminal = getTerminalField(); + boolean originalColor = getColorEnabled(); + String cacheProperty = Constants.MAVEN_LOGGER_CACHE_OUTPUT_STREAM; + String fileProperty = Constants.MAVEN_LOGGER_LOG_FILE; + String originalCache = System.getProperty(cacheProperty); + String originalFile = System.getProperty(fileProperty); + ByteArrayOutputStream earlyOutput = new ByteArrayOutputStream(); + ByteArrayOutputStream lateOutput = new ByteArrayOutputStream(); + try (PrintStream earlyStream = new PrintStream(earlyOutput, true, UTF_8.name()); + PrintStream lateStream = new PrintStream(lateOutput, true, UTF_8.name()); + Terminal dumb = new DumbTerminal( + "Maven", + Terminal.TYPE_DUMB_COLOR, + InputStream.nullInputStream(), + OutputStream.nullOutputStream(), + null)) { + // Install a completed terminal so configure() can call getType() without blocking. + injectTerminal(dumb); + + // Set up logger to cache System.out at activate() time. + System.setProperty(cacheProperty, "true"); + System.setProperty(fileProperty, "System.out"); + System.setOut(earlyStream); + activateSlf4j(); + + // Run logging() with -color=always. It must NOT call awaitTerminalInitialization() + // before activate(), so it completes immediately (no blocking terminal). + MavenCli cli = new MavenCli(); + CliRequest request = new CliRequest(new String[] {"-Dstyle.color=always"}, null); + cli.cli(request); + cli.properties(request); + cli.logging(request); + + // Swap System.out AFTER logging() has cached earlyStream. + System.setOut(lateStream); + + // The logger must still write to earlyStream. + LoggerFactory.getLogger(MavenCliLoggingTest.class).warn("terminal-output-marker"); + assertTrue( + earlyOutput.toString(UTF_8.name()).contains("terminal-output-marker"), + "Logger must write to the stream that was active when logging() ran with -color=always"); + assertFalse( + lateOutput.toString(UTF_8.name()).contains("terminal-output-marker"), + "Logger must not switch to the stream installed after logging() completed"); + } finally { + System.setOut(originalOut); + injectTerminal(originalTerminal); + setColorEnabled(originalColor); + restoreProperty(cacheProperty, originalCache); + restoreProperty(fileProperty, originalFile); + activateSlf4j(); + } + } + + /** + * When {@code -l <logfile>} is used, {@code logging()} must call + * {@link MessageUtils#awaitTerminalInitialization()} before redirecting {@code System.out} + * to the log file. Without the await, the terminal's installed consumer (which may call + * {@code System.setOut}) could race with the log-file redirect, causing logs to leak to Review Comment: **Compile error — empty test body.** The method signature opens here but the body is blank (line 128 is empty), and then `private void cachesOutputAfterTerminalInstallation(String color)` starts at the class level. Java does not allow a method declaration inside a method body — this will not compile. From 3.10.x PR #13799, the fix is: ```suggestion void cachesOutputAfterTerminalInstallationWithColorDisabled() throws Exception { cachesOutputAfterTerminalInstallation("never"); } ``` Also add the missing batch-mode test: ```java @Test void cachesOutputAfterTerminalInstallationInBatchMode() throws Exception { cachesOutputAfterTerminalInstallation("batch"); } ``` -- 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]
