Skip to content

add a 'Run Flaky Tests' step that does not fail the build #1239

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

Merged
merged 4 commits into from
Mar 13, 2019

Conversation

analogrelay
Copy link

@analogrelay analogrelay commented Mar 12, 2019

This adds a step to all the builds, immediately following the main build step, which runs all the tests marked "Flaky". If this step fails, it does not fail the build. However, the TRX files and logs should be collected and published, so the Tests view should show the failing flaky tests.

Once this is working, I can fairly easily port it to EF which is using Arcade. AspNetCore will need something a little different.

Closes #1224

@analogrelay
Copy link
Author

Let's see how/if this works :)

Copy link
Member

@dougbu dougbu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good as long as the validation checks are passing 😺

<!-- Azure Pipelines Flakiness -->
<PropertyGroup Condition="'$(AGENT_OS)' != ''">
<_FlakyRunAdditionalArgs>$(_FlakyRunAdditionalArgs) -trait "Flaky:AzP:All=true" -trait "Flaky:AzP:OS:$(AGENT_OS)=true"</_FlakyRunAdditionalArgs>
<_NonFlakyRunAdditionalArgs>$(_NonFlakyRunAdditionalArgs) -notrait "Flaky:AzP:All=true" -notrait "Flaky:AzP:OS:$(AGENT_OS)=true"</_NonFlakyRunAdditionalArgs>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't -trait "Flaky:AzP:All=true" and -notrait "Flaky:AzP:All=true" unnecessary in these two lines?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not quite. The logic isn't as simple as just setting the OS-based flag. The Flaky attribute is trying to be a little agnostic as to the "filters" in use (since we may add other pivots like architectures, etc.), so if you say it's flaky on Darwin, like so:

[Flaky("...", AzurePipelines.macOS)]

Then we set the xUnit trait: Flaky:AzP:OS:Darwin=true

If you say it's flaky everywhere, like so:

[Flaky("...", AzurePipelines.All)]

Then we set a different xUnit trait: Flaky:AzP:All=true since we don't want the Flaky attribute logic to have to enumerate every possible pivot and filter.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did it this way so that the complexity could live in the build script rather than having to keep updating the Flaky attribute whenever we add new pivots or new pivot pieces. If we wanted to simplify this logic, we'd need Flaky to inject traits representing all possible pivots for which the test is flaky which makes things like AzurePipelines.All tricky.

@analogrelay
Copy link
Author

Looks like it's somewhat working. The main AzP summary page does what I'd hoped:

image

The tests page shows all tests passing, I need to check the scripts that collect test results.

@analogrelay
Copy link
Author

Ahh, they didn't run at all:

The application to execute does not exist: '/home/vsts/.nuget/packages/xunit.runner.console/2.4.1/tools/netcoreapp2.0/xunit.console.dll'
=== COMMAND LINE ===
"/home/vsts/work/1/s/.dotnet/dotnet" exec --depsfile "/home/vsts/work/1/s/artifacts/bin/Microsoft.Extensions.Configuration.Tests/Debug/netcoreapp3.0/Microsoft.Extensions.Configuration.Tests.deps.json" --runtimeconfig "/home/vsts/work/1/s/artifacts/bin/Microsoft.Extensions.Configuration.Tests/Debug/netcoreapp3.0/Microsoft.Extensions.Configuration.Tests.runtimeconfig.json"  "/home/vsts/.nuget/packages/xunit.runner.console/2.4.1/tools/netcoreapp2.0/xunit.console.dll" "/home/vsts/work/1/s/artifacts/bin/Microsoft.Extensions.Configuration.Tests/Debug/netcoreapp3.0/Microsoft.Extensions.Configuration.Tests.dll" -noautoreporters -xml "/home/vsts/work/1/s/artifacts/TestResults/Debug/Flaky/Microsoft.Extensions.Configuration.Tests_netcoreapp3.0_x64.xml" -html "/home/vsts/work/1/s/artifacts/TestResults/Debug/Flaky/Microsoft.Extensions.Configuration.Tests_netcoreapp3.0_x64.html" -trait "Flaky:All=true" -trait "Flaky:AzP:All=true" -trait "Flaky:AzP:OS:Linux=true" > "/home/vsts/work/1/s/artifacts/log/Debug/Flaky/Microsoft.Extensions.Configuration.Tests_netcoreapp3.0_x64.log" 2>&1

@analogrelay
Copy link
Author

GitHub Checks:

image

AzP Tests Page:

image

Unix builds are failing due to a permissions error (grumble...). I'll fix it

Looks good to me as long as Unix follows along after the permissions are fixed.

I'm going to leave the FlakyAttributeTests in place for now. They do mean that Extensions will always have failing flaky tests, because they are in fact tests that test the FlakyAttribute.

@ryanbrandenburg
Copy link

RE Tests that test FlakyAttributeTests: RAAS will have to start having a list of tests which it won't fail for (because they're intended to fail.).

@analogrelay
Copy link
Author

Ah, good point. Ideally it should just be ignoring all tests from the flaky test pass, but I don't think you get that much detail when querying the APIs you're querying.

@ryanbrandenburg
Copy link

I don't follow why we would want RaaS to ignore tests from the Flaky pass. RaaS should triage them like normal and update the affected issues. Isn't that the whole point of the Flaky attribute, to still get stats about how often these fail, without blocking?

@analogrelay
Copy link
Author

Assuming we think it will work well, that should be fine. My concern is that a flaky test will already have an issue tracking it (the attribute requires that an issue be filled in). Will RaaS be able to find and update that issue? Or will we end up with two issues?

@analogrelay
Copy link
Author

Merging this now.

@Eilon - Be aware that RaaS may file issues in AspNetCore-Internal for the FlakyAttributeTest tests because it doesn't know they are expected to fail, and RaaS looks at all failing tests, which includes these. Of course, RaaS only runs on failing builds in master right now and those are much rarer in Extensions so they may never come up, but I wanted to bring it up.

@ryanbrandenburg is planning to fix that but we don't want to block this PR on that change. We can just disregard those issues when they appear.

@analogrelay analogrelay merged commit 50648ac into master Mar 13, 2019
analogrelay added a commit to dotnet/aspnetcore that referenced this pull request Mar 19, 2019
@smitpatel smitpatel deleted the anurse/flaky-pass branch April 17, 2019 03:19
@ghost ghost locked as resolved and limited conversation to collaborators May 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants