This is an automated email from the ASF dual-hosted git repository. FreeAndNil pushed a commit to branch Feature/316-content-loss in repository https://gitbox.apache.org/repos/asf/logging-log4net.git
commit 3273fc3a96423eadc6394c31f6188753437f491a Author: Jan Friedrich <[email protected]> AuthorDate: Thu Sep 3 22:31:48 2026 +0200 stop the ANSI terminal appender dropping empty messages #316 - Logging an empty message threw IndexOutOfRangeException: the branch meant for a one-character message read message[0] without checking there was one. - AppenderSkeleton caught it, so the event simply disappeared. - The reset codes now go at one computed offset, leaving no short-message branch to get wrong. - All ten line-break cases are pinned by test. They carry explicit names because dotnet test --filter cannot see them otherwise; CLAUDE.md records why. audit da18b6fd-f029 --- CLAUDE.md | 6 ++ src/changelog/3.5.0/316-ansi-empty-render.xml | 13 +++ .../Appender/AnsiColorTerminalAppenderTest.cs | 104 +++++++++++++++++++++ src/log4net/Appender/AnsiColorTerminalAppender.cs | 51 +++++----- 4 files changed, 144 insertions(+), 30 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index bba164a2..0e7bcaa2 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -149,6 +149,12 @@ almost always be doing. the assertion is about control characters, use `Contains.Substring(x).Using(StringComparison.Ordinal)`, negated with the `!` operator that `Constraint` defines, or assert the whole value with `Is.EqualTo`, which is ordinal. +- **Give a `[TestCase]` an explicit `TestName` when an argument holds a control character.** + Otherwise the whole fixture can become invisible to `dotnet test --filter`, silently: it is + listed by `--list-tests` and runs in a full pass, but every filter reports "No test matches". + Reproduced with `[TestCase("one", "\x1b[0m")]`; a single argument holding the same escape is + fine, so it takes two arguments and an escape character. `AnsiColorTerminalAppenderTest` names + all ten of its cases for that reason, and a filtered run there is 54 ms against 9 s for the suite. - Mark a test `[NonParallelizable]` when it mutates static state (`LogLog.InternalDebugging`, a static field on a test double, a process-wide native registration). - Wrap expected internal logging in `LogLog.ExecuteWithoutEmittingInternalMessages(...)` and capture diff --git a/src/changelog/3.5.0/316-ansi-empty-render.xml b/src/changelog/3.5.0/316-ansi-empty-render.xml new file mode 100644 index 00000000..bf01d9ca --- /dev/null +++ b/src/changelog/3.5.0/316-ansi-empty-render.xml @@ -0,0 +1,13 @@ +<?xml version="1.0" encoding="UTF-8"?> +<entry xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"; + xmlns="https://logging.apache.org/xml/ns"; + xsi:schemaLocation="https://logging.apache.org/xml/ns https://logging.apache.org/xml/ns/log4j-changelog-0.xsd"; + type="fixed"> + <issue id="316" link="https://github.com/apache/logging-log4net/pull/316"/> + <description format="asciidoc"> + stop `AnsiColorTerminalAppender` dropping an event that renders to nothing. The branch meant + for a single character read the first one without checking there was one, so an empty render + threw and the event was lost. The reset codes are now placed by one computed offset, which has no + special case to get wrong (audit da18b6fd-f029) + </description> +</entry> diff --git a/src/log4net.Tests/Appender/AnsiColorTerminalAppenderTest.cs b/src/log4net.Tests/Appender/AnsiColorTerminalAppenderTest.cs new file mode 100644 index 00000000..222dce26 --- /dev/null +++ b/src/log4net.Tests/Appender/AnsiColorTerminalAppenderTest.cs @@ -0,0 +1,104 @@ +#region Apache License +// +// 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. +// +#endregion + +using System; +using System.IO; + +using log4net.Appender; +using log4net.Core; +using log4net.Layout; + +using NUnit.Framework; + +namespace log4net.Tests.Appender; + +/// <summary> +/// Tests for <see cref="AnsiColorTerminalAppender"/>, which places the terminal reset codes +/// before any trailing line break so the colour ends with the text. +/// </summary> +[TestFixture] +[NonParallelizable] +public class AnsiColorTerminalAppenderTest +{ + /// <summary>Matches the appender's private PostEventCodes.</summary> + private const string Reset = "\x1b[0m"; + + /// <summary>The reset codes belong before the line break, whichever one it is.</summary> + // Explicit names: two arguments where one holds an escape character make the whole fixture + // invisible to "dotnet test --filter", reproduced with [TestCase("one", "\x1b[0m")]. + [TestCase("", Reset, TestName = "AnEmptyRender")] + [TestCase("x", "x" + Reset, TestName = "ASingleCharacter")] + [TestCase("\n", Reset + "\n", TestName = "NothingButALineFeed")] + [TestCase("text", "text" + Reset, TestName = "NoTrailingLineBreak")] + [TestCase("\r", Reset + "\r", TestName = "NothingButACarriageReturn")] + [TestCase("text\n", "text" + Reset + "\n", TestName = "TrailingLineFeed")] + [TestCase("text\r", "text" + Reset + "\r", TestName = "TrailingCarriageReturn")] + [TestCase("text\r\n", "text" + Reset + "\r\n", TestName = "TrailingCarriageReturnLineFeed")] + [TestCase("text\n\r", "text" + Reset + "\n\r", TestName = "TrailingLineFeedCarriageReturn")] + [TestCase("text\n\n", "text\n" + Reset + "\n", TestName = "TrailingDoubledLineFeedCountsAsOne")] + public void TheResetCodesGoBeforeATrailingLineBreak(string message, string expected) + { + RecordingErrorHandler errorHandler = new(); + // Level.Info has no colour mapping configured, so nothing is prepended and the rendered + // message is exactly what was logged, down to the empty string. + AnsiColorTerminalAppender appender = new() + { + Layout = new PatternLayout("%message"), + ErrorHandler = errorHandler + }; + appender.ActivateOptions(); + + TextWriter previous = Console.Out; + using StringWriter captured = new(); + try + { + Console.SetOut(captured); + // DoAppend is overloaded on LoggingEvent and LoggingEvent[], so this new cannot be short. + appender.DoAppend(new LoggingEvent(new() + { + Level = Level.Info, + Message = message, + LoggerName = nameof(AnsiColorTerminalAppenderTest) + })); + } + finally + { + Console.SetOut(previous); + } + + Assert.That(errorHandler.Message, Is.Empty, "the event must not be dropped"); + Assert.That(captured.ToString(), Is.EqualTo(expected)); + } + + /// <summary>Collects what the appender reports, so a dropped event is visible.</summary> + private sealed class RecordingErrorHandler : IErrorHandler + { + /// <summary>Everything reported so far.</summary> + internal string Message { get; private set; } = string.Empty; + + /// <inheritdoc/> + public void Error(string message) => Message += message + '\n'; + + /// <inheritdoc/> + public void Error(string message, Exception e) => Message += message + '\n'; + + /// <inheritdoc/> + public void Error(string message, Exception? e, ErrorCode errorCode) => Message += message + '\n'; + } +} diff --git a/src/log4net/Appender/AnsiColorTerminalAppender.cs b/src/log4net/Appender/AnsiColorTerminalAppender.cs index b685c456..1c65d027 100644 --- a/src/log4net/Appender/AnsiColorTerminalAppender.cs +++ b/src/log4net/Appender/AnsiColorTerminalAppender.cs @@ -251,36 +251,10 @@ protected override void Append(LoggingEvent loggingEvent) loggingMessage = levelColors.CombinedColor + loggingMessage; } - // on most terminals there are weird effects if we don't clear the background color - // before the new line. This checks to see if it ends with a newline, and if - // so, inserts the clear codes before the newline, otherwise the clear codes - // are inserted afterward. - if (loggingMessage.Length > 1) - { - if (loggingMessage.EndsWith("\r\n") || loggingMessage.EndsWith("\n\r")) - { - loggingMessage = loggingMessage.Insert(loggingMessage.Length - 2, PostEventCodes); - } - else if (loggingMessage.EndsWith("\n") || loggingMessage.EndsWith("\r")) - { - loggingMessage = loggingMessage.Insert(loggingMessage.Length - 1, PostEventCodes); - } - else - { - loggingMessage += PostEventCodes; - } - } - else - { - if (loggingMessage[0] is '\n' or '\r') - { - loggingMessage = PostEventCodes + loggingMessage; - } - else - { - loggingMessage += PostEventCodes; - } - } + // On most terminals there are weird effects if the background colour is not cleared before + // the line break, so the reset codes go before it rather than after. + loggingMessage = loggingMessage.Insert( + loggingMessage.Length - TrailingLineBreakLength(loggingMessage), PostEventCodes); if (_writeToErrorStream) { @@ -295,6 +269,23 @@ protected override void Append(LoggingEvent loggingEvent) } + /// <summary> + /// How many characters of line break the message ends with, 0, 1 or 2. + /// </summary> + private static int TrailingLineBreakLength(string message) + { + int last = message.Length - 1; + if (last < 0 || message[last] is not '\n' and not '\r') + { + return 0; + } + + int previous = last - 1; + return previous >= 0 && message[previous] is '\n' or '\r' && message[previous] != message[last] + ? 2 + : 1; + } + /// <summary> /// This appender requires a <see cref="Layout"/> to be set. /// </summary>
