-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Update host test command execution logging: include PID on exit, remove confusing OK/FAIL, add caller name #116658
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR augments test command execution logging by passing the calling test method’s name into command invocations and updates exit-wait semantics in bundle tests. Key changes include:
- Added a
callerparameter (via[CallerMemberName]) toRunTestand related methods and propagated it intoExecute(caller). - Removed the old
expectedToFailargument fromWaitForExit, now relying on default behavior. - Cleaned up test-only behavior scoping by ignoring the disposable scope instead of disposing it.
Reviewed Changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| FrameworkResolution/RollForwardPreReleaseOnly.cs | Added caller param to RunTest overload and pass into call. |
| FrameworkResolution/RollForwardOnNoCandidateFxSettings.cs | Updated RunTest overload to accept caller and forward it. |
| FrameworkResolution/RollForwardOnNoCandidateFxMultipleFrameworks.cs | Added caller param to RunTest and propagated to base call. |
| FrameworkResolution/RollForwardOnNoCandidateFx.cs | Introduced caller param on RunTestWithOneFramework and others. |
| FrameworkResolution/RollForwardMultipleFrameworks.cs | Added caller to RunTest signature and forwarding. |
| FrameworkResolution/RollForwardAndRollForwardOnNoCandidateFxSettings.cs | Updated RunTest overload to include caller. |
| FrameworkResolution/MultipleHives.cs | Added caller param to RunTest overloads; removed disposal of test-only behavior scope. |
| FrameworkResolution/IncludedFrameworksSettings.cs | Introduced caller parameter on test helpers. |
| FrameworkResolution/FxVersionCLI.cs | Added caller param to RunTest helper. |
| FrameworkResolution/FrameworkResolutionBase.cs | Changed .Execute() to .Execute(caller) and added caller to methods. |
| FrameworkResolution/ComplexHierarchies.cs | Extended RunTest with caller and passed it to base. |
| FrameworkResolution/ApplyPatchesSettings.cs | Added [CallerMemberName] string caller to RunTest. |
| FrameworkDependentAppLaunch.cs | Updated .Execute() to .Execute(expectedToFail: true) and removed the boolean overload from WaitForExit. |
| DependencyResolution/RidAssetResolution.cs | Added caller param to abstract and override RunTest methods. |
| DependencyResolution/PerAssemblyVersionResolutionMultipleFrameworks.cs | Updated RunTest signatures to include caller. |
| DependencyResolution/PerAssemblyVersionResolution.cs | Added caller to abstract and override RunTest helpers. |
| DependencyResolution/ComponentSharedTestStateBase.cs | Extended RunComponentResolutionTest and multithreaded variants to accept caller. |
| AppHost.Bundle.Tests/BundleRename.cs | Changed WaitForExit(expectedToFail: false, ...) to WaitForExit(...). |
| AppHost.Bundle.Tests/AppLaunch.cs | Removed boolean arg from WaitForExit(true) to use default WaitForExit(). |
Comments suppressed due to low confidence (2)
src/installer/tests/HostActivation.Tests/FrameworkResolution/FxVersionCLI.cs:113
- [nitpick] The parameter name 'caller' is a bit ambiguous; consider renaming it to 'callerName' to clearly indicate it holds the calling test method's name.
private CommandResult RunTest(TestSettings testSettings, [CallerMemberName] string caller = "") =>
src/installer/tests/HostActivation.Tests/FrameworkResolution/MultipleHives.cs:277
- By discarding the IDisposable returned by TestOnlyProductBehavior.Enable, the test-only behavior remains enabled for the entire test run and may leak into other tests. Consider retaining and disposing the scope in Dispose to limit its duration.
// Enable test-only behaviour. We don't bother disabling the behaviour later,
| TestApp app, | ||
| TestSettings settings, | ||
| bool? multiLevelLookup = false) | ||
| bool? multiLevelLookup = false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove multiLevelLookup at this point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not entirely. For framework resolution, it can still be used for tfm < 7.0 - for example, component using roll-forward. We can probably pare it down a bit and there's definitely things I'd like to clean up here (but in some separate change).
When launching a process to test, host tests write to the console on launch, wait for exit, and exit. Currently, they also log OK/FAIL based on the exit code and a parameter for whether failure is expected. All tests already do their own validation for if the command should pass/fail - having this extra logging is not useful and can be actively confusing, since it relies on tests also passing in that parameter (even though that is not used for actual validation/asserts).
This PR updates the logging to remove the OK/FAIL, include the PID on exit, and add the caller name (for test identification).
Example of existing logging:
With this PR:
Contributes to #116520
cc @dotnet/appmodel @AaronRobinsonMSFT