Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions src/TestingUtils/Microsoft.AspNetCore.Testing/src/HelixQueues.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
using System;
using System.Collections.Generic;
using System.Reflection;

namespace Microsoft.AspNetCore.Testing
{
public static class HelixQueues
{
public const string All = "all";
public const string None = "none";

// Queue names end in ';' because it makes it easier to concat these into a list using a constant expression:
// HelixQueues.Fedora28 + HelixQueues.Centos7

public const string Fedora28 = "Fedora.28." + HelixSuffix + ";";
public const string Fedora27 = "Fedora.27." + HelixSuffix + ";";
public const string Redhat7 = "Redhat.7." + HelixSuffix + ";";
public const string Debian9 = "Debian.9." + HelixSuffix + ";";
public const string Debian8 = "Debian.8." + HelixSuffix + ";";
public const string Centos7 = "Centos.7." + HelixSuffix + ";";
public const string Ubuntu1604 = "Ubuntu.1604." + HelixSuffix + ";";
public const string Ubuntu1810 = "Ubuntu.1810." + HelixSuffix + ";";
public const string macOS1012 = "OSX.1012." + HelixSuffix + ";";
public const string Windows10 = "Windows.10.Amd64.ClientRS4.VS2017.Open;"; // Doesn't have the default suffix!

private const string HelixSuffix = "Amd64.Open";
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately we did/will have Arm64 queues too, they just were exploding so they aren't ready for frontline action yet. Maybe we need a Flaky Queue too :)

Copy link
Author

Choose a reason for hiding this comment

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

I don't think we need a flaky queue, with this logic we can have our test scripts run both flaky and non-flaky tests, but only fail the build on non-flaky failures.

We can definitely update this as we have new queue explosions :). I can put Amd64 in the field names for now to ensure they don't break when we add Arm64 though.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah what I mean is its possible we will want to run on new queues (i.e. arm testing) where the queues themselves have a high failure rate, today we just removed the queue entirely, but it would be nice to be able to run all tests on a flaky queue (just in the don't fail the build bucket). We would have to mark every test with [Flaky(OnHelixQueues="imsoflaky")] I think without the concept. Not something we need now, but something to keep in mind for the future

}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
using System;
using System.Collections.Generic;
using Xunit.Sdk;

namespace Microsoft.AspNetCore.Testing.xunit
{
[TraitDiscoverer("Microsoft.AspNetCore.Testing.xunit.FlakyTestDiscoverer", "Microsoft.AspNetCore.Testing")]
[AttributeUsage(AttributeTargets.Method)]
public sealed class FlakyAttribute : Attribute, ITraitAttribute
{
private List<string> _queues = new List<string>() { "all" };

/// <summary>
/// Gets a URL to a GitHub issue tracking this flaky test.
/// </summary>
public string GitHubIssueUrl { get; }

/// <summary>
/// Gets or sets a list of helix queues on which this test is flaky. Defaults to <see cref="HelixQueues.All"/> indicating it is flaky on all Helix queues. See
/// <see cref="HelixQueues"/> for a list of valid values.
/// </summary>
public string OnHelixQueues
{
get => string.Join(";", _queues);
set => _queues = new List<string>(value.Split(';'));
Copy link

Choose a reason for hiding this comment

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

Do you need to pass in StringSplitOptions.RemoveEmptyEntries?

Copy link
Author

Choose a reason for hiding this comment

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

Probably, though it's not broken without it I believe. It would just generate the trait Flaky:Helix:=true which we never care about :).

}

/// <summary>
/// Gets or sets a boolean indicating if this test is flaky on Azure DevOps Pipelines. Defaults to <see langword="true" />.
/// </summary>
public bool OnAzDO { get; set; } = true;
Copy link

Choose a reason for hiding this comment

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

Does this need AzDO build definition name (OS)?

Copy link
Author

Choose a reason for hiding this comment

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

Perhaps


/// <summary>
/// Gets a list of Helix queues on which the test is flaky (including <see cref="HelixQueues.All"/> if specified).
/// </summary>
public IReadOnlyList<string> FlakyQueues => _queues;

public FlakyAttribute(string gitHubIssueUrl)
{
GitHubIssueUrl = gitHubIssueUrl;
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
using System;
using System.Collections.Generic;
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.OnAzDO)
{
yield return new KeyValuePair<string, string>("Flaky:AzDO", "true");
}

foreach(var queue in attribute.FlakyQueues)
{
yield return new KeyValuePair<string, string>($"Flaky:Helix:{queue}", "true");
}
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
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()
{
throw new Exception("Flakey!");
Copy link

Choose a reason for hiding this comment

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

Flaky

Copy link
Author

Choose a reason for hiding this comment

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

Yep, also I don't want this test to fail for dev builds so I'll have it detect either AzDO or Helix and only fail there.

}

[Fact]
[Flaky("http://example.com", OnAzDO = false)]
public void FlakyInHelixOnly()
{
if (!string.IsNullOrEmpty(Environment.GetEnvironmentVariable("HELIX")))
{
throw new Exception("Flaky on Helix!");
}
}

[Fact]
[Flaky("http://example.com", OnAzDO = false, OnHelixQueues = HelixQueues.macOS1012 + HelixQueues.Fedora28)]
public void FlakyInSpecificHelixQueue()
{
var queueName = Environment.GetEnvironmentVariable("HELIX");
Copy link
Author

Choose a reason for hiding this comment

The 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 :)

Copy link
Member

@HaoK HaoK Mar 6, 2019

Choose a reason for hiding this comment

The 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 Flaky(OnlyPlatforms = "OSX;Linux") so we don't have to enumerate every queue

So maybe we need to :

   public class HelixQueueDefinition {
       public string Id;
       public string Platform
   }
   public static class HelixPlatforms {
       public string Windows = "Windows";
       public string OSX = "OSX";
       public string Linux = "Linux";
   }

Copy link
Author

Choose a reason for hiding this comment

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

We could just do that by building combo-consts like so:

        // 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 Flaky:Helix:Queue:[queuename], we could have other facets in that sequence and as long as the script running the tests injects all the necessary filters, it should work.

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.macOS1012, HelixQueues.Fedora28 };
if (failingQueues.Contains(queueName))
{
throw new Exception("Flaky on Helix!");
}
}
}

[Fact]
[Flaky("http://example.com", OnHelixQueues = HelixQueues.None)]
Copy link

Choose a reason for hiding this comment

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

Can we use these on a few existing tests we know are flaky? Start putting the badge of shame up.

Copy link
Author

Choose a reason for hiding this comment

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

Not yet. We'll need build goop to actually exclude them

Copy link
Author

Choose a reason for hiding this comment

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

So you could use it when this is in, but it won't actually do anything.

Copy link
Author

Choose a reason for hiding this comment

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

To make it more fun, it looks like Extensions and AspNetCore will need slightly different build goop, but it looks doable.

Copy link
Member

Choose a reason for hiding this comment

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

The good news is that helix isn't used on extensions... so you won't be getting many flaky helix tests on there hopefully :)

public void FlakyInAzDoOnly()
{
if (!string.IsNullOrEmpty(Environment.GetEnvironmentVariable("BUILD_BUILDNUMBER")))
{
throw new Exception("Flaky on AzDO!");
}
}
}
}