Report raw invalid value for the remaining RunConfiguration bool settings - #16296
Closed
Jakub Jareš (nohwnd) wants to merge 2 commits into
Closed
Report raw invalid value for the remaining RunConfiguration bool settings#16296Jakub Jareš (nohwnd) wants to merge 2 commits into
Jakub Jareš (nohwnd) wants to merge 2 commits into
Conversation
…ings More follow-ups from the review on microsoft#16271. DisableSharedTestHost and SkipDefaultAdapters had the same bug as IsTargetPlatformInferred: the SettingsException reported the parsed bool (always False) instead of the value the user wrote. Both report the raw text now, and each has a test that locks in the message. Also fix the forced-x64 verbose trace in DotnetTestHostManager. It still called the flag IsDefaultTargetArchitecture and misspelled architecture as "architecure". The value comes from IsTargetPlatformInferred now, so it says that instead. 🤖
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves diagnostic clarity for RunConfiguration boolean parsing by ensuring invalid XML values are reported as the raw text provided by the user (instead of the parsed bool default), and aligns a related DotnetTestHostManager verbose trace message with the newer IsTargetPlatformInferred naming.
Changes:
- Update
RunConfiguration.FromXmlto report the raw invalid value forDisableSharedTestHostandSkipDefaultAdapters. - Add unit tests asserting the full
SettingsExceptionmessage for invalidDisableSharedTestHost/SkipDefaultAdaptersvalues. - Fix the forced-x64 verbose trace message wording in
DotnetTestHostManager(flag name + “architecture” spelling).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test/Microsoft.TestPlatform.ObjectModel.UnitTests/RunSettings/RunConfigurationTests.cs | Adds message-asserting tests for invalid DisableSharedTestHost / SkipDefaultAdapters values. |
| src/Microsoft.TestPlatform.TestHostProvider/Hosting/DotnetTestHostManager.cs | Updates verbose trace to reference IsTargetPlatformInferred and fixes “architecture” spelling in that message. |
| src/Microsoft.TestPlatform.ObjectModel/RunSettings/RunConfiguration.cs | Changes invalid-bool exception formatting to include raw XML values for two RunConfiguration bool settings. |
When <IsTargetPlatformInferred> holds a non-boolean value, the parse failure reported the parsed bool default (False) instead of the text the user actually wrote, so the error was misleading. Pass the raw element value to the SettingsException, matching the fix already applied to DisableSharedTestHost and SkipDefaultAdapters. Strengthen the invalid-value test to assert the full exception message, consistent with the sibling tests. 🤖
Comment on lines
1024
to
1034
| case nameof(DisableSharedTestHost): | ||
| { | ||
| XmlRunSettingsUtilities.ThrowOnHasAttributes(reader); | ||
| string element = reader.ReadElementContentAsString(); | ||
|
|
||
| bool boolValue; | ||
| if (!bool.TryParse(element, out boolValue)) | ||
| { | ||
| throw new SettingsException(string.Format(CultureInfo.CurrentCulture, | ||
| Resources.Resources.InvalidSettingsIncorrectValue, Constants.RunConfigurationSettingsName, boolValue, elementName)); | ||
| Resources.Resources.InvalidSettingsIncorrectValue, Constants.RunConfigurationSettingsName, element, elementName)); | ||
| } |
| if (forceToX64) | ||
| { | ||
| EqtTrace.Verbose($"DotnetTestHostmanager: Forcing the search to x64 architecure, IsDefaultTargetArchitecture '{_isDefaultTargetArchitecture}' OS '{_platformEnvironment.OperatingSystem}' framework '{_targetFramework}'"); | ||
| EqtTrace.Verbose($"DotnetTestHostmanager: Forcing the search to x64 architecture, IsTargetPlatformInferred '{_isDefaultTargetArchitecture}' OS '{_platformEnvironment.OperatingSystem}' framework '{_targetFramework}'"); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
More follow-ups from the review on #16271, split out from #16295 so each PR stays focused.
Two RunConfiguration bool settings had the same bug #16295 fixes for
IsTargetPlatformInferred: an invalid<DisableSharedTestHost>or<SkipDefaultAdapters>value threw aSettingsExceptionthat reported the parsed bool (alwaysFalse) instead of the text the user actually wrote. Both report the raw value now, matchingCreateNoNewWindow, and each has a test that asserts the full message.Also fixed the forced-x64 verbose trace in
DotnetTestHostManager: it still called the flagIsDefaultTargetArchitectureand misspelledarchitectureasarchitecure. The value comes fromIsTargetPlatformInferrednow, so the trace says that.Verified locally in Release: ObjectModel.UnitTests (210, includes the two new ones) and TestHostProvider.UnitTests (114) pass.
🤖