Add regression coverage for metadata newline preservation - #14261
Conversation
There was a problem hiding this comment.
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
XmlReaderExtensionto 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. |
|
@dotnet-policy-service agree company="Microsoft" |
There was a problem hiding this comment.
Caution
agentic threat detected
Threat detection flagged this output in warn mode. Manual review is REQUIRED before any follow-up automation.
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
3bdd057 to
1b4c931
Compare
OvesN
left a comment
There was a problem hiding this comment.
Good expansion of our test coverage. I only have a few minor comments.
1f09485 to
d0f7e50
Compare
Fixes #4083
Context
This change strengthens regression coverage around newline handling in project file parsing.
Historically, switching from
XmlTextReadertoXmlReader.Createin 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
src/Build.UnitTests/Construction/XmlReaderWithoutLocation_Tests.csto verify multiline metadata with CRLF is preserved:loadProjectsReadOnly: true)src/Build/Xml/XmlReaderExtension.csto clearly document whyXmlTextReaderis used and linked related issues (#4210,#4213,#4083,#6232,#6669).Testing
XmlReaderWithoutLocation_Testsand validated the new regression tests pass.Execregression 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.