"Flaky" attribute for tests that are flaky in various environments

[analogrelay]

Tracking work:

• Add flaky attribute add FlakyAttribute to mark flaky tests extensions#1222
• Enable flaky test pass in Extensions Run flaky tests in separate pass extensions#1224
• Enable flaky attribute in AspNetCore AzP
• Enable flaky attribute in AspNetCore Helix

public static class HelixQueues
{
    // Must be const because they are used in attributes!
    public const string All = "All";
    public const string Debian8 = "...";
    // ...
}

public sealed class FlakyAttribute
{
    // Required for code inspection analytics (verifying that the issue remains open, etc.)
    public string GitHubIssueLink { get; }

    // Specific helix queues on which this test is deemed "flaky"
    // If not specified or 'all', implies all queues
    // If 'none', implies that test runs on all queues
    // Otherwise, this is a semi-colon-delimited list of queues
    public string OnHelixQueues { get; set; } = "all";

    // Indicates if this test is flaky in AzDO
    public bool OnAzDO { get; set; } = true;

    public FlakyAttribute(string gitHubIssueLink) { ... }
}

Usage examples:

• [Flaky("...")] - This test is always flaky
• [Flaky("...", OnHelixQueues = "none")] - This test is flaky on AzDO but never on Helix
• [Flaky("...", OnAzDO = false)] - This test is flaky on Helix but never on AzDO
• [Flaky("...", OnHelixQueues = "Debian8.whatchamajigger...;Ubuntu.CromulantCrux...")] - This test is flaky on AzDO and specific Helix queues but never on the other Helix queues
• [Flaky("...", OnHelixQueues = "Debian8.whatchamajigger...;Ubuntu.CromulantCrux...", OnAzDO = false)] - This test is only on specific helix queues and never flaky on the other queues and AzDO

The idea being that the attribute defines the environments in which the test is flaky and the tooling for those builds will sequester the test as necessary.

Discussions are open on making sure these properties are clear and understandable :). My goal was to say that by default Flaky indicates the entire test is flaky in all environments and then the other attributes can be used to "loosen" the requirements.

The implementation is still somewhat TBD (I'll be playing with this today) but the idea is this:

• The properties of the attribute determine if the flaky xunit trait will be applied
• The build script will run two passes for each project:
  • One excluding the flaky trait
  • One including the flaky trait which will ignore the exit code, but still record the results.

This all depends on the infrastructure supporting what I want to do here, but I think we can get away with not having a separate AspNetCore-flaky-ci run :).

I think this can live in https://github.com/aspnet/Extensions/tree/master/src/TestingUtils/Microsoft.AspNetCore.Testing and be accessible to everyone.. or we can use a shared-source file like we do with SkipOnHelixAttribute today.

@Eilon @muratg @mkArtakMSFT @ajcvickers @HaoK @ryanbrandenburg @dougbu (maybe we need an aspnet/engineering GitHub team ;))

[analogrelay]

Some useful notes (for my own reference):

• https://kenbonny.net/2016/12/20/creating-a-custom-xunit-trait/comment-page-1/
• https://github.com/Microsoft/vstest-docs/blob/master/docs/filter.md

[analogrelay]

One thing to think about is how this attribute affects dev builds. My feeling is that it has no effect and we still run flaky tests on dev builds. That's the easiest pattern right now, but I'm open to discussion :).

[Eilon]

Hmm I suppose we could try that, but why bother?

[ryanbrandenburg]

AzDO seems about as likely to have queue-specific flakiness as Helix, may as well allow it to be list the queue's it hates too.

[analogrelay]

Can do!

[analogrelay]

Hmm I suppose we could try that, but why bother?

Mostly because of how the traits work. By default, VS doesn't apply any trait filters, nor does dotnet test (with no additional filters). The only place we could reliably add flaky test filtering for local dev builds is in build.cmd -test and if they decide to run dotnet test --filter "Flaky:Local=true" or something like that.

[Eilon]

Ah OK

[analogrelay]

Plus the goal here is clean CI builds. Right now, I'm not too worried if engineers who are running tests in the projects containing flakiness see flakiness :). It's likely their responsibility anyway 😈.

[HaoK]

Maybe this is getting too far ahead, but it seems like we also need some kind of automated good behavior/parole for falsely accused 'innocent' tests that might be jailed for things unrelated to them, i.e. will we be able to easily do something like parole any tests that haven't failed in a week/month? Seems important to have a reasonably lax parole system if its 1 strike and its off to flake island...

[analogrelay]

Yeah, that's the kind of analysis we can do by scraping the TRX/xUnit reports from the flaky test runs. That's one major reason I want to keep running and reporting them :).

[Eilon]

This is done.