-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Add retries to some FSW tests #68368
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: @dotnet/area-system-io Issue Details
|
Since not one of these has failed on Mac although I see them running there (I guess we have a more robust approach on Mac) I could potentially disable the retry mechanism when on Mac. This is fine for now. |
src/libraries/System.IO.FileSystem.Watcher/tests/FileSystemWatcher.Directory.Changed.cs
Show resolved
Hide resolved
src/libraries/System.IO.FileSystem.Watcher/tests/FileSystemWatcher.Directory.Changed.cs
Show resolved
Hide resolved
src/libraries/System.Runtime.InteropServices.RuntimeInformation/tests/DescriptionNameTests.cs
Show resolved
Hide resolved
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.
The new retries look good, but I do think the RetryHelper.Execute method that logs the diagnostic should be exclusive to FileSystemWatcher, to avoid an unnecessary Contains check on all tests that are have the helix bool returning true.
@@ -22,6 +23,8 @@ public static partial class PlatformDetection | |||
// do it in a way that failures don't cascade. | |||
// | |||
|
|||
public static readonly bool IsInHelix = Environment.GetEnvironmentVariables().Keys.Cast<string>().Where(key => key.StartsWith("HELIX")).Any(); |
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.
This should be done lazily, similar to how other expensive properties in PlatformDetection are initialized lazily.
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.
I'll fix
Uh oh!
There was an error while loading. Please reload this page.