-
Notifications
You must be signed in to change notification settings - Fork 10.5k
Unquarantine tests #39659
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
Unquarantine tests #39659
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
discussed with @adityamandaleeka offline that we are ok with unquarantining these tests for good behavior |
| { | ||
| var content = new StringContent(new string('a', 100000)); | ||
| for (int i = 0; i < 500; i++) | ||
| for (int i = 0; i < 50; i++) |
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.
Why fewer iterations?
So I could also remove |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
dougbu
left a comment
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 see @adityamandaleeka was supportive of these actions but I'm not sure why -- both why we're not following the usual process and why these particular tests.
| public class RequestBodyTests | ||
| { | ||
| [ConditionalFact] | ||
| [QuarantinedTest("https://github.com/dotnet/aspnetcore/issues/27399")] |
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.
Why is this test an un-quarantine candidate❔ There's no record of a fix.
|
|
||
| [ConditionalFact] | ||
| [QuarantinedTest("https://github.com/dotnet/aspnetcore/issues/26294")] | ||
| [SkipNonHelix("This test takes 5 minutes to run")] |
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.
Running successfully 💯% of the time isn't sufficient reason to un-quarantine. Does the following apply (from the doc)❔
Find the set of tests that were supposedly fixed at least 30 days ago.
| } | ||
|
|
||
| [ConditionalFact] | ||
| [QuarantinedTest("https://github.com/dotnet/aspnetcore/issues/27400")] |
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 issue was also never marked as test-fixed
|
This wasn't following the normal unquarantine process, I chatted with Aditya and we made a feature team call that these tests were fine. If they end up failing again, they'd also be good candidates for the new helix retry. I just was looking at these test failures since I was on build ops anyways |
Based on #39659 (comment), adding ReaderThrowsResetExceptionOnInvalidBody to the helix retry list as it's been failing outside of quarantine
For good behavior:
RequestBody_SyncReadDisabledByDefault_WorksWhenEnabled

ReadAndWriteSynchronously (and shorten iteration count 500 -> 50)

ReaderThrowsResetExceptionOnInvalidBody
