-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Fix FrameworkDependentAppLaunch.AppHost_GlobalLocation 'Failed to restore file' CI failure #116831
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 addresses an intermittent CI failure by increasing IO retry attempts, reorganizes and consolidates install-location tests, and removes redundant working-directory test cases.
- Increased the retry count in
TestFileBackup
from 5 to 25 to reduce flaky copy failures. - Refactored
InstallLocation
tests to use a sharedTestBehaviourEnabledApp
, added two new tests for default and registered install locations, and renamed one test for clarity. - Removed redundant working-directory variants and inlined
DotNetRoot
arguments inFrameworkDependentAppLaunch
tests.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
src/installer/tests/TestUtils/TestFileBackup.cs | Increased default IO retry attempts from 5 to 25 to mitigate intermittent copy errors. |
src/installer/tests/HostActivation.Tests/InstallLocation.cs | Refactored to reuse a single app instance with test behavior, added default/registered tests, and renamed a test for consistency. |
src/installer/tests/HostActivation.Tests/FrameworkDependentAppLaunch.cs | Removed working-directory tests and simplified DotNetRoot usage. |
Comments suppressed due to low confidence (1)
src/installer/tests/HostActivation.Tests/InstallLocation.cs:485
- The property name 'TestBehaviourEnabledApp' uses British spelling 'Behaviour' while the class 'TestOnlyProductBehavior' uses American 'Behavior'. Consider renaming to 'TestBehaviorEnabledApp' for consistency.
public TestApp TestBehaviourEnabledApp { get; }
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.
LGTM
We're hitting an intermittent failure (particularly on Debug) trying to copy over files immediately after the process exits. Since this passes the majority of the time, it does not seem to indicate an actual issue where the files are left in use.
InstallLocation
test class as it fits better there (testing registered and default install locations)InstallLocation
test class create one app with test behaviour enabled instead of repeatedly doing it in half of its tests cases.This also removes explicitly running the app with the app directory as the working directory, which doesn't seem interesting for us to keep testing.
Fixes #115851
cc @dotnet/appmodel @AaronRobinsonMSFT