Fix TRX logger mangling astral-plane (non-BMP) characters

Merged
AZ
3mo ago
+162-18

Symptom

Every astral-plane (non-BMP) character written to a TRX file is corrupted into literal escape text. A test named party πŸŽ‰ done is recorded in the TRX as the literal text party \ud83c\udf89 done. The same happens to anything the emoji appears in: Console.WriteLine output (Output/StdOut, StdErr, DebugTrace), error messages and stack traces, TestMethod/@name and @className, computer name, attachment descriptions, owners and categories.

This hits emoji, rare CJK extensions, mathematical alphanumerics and several historic scripts. It affects the classic vstest path and the Microsoft Testing Platform path equally, because the bug is in the shared TRX writer, not in any adapter.

Root cause

XmlPersistence.RemoveInvalidXmlChar operates on UTF-16 code units:

// we are handling only #x9 | #xA | #xD | [#x20-#xD7FF] | [#xE000-#xFFFD]
// because C# support unicode character in range \^@ to \uFFFF
string invalidChar = @"[^\x09\x0A\x0D\x20-\uD7FF\uE000-\uFFFD]";
return Regex.Replace(str, invalidChar, evaluator);

The allowed set stops at \uD7FF and resumes at \uE000, so it skips U+D800–U+DFFF entirely β€” the surrogate range. Each half of a valid, well-formed surrogate pair is therefore independently classified as an invalid XML character and escaped. The comment is self-refuting: it quotes the XML spec including [#x10000-#x10FFFF] and then excludes the only mechanism by which UTF-16 can represent that range.

After β€” a surrogate-aware scan:

if (char.IsSurrogatePair(str, i)) { /* copy both code units through untouched */ i++; continue; }
if (IsValidXmlChar(str[i])) { /* copy through */ continue; }
/* otherwise escape as \uXXXX */

This follows the same control flow as the Jsonite JSON writer in testfx (JsonWriter.cs), which solves the identical problem: look ahead on a high surrogate, emit both code units if a low surrogate follows, treat it as invalid otherwise. char.IsSurrogatePair(str, i) does the lookahead-and-validate in one call and is safe at end-of-string, so a trailing high surrogate correctly falls through to the invalid branch without any out-of-range indexing. As a side benefit the StringBuilder is allocated lazily, so the overwhelmingly common all-valid string is now returned as-is with no allocation, replacing an unconditional Regex.Replace.

Lone surrogates are still escaped β€” deliberately

The fix is emphatically not "allow U+D800–U+DFFF". That would trade a string-mangling bug for a "vstest emits unparseable XML" bug, which is strictly worse β€” it fails at read time for every consumer of the TRX instead of degrading one string. A lone/unpaired surrogate is not a valid Unicode scalar value and is not valid XML β€” XmlWriter throws on it β€” so it must continue to be escaped. Only properly paired surrogates pass through. The comment in the source now spells this out, so it doesn't get "simplified" away later.

Covered cases: valid pair anywhere in the string survives byte-for-byte; lone high surrogate escaped; lone low surrogate escaped; high surrogate as the final character escaped without out-of-range indexing; low surrogate as the first character escaped. Behaviour for all other invalid characters (\0, \v, \x05, \uFFFE, …) and for all valid BMP characters is unchanged.

An existing test encoded the old, wrong behaviour

XmlPersistenceTests.SaveObjectShouldReplaceInvalidCharacter put (char)0xd800 and (char)0xdc00 adjacent in its "invalid characters" array and asserted the output contained \ud800\udc00. Those two chars form a valid surrogate pair for U+10000, which is legal XML, so the assertion was wrong. Rather than deleting the test, it has been split so both halves of the behaviour are pinned:

  • SaveObjectShouldReplaceInvalidCharacter keeps the genuinely invalid 0x5, 0xb, 0xf, 0xfffe, 0x0 with unchanged expectations, guarding against over-correcting.
  • SaveObjectShouldNotReplaceAdjacentHighAndLowSurrogate asserts that the exact (char)0xd800, (char)0xdc00 sequence now round-trips unescaped.
  • SaveObjectShouldReplaceLoneHighSurrogate asserts a lone (char)0xd800 still comes out as \ud800.

Further new tests: valid pair preserved through an attribute value and through element inner text; lone low surrogate escaped; high surrogate at end-of-string escaped; a string mixing a valid pair, a lone surrogate and other invalid characters handled correctly in one pass; BMP non-ASCII (Grüße ζ—₯本θͺž Čau) untouched; and a round-trip test asserting the produced XML still parses with XmlDocument.LoadXml with a real emoji present in both an attribute and element text.

Validation

Microsoft.TestPlatform.Extensions.TrxLogger.UnitTests β€” 164/164 passing in Release on both target frameworks (net481 and net11.0).

Precedent: the HTML logger in this repo already gets this right

Microsoft.TestPlatform.Extensions.HtmlLogger has its own copy of this sanitization and already handles surrogate pairs correctly (HtmlLogger.cs:44-46):

private static readonly Regex InvalidXmlCharsRegex = new(
    @"[^\x09\x0A\x0D\x20-\uD7FF\uE000-\uFFFD\uD800-\uDFFF]|[\uD800-\uDBFF](?![\uDC00-\uDFFF])|(?<![\uD800-\uDBFF])[\uDC00-\uDFFF]",
    RegexOptions.Compiled);

That pattern excludes the surrogate range from the "invalid" class and then matches only lone surrogates via lookahead/lookbehind β€” semantically identical to what this change does for the TRX writer, just expressed as a regex rather than a scan.

So the correct behaviour was already established in this codebase; the TRX writer simply never received it. That makes the current TRX behaviour an inconsistency rather than a deliberate design choice, and it means the two loggers now agree on how a test name containing an emoji is recorded.