Replace cryptic error messages with descriptive ones

[Evangelink]

Summary

Replace placeholder and near-meaningless error messages with descriptive ones that help developers diagnose issues.

Addresses Task 3 of #8065.

Changes

Three locations had cryptic or placeholder error messages:

1. JsonObjectSerializer.cs: "err" → descriptive message about uninitialized Properties delegate
2. RunConfigurationSettings.cs: "Invalid runsettings" → message showing the actual vs expected root element name
3. DotnetMuxerLocator.cs: "Invalid image" → message including the file path and possible cause

[Copilot]

Pull request overview

This PR improves diagnostic quality by replacing placeholder/cryptic exception messages with descriptive, actionable messages in MSTest and Microsoft.Testing.Platform components.

Changes:

• Updated a placeholder InvalidOperationException("err") to explain when the Properties delegate is uninitialized in JSON-RPC serialization.
• Improved runsettings parsing failure output to include the actual vs expected XML root element name.
• Improved dotnet muxer architecture detection failure output to include the PE image path and likely causes.

Show a summary per file

File	Description
src/Platform/Microsoft.Testing.Platform/ServerMode/JsonRpc/Json/JsonObjectSerializer.cs	Replaces a placeholder exception message with an actionable explanation for missing serializer configuration.
src/Adapter/MSTestAdapter.PlatformServices/RunConfigurationSettings.cs	Enhances runsettings root-element validation errors by showing actual vs expected XML root element.
src/Platform/Microsoft.Testing.Platform.MSBuild/Tasks/DotnetMuxerLocator.cs	Enhances architecture-detection failure message by including the path and suggesting likely causes.

Copilot's findings

• Files reviewed: 3/3 changed files
• Comments generated: 0

[Evangelink]

Summary

Workflow: Test Expert Reviewer 🧪
Date: 2026-05-11
Repository: microsoft/testfx

Key Findings

This PR replaces three cryptic error messages ("err", "Invalid runsettings", "Invalid image") with descriptive, context-rich diagnostics — a clear improvement. No test files were modified.

1. [Coverage — Minor] RunConfigurationSettings.ReadToRootNode: The FormatException path (invalid XML root element) has no test coverage. An existing test class (RunConfigurationSettingsTests.cs) already has the mock infrastructure to add one easily.

2. [Coverage — Minor] JsonObjectSerializer.GetProperties: The Properties == null guard path has no test coverage. A unit test verifying the InvalidOperationException is thrown would prevent this path from silently regressing.

3. DotnetMuxerLocator.TryGetDotnetPathByArchitecture: The null-coalescing throw is similarly untested, but given the PE-image inspection context, testing this path is inherently more complex (requires a crafted binary); flagging as low-priority.

Recommendations

• Consider adding two lightweight unit tests as described in the inline comments. Both error paths are now well-described and worth protecting.
• The message changes themselves look correct and helpful — no issues with the production code logic.

---

Generated by Test Expert Reviewer

🧪 Test quality reviewed by Test Expert Reviewer 🧪

[Evangelink]

Summary

Workflow: Expert Code Reviewer
Date: 2026-05-11
Repository: microsoft/testfx

Key Findings

No actionable issues found. All three changes are focused error message improvements on error paths only:

• JsonObjectSerializer.cs — "err" → descriptive message with nameof(Properties) and GetType().Name. The exception is only reachable if a subclass of JsonObjectSerializer leaves Properties uninitialized; the generic JsonObjectSerializer<T> always sets it in the constructor. No threading, performance, or API compat concerns.
• RunConfigurationSettings.cs — "Invalid runsettings" → message embedding reader.Name. reader.Name is the XML element name, developer-controlled, appropriate to surface in a diagnostic exception.
• DotnetMuxerLocator.cs — "Invalid image" → message including path. This is a private MSBuild task method; including the file path in an exception message is appropriate for build diagnostics and helps developers identify which binary failed parsing.

Recommendations

None required.

---

Generated by Expert Code Reviewer

🧠 Reviewed by Expert Code Reviewer 🧠

[Evangelink]

Summary

Workflow: PR Nitpick Reviewer 🔍
Date: 2026-05-11
Repository: microsoft/testfx

This is a small, focused PR (3 files, 3 lines changed) that makes a clear quality improvement: replacing terse or placeholder exception messages with descriptive, diagnostic ones. The intent and execution are both excellent.

Key Findings

All findings are Minor — the messages as written are already significantly better than what they replace. These are refinements, not blockers.

1. JsonObjectSerializer.cs — The second sentence of the new error message gives generic advice ("Ensure all serializer instances are fully configured before use") rather than pointing the developer directly to the fix (nameof(GetProperties)). See inline comment.

2. RunConfigurationSettings.cs — XML element names in the error message are wrapped in single quotes. Angle brackets (<RunSettings>, <{reader.Name}>) are the more conventional notation for XML element names and improve readability for developers familiar with the runsettings format. See inline comment.

3. DotnetMuxerLocator.cs — The phrase "unsupported format" is slightly ambiguous (could be confused with generic image files). "Unrecognized machine type" more precisely describes the actual failure path in the PE header parsing switch statement. See inline comment.

Positive Highlights

• Using nameof(Properties) in the JsonObjectSerializer message is great for refactoring safety ✅
• Including {reader.Name} in the RunConfigurationSettings message (showing the actual root element that was found) is very actionable for the developer ✅
• Including {path} in the DotnetMuxerLocator message so the problematic file is immediately visible is a solid improvement ✅

---

🔍 Meticulously inspected by PR Nitpick Reviewer

🔍 Meticulously inspected by PR Nitpick Reviewer 🔍

[Evangelink]

Summary

Workflow: Expert Code Reviewer
Date: 2026-05-11
Repository: microsoft/testfx

Key Findings

No actionable issues found. The new commit (36d2d26) correctly addresses all previous reviewer feedback:

• JsonObjectSerializer.cs — Error message now explicitly guides the developer to assign the Properties delegate. GetType().Name gives the concrete subclass name at runtime — correct and useful.
• RunConfigurationSettings.cs — Angle-bracket XML element notation in the error message is the right call for developer-facing XML diagnostics.
• DotnetMuxerLocator.cs — "machine type" is the accurate PE header term; the message is now precise and actionable.
• RunConfigurationSettingsTests.cs — New test correctly inherits TestContainer (public parameterless method, no [TestMethod] required), exercises the FormatException path, and asserts both expected message fragments against the actual error format. Compiles and runs correctly.
• JsonTests.cs — TestJsonObjectSerializer : JsonObjectSerializer (primary constructor alias, C# 12) correctly leaves Properties unset; StringAssert.Contains on nameof(Properties) and nameof(TestJsonObjectSerializer) precisely verifies the new message content. Consistent with existing MSTest assertion style in this project.

No threading, performance, API compatibility, cross-TFM, resource management, or security concerns.

Recommendations

None required.

---

Generated by Expert Code Reviewer

🧠 Reviewed by Expert Code Reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠

[Evangelink]

Summary

Workflow: Test Expert Reviewer 🧪
Date: 2026-05-11
Repository: microsoft/testfx

Key Findings

The previous review (run 25668197715) flagged two missing tests for the new error paths. Both have been added in this update, and both are well-implemented:

1. RunConfigurationSettingsTests.GetSettingsShouldThrowFormatExceptionWhenRootElementIsNotRunSettings — Correctly structured for the TestContainer-based test project (public parameterless method auto-discovered; no [TestMethod] needed). Uses FluentAssertions as required by this project. Asserts on both meaningful message substrings (<NotRunSettings> and <RunSettings>) — appropriately tests behavior without over-coupling to the full message string.

2. JsonTests.GetProperties_WhenPropertiesDelegateIsNotSet_ThrowsInvalidOperationException — Correctly uses [TestMethod] and MSTest asserts (Assert.ThrowsExactly, StringAssert.Contains) as required by Microsoft.Testing.Platform.UnitTests. Uses nameof for refactoring safety. Clear Arrange/Act/Assert structure. Private TestJsonObjectSerializer nested class is a clean, minimal setup.

Positive Highlights

• Both tests cover only what they claim, with no over-assertion on full message text ✅
• nameof usage in assertions makes tests refactoring-safe ✅
• No shared mutable state; both tests are fully isolated ✅
• Correct assertion libraries used per-project ✅

Recommendations

No issues found.

---

Generated by Test Expert Reviewer

🧪 Test quality reviewed by Test Expert Reviewer 🧪

🧪 Test quality reviewed by Test Expert Reviewer 🧪

[Copilot]

Copilot's findings

• Files reviewed: 5/5 changed files
• Comments generated: 3

[Evangelink]

Summary

Workflow: Test Expert Reviewer 🧪
Date: 2026-05-11
Repository: microsoft/testfx

Key Findings

The new commit (54debfc) that migrated from StringAssert.Contains to Assert.Contains introduced a broken assertion due to an argument-order mismatch:

• StringAssert.Contains(string value, string substring) — the searched string comes first
• Assert.Contains(string substring, string value) — the substring to search for comes first

The test now calls Assert.Contains(exception.Message, nameof(JsonObjectSerializer.Properties)), which checks whether the full ~100-character exception message is a substring of "Properties" (10 chars). This is always false, so the test will always fail in CI.

The test was correct in the previous commit. Only the two Assert.Contains arguments need to be swapped.

Recommendations

Swap the arguments on both lines:

// Before (broken):
Assert.Contains(exception.Message, nameof(JsonObjectSerializer.Properties));
Assert.Contains(exception.Message, nameof(TestJsonObjectSerializer));

// After (correct):
Assert.Contains(nameof(JsonObjectSerializer.Properties), exception.Message);
Assert.Contains(nameof(TestJsonObjectSerializer), exception.Message);

---

Generated by Test Expert Reviewer

🧪 Test quality reviewed by Test Expert Reviewer 🧪

🧪 Test quality reviewed by Test Expert Reviewer 🧪

[Evangelink]

Summary

Workflow: Expert Code Reviewer
Date: 2026-05-11
Repository: microsoft/testfx

Key Findings

The latest commit (54debfc) applied the MSTEST0046 fix (migrate StringAssert.Contains → Assert.Contains) but did not account for the parameter-order reversal that the migration requires.

[Correctness] — JsonTests.cs lines 148–149: Inverted Assert.Contains arguments

StringAssert.Contains(value, substring) and Assert.Contains(substring, value) have opposite argument orders. A mechanical find-and-replace without swapping the arguments produces a test that always fails:

// Current (broken): checks if "Properties" contains the full exception message
Assert.Contains(exception.Message, nameof(JsonObjectSerializer.Properties));

// Correct: checks if exception.Message contains "Properties"
Assert.Contains(nameof(JsonObjectSerializer.Properties), exception.Message);

Impact: Both assertions will always throw AssertFailedException, meaning the test can never pass and CI will be broken by this commit.

No Other Issues

The production-code changes (JsonObjectSerializer.cs, RunConfigurationSettings.cs, DotnetMuxerLocator.cs) are correct and unchanged from the previously-reviewed 36d2d26 commit. No threading, performance, API compatibility, cross-TFM, resource management, or security concerns.

Recommendations

Swap the argument order on both Assert.Contains calls:

Assert.Contains(nameof(JsonObjectSerializer.Properties), exception.Message);
Assert.Contains(nameof(TestJsonObjectSerializer), exception.Message);

---

Generated by Expert Code Reviewer

🧠 Reviewed by Expert Code Reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠

[Evangelink]

Summary

Workflow: Expert Code Reviewer 🧠
Date: 2026-05-11
Repository: microsoft/testfx

Key Findings

No actionable issues found. The latest commit (2aea8b2) correctly resolves both issues flagged in the previous expert review run (25685338793):

• Assert.Contains argument order — Inverted (exception.Message, nameof(...)) corrected to (nameof(...), exception.Message). Both assertions now correctly verify that exception.Message contains the expected substrings.
• [TestMethod] on RunConfigurationSettingsTests — Added to the new GetSettingsShouldThrowFormatExceptionWhenRootElementIsNotRunSettings method.

The three production-code changes remain correct and unchanged from commit 36d2d26:

• JsonObjectSerializer.cs — "err" → descriptive nameof-safe message with concrete runtime type via GetType().Name. Exception only reachable if a subclass leaves Properties uninitialized; JsonObjectSerializer<T> always sets it in the constructor.
• RunConfigurationSettings.cs — "Invalid runsettings" → message embedding reader.Name (an XML element name — developer-controlled, safe to surface). Angle-bracket notation is appropriate for XML diagnostics.
• DotnetMuxerLocator.cs — "Invalid image" → message including path and "machine type" (the correct PE header term). This is a private MSBuild task; the file path is appropriate in a build diagnostic.

No threading, performance, API compatibility, cross-TFM, resource management, or security concerns in any of the changes.

Recommendations

None required.

---

🧠 Reviewed by Expert Code Reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠

[Evangelink]

Summary

Workflow: Test Expert Reviewer 🧪
Date: 2026-05-11
Repository: microsoft/testfx

Key Findings

The two test additions are functionally solid. The Assert.Contains argument-order bug flagged in the previous review (run 25685341050, commit 54debfc) has been correctly fixed in the current head (2aea8b):

• Assert.Contains(nameof(JsonObjectSerializer.Properties), exception.Message) ✅ correct order
• Assert.Contains(nameof(TestJsonObjectSerializer), exception.Message) ✅ correct order

One minor structural inconsistency remains:

1. [Structure — Minor] RunConfigurationSettingsTests.GetSettingsShouldThrowFormatExceptionWhenRootElementIsNotRunSettings has [TestMethod], but the class inherits TestContainer which auto-discovers all public parameterless methods. It is the only method in the file with this attribute. It won't break anything, but it signals the wrong framework to readers.

Positive Highlights

• JsonTests.GetProperties_WhenPropertiesDelegateIsNotSet_ThrowsInvalidOperationException — correct use of Assert.ThrowsExactly, clear AAA structure, nameof for refactoring safety, minimal TestJsonObjectSerializer nested class ✅
• RunConfigurationSettingsTests test uses FluentAssertions correctly (.Should().Throw<FormatException>().Which) and asserts on both expected message substrings without over-coupling to the full string ✅
• Both tests are fully isolated (no shared mutable state) ✅

Recommendations

• Remove the [TestMethod] attribute on GetSettingsShouldThrowFormatExceptionWhenRootElementIsNotRunSettings to match the convention of every other method in RunConfigurationSettingsTests.

---

Generated by Test Expert Reviewer

🧪 Test quality reviewed by Test Expert Reviewer 🧪

🧪 Test quality reviewed by Test Expert Reviewer 🧪

[Copilot]

Copilot's findings

• Files reviewed: 5/5 changed files
• Comments generated: 1

[Evangelink]

Summary

Workflow: Test Expert Reviewer 🧪
Date: 2026-05-11
Repository: microsoft/testfx

Key Findings

All previously flagged issues have been resolved. The latest commit (4d66194) removed the [TestMethod] attribute that was incorrectly present on the TestContainer-based test in RunConfigurationSettingsTests.cs.

Both test additions are clean:

1. RunConfigurationSettingsTests.cs — public parameterless method (correct TestContainer discovery pattern), uses FluentAssertions, asserts both diagnostic substrings (<NotRunSettings>, <RunSettings>) are present in the message. Well-isolated, no shared state.

2. JsonTests.cs — has [TestMethod], uses Assert.ThrowsExactly<InvalidOperationException>, then Assert.Contains(substring, value) with correct argument order (substring first, per MSTest convention). Checks both the property name and the concrete serializer type appear in the message — precisely what GetType().Name and nameof(Properties) embed at runtime.

No isolation concerns, no flakiness patterns, no false positives.

---

Generated by Test Expert Reviewer 🧪

🧪 Test quality reviewed by Test Expert Reviewer 🧪

[Evangelink]

Summary

Workflow: Expert Code Reviewer 🧠
Date: 2026-05-11
Repository: microsoft/testfx

Key Findings

No actionable issues found. The latest commit (4d66194) correctly removes the unnecessary [TestMethod] attribute from the new GetSettingsShouldThrowFormatExceptionWhenRootElementIsNotRunSettings test method — RunConfigurationSettingsTests inherits TestContainer, so all public parameterless methods are auto-discovered as tests without needing [TestMethod].

All prior findings across the full PR history have been addressed:

• JsonObjectSerializer.cs — "err" → clear, actionable message naming the uninitialized Properties delegate, the concrete subclass, and the fix. ✅
• RunConfigurationSettings.cs — "Invalid runsettings" → message showing the actual vs expected root element name using reader.Name. ✅
• DotnetMuxerLocator.cs — "Invalid image" → message including the file path and PE-specific cause. ✅
• JsonTests.cs — Assert.Contains(nameof(...), exception.Message) argument order is correct. ✅
• RunConfigurationSettingsTests.cs — new error-path test is correct; [TestMethod] removed as expected for a TestContainer-derived class. ✅

Recommendations

None — the PR is clean and ready to merge.

---

Generated by Expert Code Reviewer 🧠

🧠 Reviewed by Expert Code Reviewer 🧠