-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Disable more SendPacketsElement tests on Win11 #59359
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
Disable more SendPacketsElement tests on Win11 #59359
Conversation
|
Tagging subscribers to this area: @dotnet/ncl Issue DetailsFollow up on #59203 (comment). High failure counts confirmed by checking Kusto.
|
| { | ||
| if (PlatformDetection.IsWindows10Version22000OrGreater) | ||
| { | ||
| // [ActiveIssue("https://github.com/dotnet/runtime/issues/58898")] |
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.
Isn't it better to have the [ActiveIssue] attribute and add the platform detection as a [ConditionalFact] attribute?
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.
Addressed in d26494f. The SkipTestException trick somehow became a repo-wide practice for skipping on Windows 11, also not big fan of it.
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.
At least for the SslStream I see this is temporary. For me this seems to have less overhead comparing to build new conditional variables. The other part is that we would see it in database and it is visible during test run e.g. serves as reminder as well.
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 other part is that we would see it in database and it is visible during test run e.g. serves as reminder as well.
This makes sense, though I prefer to merge the PR as-is for now. We need to deal with #58898 with high priority anyways.
|
/backport to release/6.0 |
Follow up on #59203 (comment).
High failure counts confirmed by checking Kusto.