Add retry support #33230

HaoK · 2021-06-02T20:37:21Z

See if retries help with flaky template tests

Part of #30882

HaoK · 2021-06-03T18:01:04Z

/azp run

azure-pipelines · 2021-06-03T18:01:22Z

Azure Pipelines successfully started running 2 pipeline(s).

HaoK · 2021-06-03T22:22:02Z

Failures seen so far: (5 runs)

BlazorWasmStandalonePwaTemplate_Works failed on Mac OS with 3 retries, 2 of them were CERT changed errors, one was 30 second timeout.

HaoK · 2021-06-03T22:22:43Z

/azp run

azure-pipelines · 2021-06-03T22:23:01Z

Azure Pipelines successfully started running 2 pipeline(s).

HaoK · 2021-06-03T22:24:34Z

Summary of this PR:

Adds [Retry] attribute which defaults to 3 retries, if any of the attempts succeed, the test is considered good
Marks all the blazor template tests with retry to see if this helps the pass rate in quarantine

HaoK · 2021-06-03T23:55:43Z

/azp run

azure-pipelines · 2021-06-03T23:55:59Z

Azure Pipelines successfully started running 2 pipeline(s).

src/Testing/src/xunit/AspNetTestInvoker.cs

dougbu

Works for me if this is what the team wants. My main concern is 3 retries may paper over problems getting twice as bad e.g. going from 1 failure in 3 tries to 2 failures in 3 tries without anyone noticing.

I'm also thinking the whole GetOr part of ProjectFactoryFixture.GetOrCreateProject(...) is a Bad Idea:tm:

dougbu · 2021-06-03T23:22:33Z

src/Testing/test/RetryTest.cs

+            if (_retryFailsUntil3 != 2) throw new Exception("NOOOOOOOO");
+        }
+
+        private static int _canOverrideRetries = 0;


nit: Move to the top of the class

I departed from the usual pattern here because these are doing a bad thing in that each test has its own static int, but I felt that was cleaner than adding some kind of internal context that we can look at since this is just test code. So the grouping is really a static field per test, it would be really bad if the tests mucked with another one of these fields.

src/ProjectTemplates/BlazorTemplates.Tests/BlazorServerTemplateTest.cs

src/ProjectTemplates/BlazorTemplates.Tests/BlazorWasmTemplateTest.cs

dougbu · 2021-06-03T23:55:07Z

src/ProjectTemplates/Shared/Project.cs

@@ -102,6 +102,21 @@ public class Project : IDisposable
            try
            {
                Output.WriteLine("Acquired DotNetNewLock");
+
+                if (Directory.Exists(TemplateOutputDir))


Isn't TemplateOutputDir already very unique❔ If Path.GetRandomFileName() isn't unique enough use Guid.NewGuid() in ProjectFactoryFixture.GetOrCreateProject(...). If instead this is new needed because the retries reuse existing projects, we may want to completely remove that aspect of the fixture i.e. switch to ProjectFactoryFixture.CreateProject(...).

I can file a separate issue to track changing that if you want, but I don't really want to do additional major surgery (prefer making this PR just about adding/using retries)

There's additional complications about switching from reusing projects since our longer term hope was to actually switch to one test creating/building the template project, and a second (ordered) test would be in charge of running/verifying the tests, we were hoping that would also reduce the flakiness, or at least help us isolate which part of the test is flaky. Unfortunately we didn't get to that point yet, but that was the longer term goal

src/ProjectTemplates/Shared/TemplatePackageInstaller.cs

dougbu · 2021-06-04T00:01:45Z

src/Testing/src/xunit/AspNetTestInvoker.cs

+        {
+            var attempts = 0;
+            var timeTaken = 0.0M;
+            for (attempts = 0; attempts < retryAttribute.MaxRetries; attempts++)


Please remind me: Will this loop retry an entire [Theory] even if only one data set hits a problem❔

This is a per test case/fact (at least that's the intent), I'm pretty certain of that, since there was a different extensibility point I was looking at in a different iteration of trying to implement this.

dougbu · 2021-06-04T00:07:44Z

@markwilkie @garath @ChadNedzlek @missymessa does this duplicate near-term work on Dev WF❔

src/Testing/src/xunit/AspNetTestInvoker.cs

HaoK · 2021-06-04T00:21:58Z

When the helix retry stuff is ready, we should definitely switch to just using the schema/rules as it is much more powerful, but this is basically a way for us to do a simple retry at an xunit level (this is akin to local reruns only)

ChadNedzlek · 2021-06-04T00:31:21Z

This is 100% duplicating work that dev WF should have a solution for in the next few weeks. I'd really rather we not invent a second mechanism for doing the same thing.

ChadNedzlek

Is there a reason you don't want to use the retry mechanism arcade is going to be providing in a couple weeks? It's very unfortunate to have all this duplicated work already.

HaoK · 2021-06-04T00:39:29Z

@ChadNedzlek this won't prevent us from using the arcade work at all, but this simple retry will work for our test jobs that aren't on helix, it wasn't clear to me if the test-configuration.json stuff is going to be helix only? If so, we might still benefit from retry for some of our tests that aren't on helix (components for example).

But most importantly, right now 100% of our blazor template tests are in quarantine, and its been that way for a few previews already, given that this is a pretty cheap fix, I think its worth temporarily using this to get some of our template tests back online (and switch to using the test-configuration.json stuff once its available)

discusses

ChadNedzlek · 2021-06-04T00:41:56Z

It seems like an offline discussion about your needs around retry stuff would be fruitful.

markwilkie · 2021-06-04T17:48:24Z

It seems like an offline discussion about your needs around retry stuff would be fruitful.

Yea, outside of this PR it'd be really great to understand the delta (if any) between this retry stuff and what's being implemented as part of dev wf. Sounds like rests run outside of Helix might be one, but it'd be awesome to understand that better. Is this something you think would be useful to do @HaoK ?

HaoK · 2021-06-05T03:05:22Z

@markwilkie @ChadNedzlek I don't think there's actually any gaps in the current retry plan, this PR is more of a point and time thing to try and get some blazor templates coverage back up as soon as possible, the dev wf plan currently proposed seems like a superset (on helix) and looks great. Our goal is to eventually move all our tests onto helix, and the xunit extensibility retry in this PR seems like it would map exactly onto using local reruns, so we could just turn this off once the dev wf retry stuff is available to try out.

markwilkie · 2021-06-07T15:03:37Z

@markwilkie @ChadNedzlek I don't think there's actually any gaps in the current retry plan, this PR is more of a point and time thing to try and get some blazor templates coverage back up as soon as possible, the dev wf plan currently proposed seems like a superset (on helix) and looks great. Our goal is to eventually move all our tests onto helix, and the xunit extensibility retry in this PR seems like it would map exactly onto using local reruns, so we could just turn this off once the dev wf retry stuff is available to try out.

Thanks @HaoK ! This makes sense. Let's be sure and continue to work together as we too would love to see more tests run in a consistent fashion across .NET. Cheers

src/ProjectTemplates/Shared/Project.cs

src/Testing/src/RetryAttribute.cs

pranavkm · 2021-06-07T22:54:18Z

src/Testing/src/RetryAttribute.cs

+    /// Runs a test multiple times when it fails
+    /// This can be used on an assembly, class, or method name. Requires using the AspNetCore test framework.
+    /// </summary>
+    [EditorBrowsable(EditorBrowsableState.Never)]


Is this because this ships in a user visible package? Making it Never makes it really hard to type this in regular code and gives some people PTSD (@rynowak).

I just copied

aspnetcore/src/Testing/src/RepeatAttribute.cs

Line 13 in 6cc7b9b

[EditorBrowsable(EditorBrowsableState.Never)]

as a starting point so it wasn't specifically intentional by me

Co-authored-by: Pranav K <[email protected]>

HaoK added 6 commits June 2, 2021 13:34

Add retry support

cc61999

Force create since retry will overwrite

61f740f

Delete files since retry will recreate

451bb7e

Only clean if dir exists

0dadff4

Update RetryTest.cs

a962382

Update AspNetTestInvoker.cs

a96c459

Pilchie added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates feature-templates labels Jun 3, 2021

HaoK marked this pull request as ready for review June 3, 2021 22:23

HaoK requested a review from Pilchie as a code owner June 3, 2021 22:23

HaoK requested review from a team June 3, 2021 22:23

JamesNK reviewed Jun 4, 2021

View reviewed changes

src/Testing/src/xunit/AspNetTestInvoker.cs Show resolved Hide resolved

dougbu reviewed Jun 4, 2021

View reviewed changes

JamesNK reviewed Jun 4, 2021

View reviewed changes

src/Testing/src/xunit/AspNetTestInvoker.cs Show resolved Hide resolved

JamesNK reviewed Jun 4, 2021

View reviewed changes

src/Testing/src/xunit/AspNetTestInvoker.cs Show resolved Hide resolved

ChadNedzlek previously requested changes Jun 4, 2021

View reviewed changes

HaoK added 2 commits June 3, 2021 17:49

PR feedback

0aff7ef

Revert change

6cc7b9b

pranavkm approved these changes Jun 7, 2021

View reviewed changes

HaoK and others added 2 commits June 7, 2021 16:26

Update src/Testing/src/RetryAttribute.cs

57a1750

Co-authored-by: Pranav K <[email protected]>

Use recursive delete

e45c0e5

HaoK enabled auto-merge (squash) June 7, 2021 23:29

Update Project.cs

ab97017

dougbu mentioned this pull request Jun 8, 2021

Flaky test: Template_Produces_The_Right_Set_Of_FilesAsync #32406

Closed

HaoK merged commit 49a1014 into main Jun 8, 2021

HaoK deleted the haok/retry2 branch June 8, 2021 07:36

ghost added this to the 6.0-preview6 milestone Jun 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add retry support #33230

Add retry support #33230

HaoK commented Jun 2, 2021

HaoK commented Jun 3, 2021

azure-pipelines bot commented Jun 3, 2021

HaoK commented Jun 3, 2021 •

edited

Loading

HaoK commented Jun 3, 2021

azure-pipelines bot commented Jun 3, 2021

HaoK commented Jun 3, 2021

HaoK commented Jun 3, 2021

azure-pipelines bot commented Jun 3, 2021

dougbu left a comment

dougbu Jun 3, 2021

HaoK Jun 4, 2021

dougbu Jun 3, 2021 •

edited

Loading

HaoK Jun 4, 2021

HaoK Jun 4, 2021

dougbu Jun 4, 2021

HaoK Jun 4, 2021

dougbu commented Jun 4, 2021

HaoK commented Jun 4, 2021

ChadNedzlek commented Jun 4, 2021

ChadNedzlek left a comment

HaoK commented Jun 4, 2021

ChadNedzlek commented Jun 4, 2021

markwilkie commented Jun 4, 2021

HaoK commented Jun 5, 2021

markwilkie commented Jun 7, 2021

pranavkm Jun 7, 2021

HaoK Jun 7, 2021

Add retry support #33230

Add retry support #33230

Conversation

HaoK commented Jun 2, 2021

HaoK commented Jun 3, 2021

azure-pipelines bot commented Jun 3, 2021

HaoK commented Jun 3, 2021 • edited Loading

HaoK commented Jun 3, 2021

azure-pipelines bot commented Jun 3, 2021

HaoK commented Jun 3, 2021

HaoK commented Jun 3, 2021

azure-pipelines bot commented Jun 3, 2021

dougbu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougbu Jun 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougbu commented Jun 4, 2021

HaoK commented Jun 4, 2021

ChadNedzlek commented Jun 4, 2021

ChadNedzlek left a comment

Choose a reason for hiding this comment

HaoK commented Jun 4, 2021

ChadNedzlek commented Jun 4, 2021

markwilkie commented Jun 4, 2021

HaoK commented Jun 5, 2021

markwilkie commented Jun 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HaoK commented Jun 3, 2021 •

edited

Loading

dougbu Jun 3, 2021 •

edited

Loading