Add regression coverage for metadata newline preservation by VolPlita · Pull Request #14261 · dotnet/msbuild

VolPlita · 2026-07-03T15:32:47Z

Context

This change strengthens regression coverage around newline handling in project file parsing.

Historically, switching from XmlTextReader to XmlReader.Create in read-only load paths caused newline/whitespace normalization changes that broke multiline metadata and command scenarios. This PR adds coverage and clarifies why the current reader choice is intentional for compatibility.

Changes Made

Added regression tests in src/Build.UnitTests/Construction/XmlReaderWithoutLocation_Tests.cs to verify multiline metadata with CRLF is preserved:
- when loading from disk
- when loading through read-only project loading (loadProjectsReadOnly: true)
Refactored duplicated test XML setup into a shared helper method.
Updated the compatibility comment in src/Build/Xml/XmlReaderExtension.cs to clearly document why XmlTextReader is used and linked related issues (#4210, #4213, #4083, #6232, #6669).

Testing

Ran XmlReaderWithoutLocation_Tests and validated the new regression tests pass.
Ran existing multiline Exec regression tests to ensure related behavior remains covered.

Notes

This PR does not change runtime parsing behavior; it improves documentation and regression protection for existing compatibility behavior.

Copilot

Pull request overview

This PR adds regression tests to ensure MSBuild’s XML loading path continues to preserve multi-line item metadata values (notably CRLF sequences), and updates an in-code compatibility comment explaining why MSBuild continues using XmlTextReader instead of XmlReader.Create to avoid whitespace/newline normalization behavior changes.

Changes:

Expanded the compatibility comment in XmlReaderExtension to document newline/whitespace preservation rationale and link related history.
Added Windows-only regression tests that load a project from disk (including “read-only” load) and assert CRLF preservation in item metadata.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
src/Build/Xml/XmlReaderExtension.cs	Updates compatibility commentary around XML reader choice to explain newline/whitespace behavior impact.
src/Build.UnitTests/Construction/XmlReaderWithoutLocation_Tests.cs	Adds regression tests validating CRLF preservation for item metadata when loading projects from disk and in read-only mode.

+            TransientTestFile projectFile = env.CreateFile("metadata-newlines.proj", GetMultilineMetadataProjectContents());
+            Project project = new Project(projectFile.Path);
+            string metadataValue = project.GetItems("I").Single().GetMetadataValue("M");


+            // Compatibility note: XmlReader.Create normalizes whitespace/newlines in ways that changed
+            // observed project values (for example, multiline Exec commands and metadata), breaking
+            // existing builds. We intentionally keep XmlTextReader behavior here to preserve those
+            // established semantics until a non-reflection, compatibility-safe replacement exists.
+            // Related history: #4210, #4213, #4083, #6232, #6669.


VolPlita · 2026-07-03T15:38:03Z

@dotnet-policy-service agree company="Microsoft"

github-actions

Caution

agentic threat detected
Threat detection flagged this output in warn mode. Manual review is REQUIRED before any follow-up automation.

Details

The threat detection results could not be parsed.

Review the workflow run logs for details.

Code Review — PR #14261

✅ 21/24 dimensions clean.

Dimension	Severity	Finding
Cross-Platform Correctness	MODERATE	`[WindowsOnlyFact]` is unneeded — `XmlTextReader` (`Normalization=false`) preserves `\r\n` on Linux too; use `[Fact]`
Design / Test Placement	MODERATE	These tests don't use the `XmlReaderNoIXmlLineInfo` wrapper that defines the class's scope; better placed in `ItemEvaluation_Tests.cs` or a new `XmlReaderExtension_Tests.cs`
Code Simplification	NIT	`Split([Environment.NewLine], ...).Length.ShouldBe(5)` is redundant — an exact `ShouldBe(...)` already guarantees the count

Design concern: test placement

XmlReaderWithoutLocation_Tests was built around one scenario: loading a project from an XmlReader that doesn't implement IXmlLineInfo. Neither new test uses the XmlReaderNoIXmlLineInfo wrapper, and loadProjectsReadOnly: true is orthogonal to "without location". Better homes would be src/Build.UnitTests/Evaluation/ItemEvaluation_Tests.cs (where metadata value semantics tests already live) or a new XmlReaderExtension_Tests.cs that explicitly targets the XmlReaderExtension behavior documented in the updated comment.

The comment update in XmlReaderExtension.cs is a clear improvement — it replaces the vague "Ignore loadAsReadOnly for now" placeholder with an accurate rationale and five cross-referenced issues. The tests themselves are correct and will catch real regressions; the findings above are structural improvements only.

Generated by Expert Code Review (on open) for #14261 · 1.8K AIC · ⊞ 30.3K

github-actions · 2026-07-03T15:59:17Z

+            metadataValue.Split([Environment.NewLine], StringSplitOptions.RemoveEmptyEntries).Length.ShouldBe(5);
+        }
+
+        [WindowsOnlyFact]


Same as above — [WindowsOnlyFact] is unnecessary. XmlTextReader's CRLF-preservation is platform-independent (controlled by Normalization=false, not by OS). Changing to [Fact] extends the regression guard to Linux at no cost.

github-actions · 2026-07-03T15:59:17Z

            Assert.Single(project.Targets);
        }
+
+        [WindowsOnlyFact]


[WindowsOnlyFact] is unnecessarily restrictive here. Tracing the implementation: XmlTextReader is constructed with no explicit Normalization setting, so _normalize = false, which causes _ps.eolNormalized = true in InitTextReaderInput — meaning EOL normalization is skipped on all platforms. File.WriteAllText (used by TransientTestFile) also writes the \r\n bytes verbatim on Linux. The assertion ShouldBe("multiple\r\nlines\r\nin\r\nthis\r\nmetadatum") would pass on Linux.

Consider changing to [Fact] to also guard this regression on Linux CI.

github-actions · 2026-07-03T15:59:17Z

+            string metadataValue = project.GetItems("I").Single().GetMetadataValue("M");
+
+            metadataValue.ShouldBe("multiple\r\nlines\r\nin\r\nthis\r\nmetadatum");
+            metadataValue.Split([Environment.NewLine], StringSplitOptions.RemoveEmptyEntries).Length.ShouldBe(5);


Same redundancy as the first test — the Split(...).Length.ShouldBe(5) is guaranteed to pass whenever the ShouldBe(...) above it passes. Consider removing.

github-actions · 2026-07-03T15:59:17Z

+            string metadataValue = project.GetItems("I").Single().GetMetadataValue("M");
+
+            metadataValue.ShouldBe("multiple\r\nlines\r\nin\r\nthis\r\nmetadatum");
+            metadataValue.Split([Environment.NewLine], StringSplitOptions.RemoveEmptyEntries).Length.ShouldBe(5);


This Split assertion is provably redundant. The preceding ShouldBe("multiple\r\nlines\r\nin\r\nthis\r\nmetadatum") is an exact character-by-character equality check — if it passes, the string is definitively that value and splitting by "\r\n" always yields exactly 5 parts. The second assertion can never independently fail and only adds noise on failure output. Consider removing it.

Add regression coverage for metadata newline preservation

3bdd057

Copilot AI review requested due to automatic review settings July 3, 2026 15:32

VolPlita temporarily deployed to copilot-pat-pool July 3, 2026 15:32 — with GitHub Actions Inactive

VolPlita temporarily deployed to copilot-pat-pool July 3, 2026 15:33 — with GitHub Actions Inactive

Copilot started reviewing on behalf of VolPlita July 3, 2026 15:33 View session

VolPlita temporarily deployed to copilot-pat-pool July 3, 2026 15:33 — with GitHub Actions Inactive

VolPlita linked an issue Jul 3, 2026 that may be closed by this pull request

Add regression test for: Newlines in metadata are \n instead of \r\n on Windows #4083

Open

Copilot AI reviewed Jul 3, 2026

View reviewed changes

VolPlita temporarily deployed to copilot-pat-pool July 3, 2026 15:55 — with GitHub Actions Inactive

VolPlita temporarily deployed to copilot-pat-pool July 3, 2026 15:59 — with GitHub Actions Inactive

github-actions Bot reviewed Jul 3, 2026

View reviewed changes

VolPlita requested a deployment to copilot-pat-pool July 3, 2026 15:59 — with GitHub Actions Abandoned

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add regression coverage for metadata newline preservation#14261

Add regression coverage for metadata newline preservation#14261
VolPlita wants to merge 1 commit into
dotnet:mainfrom
VolPlita:fix-4083-newline-regression-tests

VolPlita commented Jul 3, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

VolPlita commented Jul 3, 2026

Uh oh!

github-actions Bot left a comment

Uh oh!

github-actions Bot Jul 3, 2026

Uh oh!

github-actions Bot Jul 3, 2026

Uh oh!

github-actions Bot Jul 3, 2026

Uh oh!

github-actions Bot Jul 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

VolPlita commented Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Changes Made

Testing

Notes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

VolPlita commented Jul 3, 2026

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Code Review — PR #14261

Design concern: test placement

Uh oh!

github-actions Bot Jul 3, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jul 3, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jul 3, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jul 3, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

VolPlita commented Jul 3, 2026 •

edited

Loading