-
Notifications
You must be signed in to change notification settings - Fork 840
add FlakyAttribute to mark flaky tests #1222
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
Changes from 11 commits
15e5ac6
a54c39f
df91f17
1d6767f
eaafdf1
5e20c6e
cb6a933
23eecb6
c64a342
d0aef7b
396f381
ef2b905
0b3c147
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| <Project> | ||
| <Target Name="_GenerateTestPasses" DependsOnTargets="$(_GetTestsToRunTarget)"> | ||
| <PropertyGroup> | ||
| <_FlakyRunAdditionalArgs>-trait "Flaky:All=true" -trait "Flaky:AzP:All=true" -trait "Flaky:AzP:OS:$(AGENT_OS)=true"</_FlakyRunAdditionalArgs> | ||
| <_NonFlakyRunAdditionalArgs>-notrait "Flaky:All=true" -notrait "Flaky:AzP:All=true" -notrait "Flaky:AzP:OS:$(AGENT_OS)=true"</_NonFlakyRunAdditionalArgs> | ||
| </PropertyGroup> | ||
| <ItemGroup> | ||
| <!-- Take the TestToRun values and update them to only run non-flaky tests --> | ||
| <NonFlakyRuns Include="@(TestToRun)"> | ||
| <TestRunnerAdditionalArguments>%(TestRunnerAdditionalArguments) $(_NonFlakyRunAdditionalArgs)</TestRunnerAdditionalArguments> | ||
| </NonFlakyRuns> | ||
|
|
||
| <!-- Add new runs that do run flaky tests but ignore the exit code --> | ||
| <FlakyRuns Include="@(TestToRun)"> | ||
| <TestRunnerAdditionalArguments>%(TestRunnerAdditionalArguments) $(_FlakyRunAdditionalArgs)</TestRunnerAdditionalArguments> | ||
| <TestRunnerIgnoreExitCode>true</TestRunnerIgnoreExitCode> | ||
| <ResultsHtmlPath>$([System.IO.Path]::ChangeExtension(%(ResultsHtmlPath), '.flaky.html'))</ResultsHtmlPath> | ||
| <ResultsStdOutPath>$([System.IO.Path]::ChangeExtension(%(ResultsStdOutPath), '.flaky.log'))</ResultsStdOutPath> | ||
| <ResultsXmlPath>$([System.IO.Path]::ChangeExtension(%(ResultsXmlPath), '.flaky.xml'))</ResultsXmlPath> | ||
| </FlakyRuns> | ||
|
|
||
| <!-- Replace the previous runs --> | ||
| <TestToRun Remove="@(TestToRun)" /> | ||
|
|
||
| <!-- TODO: Replace with below when https://github.com/dotnet/arcade/issues/2182 is in --> | ||
| <!-- <TestToRun Include="@(NonFlakyRuns);@(FlakyRuns)" /> --> | ||
| <TestToRun Include="@(NonFlakyRuns)" /> | ||
| </ItemGroup> | ||
| </Target> | ||
|
|
||
| <!-- Replace Arcade's Test target with a custom one --> | ||
| <Target Name="Test" DependsOnTargets="_GenerateTestPasses;RunTests" Condition="'$(IsUnitTestProject)' == 'true' or '$(IsPerformanceTestProject)' == 'true'" /> | ||
| </Project> | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -91,6 +91,14 @@ public virtual void Initialize(MethodInfo methodInfo, object[] testMethodArgumen | |
|
|
||
| public virtual void Dispose() | ||
| { | ||
| if(_testLog == null) | ||
| { | ||
| // It seems like sometimes the MSBuild goop that adds the test framework can end up in a bad state and not actually add it | ||
| // Not sure yet why that happens but the exception isn't clear so I'm adding this error so we can detect it better. | ||
| // -anurse | ||
| throw new InvalidOperationException("LoggedTest base class was used but nothing initialized it! The test framework may not be enabled. Try cleaning your 'obj' directory."); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hit this while testing... not sure why, but want to make it a little easier to catch and possibly diagnose if it happens again. It doesn't seem to happen at all in AzP because we use a clean environment. The ".cache" file for my assembly attributes was broken somehow which meant the MSBuild goop never actually generated new Assembly Attributes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah right, I think we are missing an issue tracking reenabling this on helix, If you can isolate/fix the root cause, we could take this out https://github.com/aspnet/AspNetCore/blob/master/eng/targets/Helix.props#L18 |
||
| } | ||
|
|
||
| _initializationException?.Throw(); | ||
| _testLog.Dispose(); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Reflection; | ||
|
|
||
| namespace Microsoft.AspNetCore.Testing | ||
| { | ||
| public static class AzurePipelines | ||
| { | ||
| public const string All = Prefix + "All"; | ||
| public const string Windows = OsPrefix + "Windows_NT"; | ||
| public const string macOS = OsPrefix + "Darwin"; | ||
| public const string Linux = OsPrefix + "Linux"; | ||
|
|
||
| private const string Prefix = "AzP:"; | ||
| private const string OsPrefix = Prefix + "AzP:OS:"; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Reflection; | ||
|
|
||
| namespace Microsoft.AspNetCore.Testing | ||
| { | ||
| public static class HelixQueues | ||
| { | ||
| public const string All = Prefix + "All"; | ||
|
|
||
| public const string Fedora28Amd64 = QueuePrefix + "Fedora.28." + Amd64Suffix; | ||
| public const string Fedora27Amd64 = QueuePrefix + "Fedora.27." + Amd64Suffix; | ||
| public const string Redhat7Amd64 = QueuePrefix + "Redhat.7." + Amd64Suffix; | ||
| public const string Debian9Amd64 = QueuePrefix + "Debian.9." + Amd64Suffix; | ||
| public const string Debian8Amd64 = QueuePrefix + "Debian.8." + Amd64Suffix; | ||
| public const string Centos7Amd64 = QueuePrefix + "Centos.7." + Amd64Suffix; | ||
| public const string Ubuntu1604Amd64 = QueuePrefix + "Ubuntu.1604." + Amd64Suffix; | ||
| public const string Ubuntu1810Amd64 = QueuePrefix + "Ubuntu.1810." + Amd64Suffix; | ||
| public const string macOS1012Amd64 = QueuePrefix + "OSX.1012." + Amd64Suffix; | ||
| public const string Windows10Amd64 = QueuePrefix + "Windows.10.Amd64.ClientRS4.VS2017.Open"; // Doesn't have the default suffix! | ||
|
|
||
| private const string Prefix = "Helix:"; | ||
| private const string QueuePrefix = Prefix + "Queue:"; | ||
| private const string Amd64Suffix = "Amd64.Open"; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,75 @@ | ||
| using System; | ||
| using System.Collections.Generic; | ||
| using Xunit.Sdk; | ||
|
|
||
| namespace Microsoft.AspNetCore.Testing.xunit | ||
| { | ||
| /// <summary> | ||
| /// Marks a test as "Flaky" so that the build will sequester it and ignore failures. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// <para> | ||
| /// This attribute works by applying xUnit.net "Traits" based on the criteria specified in the attribute | ||
| /// properties. Once these traits are applied, build scripts can include/exclude tests based on them. | ||
| /// </para> | ||
| /// <para> | ||
| /// All flakiness-related traits start with <code>Flaky:</code> and are grouped first by the process running the tests: Azure Pipelines (AzP) or Helix. | ||
| /// Then there is a segment specifying the "selector" which indicates where the test is flaky. Finally a segment specifying the value of that selector. | ||
| /// The value of these traits is always either "true" or the trait is not present. We encode the entire selector in the name of the trait because xUnit.net only | ||
| /// provides "==" and "!=" operators for traits, there is no way to check if a trait "contains" or "does not contain" a value. VSTest does support "contains" checks | ||
| /// but does not appear to support "does not contain" checks. Using this pattern means we can use simple "==" and "!=" checks to either only run flaky tests, or exclude | ||
| /// flaky tests. | ||
| /// </para> | ||
| /// </remarks> | ||
| /// <example> | ||
| /// <code> | ||
| /// [Fact] | ||
| /// [Flaky("...", HelixQueues.Fedora28Amd64, AzurePipelines.macOS)] | ||
| /// public void FlakyTest() | ||
| /// { | ||
| /// // Flakiness | ||
| /// } | ||
| /// </code> | ||
| /// | ||
| /// <para> | ||
| /// The above example generates the following facets: | ||
| /// </para> | ||
| /// | ||
| /// <list type="bullet"> | ||
| /// <item> | ||
| /// <description><c>Flaky:Helix:Queue:Fedora.28.Amd64.Open</c> = <c>true</c></description> | ||
| /// </item> | ||
| /// <item> | ||
| /// <description><c>Flaky:AzP:OS:Darwin</c> = <c>true</c></description> | ||
| /// </item> | ||
| /// </list> | ||
| /// | ||
| /// <para> | ||
| /// Given the above attribute, the Azure Pipelines macOS run can easily filter this test out by passing <c>-notrait "Flaky:AzP:OS:all=true" -notrait "Flaky:AzP:OS:Darwin=true"</c> | ||
| /// to <c>xunit.console.exe</c>. Similarly, it can run only flaky tests using <c>-trait "Flaky:AzP:OS:all=true" -trait "Flaky:AzP:OS:Darwin=true"</c> | ||
| /// </para> | ||
| /// </example> | ||
| [TraitDiscoverer("Microsoft.AspNetCore.Testing.xunit.FlakyTestDiscoverer", "Microsoft.AspNetCore.Testing")] | ||
| [AttributeUsage(AttributeTargets.Method)] | ||
| public sealed class FlakyAttribute : Attribute, ITraitAttribute | ||
| { | ||
| /// <summary> | ||
| /// Gets a URL to a GitHub issue tracking this flaky test. | ||
| /// </summary> | ||
| public string GitHubIssueUrl { get; } | ||
|
|
||
| public IReadOnlyList<string> Filters { get; } | ||
|
|
||
| /// <summary> | ||
| /// Initializes a new instance of the <see cref="FlakyAttribute"/> class with the specified <see cref="GitHubIssueUrl"/> and a list of <see cref="Filters"/>. If no | ||
| /// filters are provided, the test is considered flaky in all environments. | ||
| /// </summary> | ||
| /// <param name="gitHubIssueUrl">The URL to a GitHub issue tracking this flaky test.</param> | ||
| /// <param name="filters">A list of filters that define where this test is flaky. Use values in <see cref="AzurePipelines"/> and <see cref="HelixQueues"/>.</param> | ||
| public FlakyAttribute(string gitHubIssueUrl, params string[] filters) | ||
| { | ||
| GitHubIssueUrl = gitHubIssueUrl; | ||
| Filters = new List<string>(filters); | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.Linq; | ||
| using Xunit.Abstractions; | ||
| using Xunit.Sdk; | ||
|
|
||
| namespace Microsoft.AspNetCore.Testing.xunit | ||
| { | ||
| public class FlakyTestDiscoverer : ITraitDiscoverer | ||
| { | ||
| public IEnumerable<KeyValuePair<string, string>> GetTraits(IAttributeInfo traitAttribute) | ||
| { | ||
| if (traitAttribute is ReflectionAttributeInfo attribute && attribute.Attribute is FlakyAttribute flakyAttribute) | ||
| { | ||
| return GetTraitsCore(flakyAttribute); | ||
| } | ||
| else | ||
| { | ||
| throw new InvalidOperationException("The 'Flaky' attribute is only supported via reflection."); | ||
| } | ||
| } | ||
|
|
||
| private IEnumerable<KeyValuePair<string, string>> GetTraitsCore(FlakyAttribute attribute) | ||
| { | ||
| if (attribute.Filters.Count > 0) | ||
| { | ||
| foreach (var filter in attribute.Filters) | ||
| { | ||
| yield return new KeyValuePair<string, string>($"Flaky:{filter}", "true"); | ||
| } | ||
| } | ||
| else | ||
| { | ||
| yield return new KeyValuePair<string, string>($"Flaky:All", "true"); | ||
| } | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,104 @@ | ||
| using Microsoft.AspNetCore.Testing.xunit; | ||
| using System; | ||
| using System.Collections.Generic; | ||
| using Xunit; | ||
|
|
||
| namespace Microsoft.AspNetCore.Testing.Tests | ||
| { | ||
| public class FlakyAttributeTest | ||
| { | ||
| [Fact] | ||
| [Flaky("http://example.com")] | ||
| public void AlwaysFlaky() | ||
| { | ||
| if (!string.IsNullOrEmpty(Environment.GetEnvironmentVariable("HELIX")) || !string.IsNullOrEmpty(Environment.GetEnvironmentVariable("AGENT_OS"))) | ||
| { | ||
| throw new Exception("Flaky!"); | ||
| } | ||
| } | ||
|
|
||
| [Fact] | ||
| [Flaky("http://example.com", HelixQueues.All)] | ||
| public void FlakyInHelixOnly() | ||
| { | ||
| if (!string.IsNullOrEmpty(Environment.GetEnvironmentVariable("HELIX"))) | ||
| { | ||
| throw new Exception("Flaky on Helix!"); | ||
| } | ||
| } | ||
|
|
||
| [Fact] | ||
| [Flaky("http://example.com", HelixQueues.macOS1012Amd64, HelixQueues.Fedora28Amd64)] | ||
| public void FlakyInSpecificHelixQueue() | ||
| { | ||
| // Today we don't run Extensions tests on Helix, but this test should light up when we do. | ||
| var queueName = Environment.GetEnvironmentVariable("HELIX"); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @HaoK I noticed dotnet/aspnetcore#8231 is adding this env var, can you double-check that my logic here seems sound? The idea is to actually make this test properly fail on the specified Helix queues so we can leave it in as validation that flaky tests are indeed treated properly, so I want this test to actually be correctly detecting the queue it's running in :)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might be time to have a HelixUtils to consolidate all the helix stuff in one central file, or you could use SkipOnHelixAttribute.GetTargetHelixQueue() when that PR is merged. Logic looks fine. One other pivot we should probably consider, is adding the concept of Platform as well, so there's an easy way to mark So maybe we need to : There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could just do that by building combo- // Common Groupings
public const string Ubuntu = Ubuntu1604Amd64 + Ubuntu1810Amd64;
public const string Centos = Centos7Amd64;
public const string Debian = Debian8Amd64 + Debian9Amd64;
public const string Redhat = Redhat7Amd64;
public const string Fedora = Fedora27Amd64 + Fedora28Amd64;
public const string Windows = Windows10Amd64;
public const string macOS = macOS1012Amd64;
public const string Linux = Ubuntu + Centos + Debian + Redhat + Fedora;We could also add other pivots to the flaky traits. I already changed it a bit so that the trait we insert is |
||
| if (!string.IsNullOrEmpty(queueName)) | ||
| { | ||
|
|
||
| // Normalize the queue name to have a trailing ';' (this is only for testing anyway) | ||
| if (!queueName.EndsWith(";")) | ||
| { | ||
| queueName = $"{queueName};"; | ||
| } | ||
|
|
||
| var failingQueues = new HashSet<string>(StringComparer.OrdinalIgnoreCase) { HelixQueues.macOS1012Amd64, HelixQueues.Fedora28Amd64 }; | ||
| if (failingQueues.Contains(queueName)) | ||
| { | ||
| throw new Exception($"Flaky on Helix Queue '{queueName}' !"); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| [Fact] | ||
| [Flaky("http://example.com", AzurePipelines.All)] | ||
| public void FlakyInAzDoOnly() | ||
| { | ||
| if (!string.IsNullOrEmpty(Environment.GetEnvironmentVariable("AGENT_OS"))) | ||
| { | ||
| throw new Exception("Flaky on AzP!"); | ||
| } | ||
| } | ||
|
|
||
| [Fact] | ||
| [Flaky("http://example.com", OnHelix = HelixQueues.None, OnAzP = AzurePipelines.Windows)] | ||
| public void FlakyInAzPWindowsOnly() | ||
| { | ||
| if (string.Equals(Environment.GetEnvironmentVariable("AGENT_OS"), "Windows_NT")) | ||
| { | ||
| throw new Exception("Flaky on AzP Windows!"); | ||
| } | ||
| } | ||
|
|
||
| [Fact] | ||
| [Flaky("http://example.com", OnHelix = HelixQueues.None, OnAzP = AzurePipelines.macOS)] | ||
| public void FlakyInAzPmacOSOnly() | ||
| { | ||
| if (string.Equals(Environment.GetEnvironmentVariable("AGENT_OS"), "Darwin")) | ||
| { | ||
| throw new Exception("Flaky on AzP macOS!"); | ||
| } | ||
| } | ||
|
|
||
| [Fact] | ||
| [Flaky("http://example.com", OnHelix = HelixQueues.None, OnAzP = AzurePipelines.Linux)] | ||
| public void FlakyInAzPLinuxOnly() | ||
| { | ||
| if (string.Equals(Environment.GetEnvironmentVariable("AGENT_OS"), "Linux")) | ||
| { | ||
| throw new Exception("Flaky on AzP Linux!"); | ||
| } | ||
| } | ||
|
|
||
| [Fact] | ||
| [Flaky("http://example.com", OnHelix = HelixQueues.None, OnAzP = AzurePipelines.Linux + AzurePipelines.macOS)] | ||
| public void FlakyInAzPNonWindowsOnly() | ||
| { | ||
| var agentOs = Environment.GetEnvironmentVariable("AGENT_OS"); | ||
| if (string.Equals(agentOs, "Linux") || string.Equals(agentOs, "Darwin")) | ||
| { | ||
| throw new Exception("Flaky on AzP non-Windows!"); | ||
| } | ||
| } | ||
| } | ||
| } | ||
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.
Note that we do not run flaky tests at all right now because of dotnet/arcade#2182