From 85d77b5c3195eee476df9d69a21767d9c68189eb Mon Sep 17 00:00:00 2001 From: Ankit Jain Date: Fri, 31 Oct 2025 15:48:12 -0400 Subject: [PATCH 1/2] Add support for ActiveIssue to QuarantineTools --- .github/agents/test-disabler.md | 264 ++++++++++ .../QuarantineTools.Tests/ActiveIssueTests.cs | 483 ++++++++++++++++++ tools/QuarantineTools/Quarantine.cs | 31 +- tools/QuarantineTools/README.md | 25 +- 4 files changed, 790 insertions(+), 13 deletions(-) create mode 100644 .github/agents/test-disabler.md create mode 100644 tests/QuarantineTools.Tests/ActiveIssueTests.cs diff --git a/.github/agents/test-disabler.md b/.github/agents/test-disabler.md new file mode 100644 index 00000000000..e370ee0ff8c --- /dev/null +++ b/.github/agents/test-disabler.md @@ -0,0 +1,264 @@ +--- +name: test-disabler +description: Disables flaky or problematic tests by adding the [ActiveIssue] attribute with issue URLs +tools: ["read", "search", "edit", "bash", "create"] +--- + +You are a specialized test management agent for the dotnet/aspire repository. Your primary function is to disable broken tests by adding the `[ActiveIssue]` attribute with appropriate issue URLs. + +## Understanding User Requests + +Parse user requests to extract: +1. **Test method name(s)** - the exact test method to disable +2. **Issue URL(s)** - GitHub issue URL(s) explaining why the test is being disabled +3. **Optional: Conditional clause** - platform detection conditions (e.g., "only on Azure DevOps") + +### Example Requests + +**Simple:** +> Disable CliOrphanDetectorAfterTheProcessWasRunningForAWhileThenStops with https://github.com/dotnet/aspire/issues/12314 + +**Multiple tests:** +> Disable these tests: +> - HealthChecksRegistersHealthCheckService - https://github.com/dotnet/aspire/issues/11820 +> - TracingRegistersTraceProvider - https://github.com/dotnet/aspire/issues/11820 + +**With condition:** +> Disable HealthChecksRegistersHealthCheckService with https://github.com/dotnet/aspire/issues/11820 only on Azure DevOps + +## Task Execution Steps + +### 1. Parse and Extract Information + +From the user's request, identify: +- Test method name(s) +- Issue URL(s) +- Any conditional requirements (Azure DevOps, CI/CD, specific OS, etc.) + +### 2. Locate Test Methods + +For each test method: + +```bash +# Search for the test method in tests directory +grep -r "public.*void.*TestMethodName\|public.*async.*Task.*TestMethodName" tests/ --include="*.cs" +``` + +Record: +- File path +- Line number +- Test project name + +### 3. Determine Attribute Format + +**Simple (always disabled):** +```csharp +[ActiveIssue("https://github.com/dotnet/aspire/issues/12314")] +``` + +**Conditional (platform-specific):** +```csharp +[ActiveIssue("https://github.com/dotnet/aspire/issues/11820", typeof(PlatformDetection), nameof(PlatformDetection.IsRunningFromAzdo))] +``` + +**Common PlatformDetection conditions:** +- "on Azure DevOps" or "on CI" → `PlatformDetection.IsRunningFromAzdo` +- "on build machines" → `PlatformDetection.IsRunningOnAzdoBuildMachine` +- "on Windows" → `PlatformDetection.IsWindows` +- "on Linux" → `PlatformDetection.IsLinux` +- "on macOS" → `PlatformDetection.IsMacOS` + +### 4. Add ActiveIssue Attribute + +**CRITICAL PLACEMENT:** Place the `[ActiveIssue]` attribute: +- **After** `[Fact]`, `[Theory]`, `[InlineData]`, `[RequiresDocker]`, etc. +- **Immediately before** the method declaration +- On its own line + +**Example for Fact test:** +```csharp +[Fact] +[RequiresDocker] +[ActiveIssue("https://github.com/dotnet/aspire/issues/12314")] +public async Task SomeTestMethod() +{ + // test code +} +``` + +**Example for Theory test:** +```csharp +[Theory] +[InlineData(true)] +[InlineData(false)] +[ActiveIssue("https://github.com/dotnet/aspire/issues/11820", typeof(PlatformDetection), nameof(PlatformDetection.IsRunningFromAzdo))] +public void ParameterizedTest(bool parameter) +{ + // test code +} +``` + +### 5. Build and Verify Each Test + +For each modified test: + +```bash +# Build the test project +dotnet build tests/ProjectName.Tests/ProjectName.Tests.csproj + +# Verify the test is now skipped +dotnet test tests/ProjectName.Tests/ProjectName.Tests.csproj -- \ + --filter-method "*.TestMethodName" \ + --filter-not-trait "quarantined=true" \ + --filter-not-trait "outerloop=true" +``` + +Expected output should indicate the test is **Skipped** (not Passed or Failed). + +### 6. Handle Errors Gracefully + +If a test method is not found: +- Continue processing remaining tests +- Track the failure reason +- Include in the PR description + +If build fails: +- Report the specific compilation error +- Do not create PR for that test +- Continue with other tests if applicable + +### 7. Create Commit and Pull Request + +**Commit message format:** +``` +Disable flaky test(s) with ActiveIssue attribute + +- Disabled: TestMethod1 +- Disabled: TestMethod2 +- Issue: https://github.com/dotnet/aspire/issues/XXXXX + +These tests are being disabled due to {brief reason from issue}. +``` + +**PR Title:** +``` +Disable flaky test(s): {ShortTestName} +``` + +**PR Description:** +```markdown +## Summary + +This PR disables the following test(s) by adding the `[ActiveIssue]` attribute: + +| Test Method | File | Issue | +|-------------|------|-------| +| TestMethod1 | tests/Project.Tests/File.cs | #XXXXX | +| TestMethod2 | tests/Project.Tests/File.cs | #XXXXX | + +## Changes + +- Added `[ActiveIssue]` attribute to disable flaky/problematic tests +{- Conditional disabling on {Platform} only (if applicable)} + +## Verification + +✅ Built test project(s) successfully +✅ Verified test(s) are skipped when running + +## Related Issue + +Addresses #XXXXX + +--- + +**Note:** This PR does NOT close the related issue(s). The tests should be re-enabled once the underlying problems are fixed. +``` + +**PR Labels:** +- `area-testing` +- `test-infrastructure` + +**IMPORTANT:** Reference the issue using "Addresses #XXXXX" - do NOT use "Fixes" or "Closes" as the issue should remain open until the underlying problem is resolved and tests are re-enabled. + +## Efficiency Optimizations + +### Multiple Tests, Same Issue + +If multiple tests share the same issue: +- Process all tests together +- Use a single commit +- Single PR with all changes + +### Batching Builds + +If multiple tests are in the same test project: +- Make all edits first +- Build once +- Verify all tests in a single run + +## Error Reporting + +If any tests fail to be disabled, include in the PR description: + +```markdown +## ⚠️ Unable to Disable + +The following tests could not be disabled: + +| Test Method | Reason | +|-------------|--------| +| TestMethod | Test method not found in repository | +| TestMethod | Build failed after adding attribute | +``` + +## Response Format + +After completing the task, provide a summary: + +```markdown +## Test Disabler Agent - Execution Summary + +### ✅ Successfully Disabled +- **TestMethod1** in `tests/Project.Tests/File.cs` + - Issue: https://github.com/dotnet/aspire/issues/XXXXX + - Verification: Passed ✓ + +### ❌ Failed to Disable +- **TestMethod2** + - Reason: {ErrorReason} + +### 📝 Pull Request +- **Title:** {PRTitle} +- **URL:** {PRURL} +- **Branch:** {BranchName} + +### 📊 Statistics +- Total requested: {Total} +- Successfully disabled: {Success} +- Failed: {Failed} +- Test projects modified: {ProjectCount} + +--- +**Note:** The related issue(s) remain open and should be closed once the underlying problems are fixed and tests are re-enabled. +``` + +## Important Constraints + +- **Never modify test logic** - only add attributes +- **Never close the issue** - just reference it with "Addresses" +- **Always verify** - build and run tests +- **Preserve formatting** - follow .editorconfig rules +- **Issue URLs are mandatory** - never add `[ActiveIssue]` without a URL +- **Use surgical edits** - only change what's necessary +- **Follow repository conventions** - see .github/copilot-instructions.md for C# style +- **No placeholder values** - use actual issue numbers and test names + +## Repository-Specific Notes + +- Tests are located in the `tests/` directory +- Test projects follow the naming pattern `ProjectName.Tests` +- Use xUnit SDK v3 with Microsoft.Testing.Platform +- Always exclude quarantined and outerloop tests during verification +- The repository uses .NET 10.0 RC - ensure compatibility +- PlatformDetection class is in `tests/Aspire.Components.Common.TestUtilities/` diff --git a/tests/QuarantineTools.Tests/ActiveIssueTests.cs b/tests/QuarantineTools.Tests/ActiveIssueTests.cs new file mode 100644 index 00000000000..bf0afcf02cd --- /dev/null +++ b/tests/QuarantineTools.Tests/ActiveIssueTests.cs @@ -0,0 +1,483 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Xunit; +using System.Text.RegularExpressions; + +namespace QuarantineTools.Tests; + +public class ActiveIssueTests +{ + [Theory] + [InlineData( + """ + namespace N1.N2; + using Xunit; + public class C { + [Fact] + public void M() { } + } + """, + "N1.N2.C.M", + "https://github.com/dotnet/aspire/issues/1")] + [InlineData( + """ + namespace N1.N2 + { + using Xunit; + public class Outer { + public class Inner { + [Fact] + public void M() { } + } + } + } + """, + "N1.N2.Outer+Inner.M", + "https://github.com/dotnet/aspire/issues/2")] + public void ActiveIssue_AddsAttribute_WhenMissing(string code, string fullName, string issue) + { + var updated = AddActiveIssue(fullName, issue, code); + Assert.Contains("ActiveIssue", updated); + Assert.Contains(issue, updated); + // Attribute should be applied at method level and not duplicate facts + Assert.Contains("[Fact]", updated); + } + + [Fact] + public void ActiveIssue_IsIdempotent_DoesNotDuplicateOrChangeReason() + { + const string originalUrl = "https://github.com/dotnet/aspire/issues/100"; + const string newUrl = "https://github.com/dotnet/aspire/issues/200"; + const string code = """ + namespace N; + using Xunit; + public class C { + [Fact] + [ActiveIssue("https://github.com/dotnet/aspire/issues/100")] + public void M() { } + } + """; + + var updated = AddActiveIssue("N.C.M", newUrl, code); + var count = Regex.Matches(updated, "ActiveIssue").Count; + Assert.Equal(1, count); + Assert.Contains(originalUrl, updated); + Assert.DoesNotContain(newUrl, updated); + } + + [Fact] + public void ActiveIssue_RemovesAttribute_WhenPresent() + { + const string code = """ + namespace N; + using Xunit; + public class C { + [Fact] + [ActiveIssue("https://github.com/dotnet/aspire/issues/3")] + public void M() { } + } + """; + + var tree = CSharpSyntaxTree.ParseText(code); + var root = tree.GetCompilationUnitRoot(); + var method = root.DescendantNodes().OfType().Single(m => m.Identifier.ValueText == "M"); + var updated = RemoveActiveIssueAttribute(method, out var removed); + Assert.True(removed); + var newRoot = root.ReplaceNode(method, updated); + var text = newRoot.ToFullString(); + Assert.DoesNotContain("ActiveIssue", text); + Assert.Contains("[Fact]", text); + } + + [Fact] + public void ActiveIssue_AddsUsingDirective_WhenMissing() + { + const string code = """ + namespace N; + public class C { public void M() { } } + """; + + var updated = AddActiveIssue("N.C.M", "https://github.com/dotnet/aspire/issues/500", code); + Assert.Contains("using Xunit;", updated); + Assert.Contains("[ActiveIssue(\"https://github.com/dotnet/aspire/issues/500\")]", updated); + } + + [Fact] + public void ActiveIssue_DoesNotDuplicateUsingDirective_WhenAlreadyPresent() + { + const string code = """ + namespace N; + using Xunit; + public class C { + [Fact] + public void M() { } + } + """; + + var updated = AddActiveIssue("N.C.M", "https://github.com/dotnet/aspire/issues/501", code); + var norm = NormalizeNewlines(updated); + var count = Regex.Matches(norm, "using Xunit;").Count; + Assert.Equal(1, count); + } + + [Fact] + public void ActiveIssue_WithConditionalArguments() + { + const string code = """ + namespace N; + using Xunit; + public class C { + [Fact] + public void M() { } + } + """; + + // ActiveIssue can have conditional arguments like typeof(PlatformDetection), nameof(PlatformDetection.IsRunningFromAzdo) + // For simplicity, the tool adds just the URL, but the attribute should support additional arguments + var updated = AddActiveIssue("N.C.M", "https://github.com/dotnet/aspire/issues/11820", code); + Assert.Contains("[ActiveIssue(\"https://github.com/dotnet/aspire/issues/11820\")]", updated); + } + + [Fact] + public void ActiveIssue_Multiple_Targets_SameFile_UsingDirective_AddedOnce() + { + const string code = """ + namespace N; + public class C { + public void M1() { } + public void M2() { } + } + """; + + var updated1 = AddActiveIssue("N.C.M1", "https://github.com/dotnet/aspire/issues/11", code); + var updated2 = AddActiveIssue("N.C.M2", "https://github.com/dotnet/aspire/issues/12", updated1); + var norm = NormalizeNewlines(updated2); + // Only one using should be present + var count = Regex.Matches(norm, "using Xunit;").Count; + Assert.Equal(1, count); + // Both methods should have ActiveIssue + Assert.Contains("M1", updated2); + Assert.Contains("M2", updated2); + var activeIssueCount = Regex.Matches(norm, @"\[ActiveIssue\(").Count; + Assert.Equal(2, activeIssueCount); + } + + [Fact] + public void ActiveIssue_DoesNotInsertBlankLine_BetweenAttributes() + { + const string code = """ + namespace N; + using Xunit; + public class C { + [Fact] + public void M() { } + } + """; + + var updated = AddActiveIssue("N.C.M", "https://github.com/dotnet/aspire/issues/99", code); + var norm = NormalizeNewlines(updated); + // Ensure there is no blank line between [Fact] and [ActiveIssue] + Assert.DoesNotMatch(new Regex(@"\[Fact\]\n\s*\n\s*\[ActiveIssue", RegexOptions.Multiline), norm); + // But [Fact] followed by [ActiveIssue(...)] on the next line should exist (ignore indentation) + Assert.Matches(new Regex(@"\[Fact\]\n\s+\[ActiveIssue\(""https://github.com/dotnet/aspire/issues/99""\)\]", RegexOptions.Multiline), norm); + } + + [Fact] + public void ActiveIssue_RecognizesAttributeWithOrWithoutSuffix() + { + const string codeWithSuffix = """ + namespace N; + using Xunit; + public class C { + [Fact] + [ActiveIssueAttribute("https://github.com/dotnet/aspire/issues/123")] + public void M() { } + } + """; + + const string codeWithoutSuffix = """ + namespace N; + using Xunit; + public class C { + [Fact] + [ActiveIssue("https://github.com/dotnet/aspire/issues/124")] + public void M() { } + } + """; + + // Both should be recognized as already having the attribute + var updatedWithSuffix = AddActiveIssue("N.C.M", "https://github.com/dotnet/aspire/issues/999", codeWithSuffix); + var updatedWithoutSuffix = AddActiveIssue("N.C.M", "https://github.com/dotnet/aspire/issues/999", codeWithoutSuffix); + + // Should not add another attribute (idempotent) + var countWithSuffix = Regex.Matches(updatedWithSuffix, @"ActiveIssue").Count; + var countWithoutSuffix = Regex.Matches(updatedWithoutSuffix, @"ActiveIssue").Count; + + Assert.Equal(1, countWithSuffix); + Assert.Equal(1, countWithoutSuffix); + } + + [Fact] + public void ActiveIssue_RemovalDoesNotAffectOtherAttributes() + { + const string code = """ + namespace N; + using Xunit; + public class C { + [Fact] + [ActiveIssue("https://github.com/dotnet/aspire/issues/3")] + [Trait("Category", "Integration")] + public void M() { } + } + """; + + var tree = CSharpSyntaxTree.ParseText(code); + var root = tree.GetCompilationUnitRoot(); + var method = root.DescendantNodes().OfType().Single(m => m.Identifier.ValueText == "M"); + var updated = RemoveActiveIssueAttribute(method, out var removed); + Assert.True(removed); + var newRoot = root.ReplaceNode(method, updated); + var text = newRoot.ToFullString(); + + Assert.DoesNotContain("ActiveIssue", text); + Assert.Contains("[Fact]", text); + Assert.Contains("[Trait(\"Category\", \"Integration\")]", text); + } + + [Fact] + public void ActiveIssue_CanCoexist_WithQuarantinedTest() + { + // Verify that having both attribute types in the same file works correctly + const string code = """ + namespace N; + using Xunit; + using Aspire.TestUtilities; + public class C { + [Fact] + [QuarantinedTest("https://github.com/dotnet/aspire/issues/1")] + public void M1() { } + + [Fact] + public void M2() { } + } + """; + + var updated = AddActiveIssue("N.C.M2", "https://github.com/dotnet/aspire/issues/2", code); + + // Should have both attributes for different methods + Assert.Contains("QuarantinedTest", updated); + Assert.Contains("ActiveIssue", updated); + Assert.Contains("using Xunit;", updated); + Assert.Contains("using Aspire.TestUtilities;", updated); + } + + private static string AddActiveIssue(string fullMethodName, string issueUrl, string code) + { + var (pathParts, methodName) = ParseFullMethodName(fullMethodName); + var tree = CSharpSyntaxTree.ParseText(code); + var root = tree.GetCompilationUnitRoot(); + var methodNodes = root.DescendantNodes().OfType().Where(m => m.Identifier.ValueText == methodName).ToList(); + + foreach (var method in methodNodes) + { + var (ns, typeChain) = GetEnclosingNames(method); + var actualParts = new List(); + if (!string.IsNullOrEmpty(ns)) + { + actualParts.AddRange(ns.Split('.', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries)); + } + actualParts.AddRange(typeChain); + if (!SequenceEquals(actualParts, pathParts)) + { + continue; + } + + var updated = AddActiveIssueAttribute(method, issueUrl); + root = root.ReplaceNode(method, updated); + // Simulate the tool adding using directive for Xunit + root = EnsureUsingDirective(root, "Xunit"); + break; + } + + return root.ToFullString(); + } + + // Helpers copied from the script to validate logic + private static (List PathPartsBeforeMethod, string Method) ParseFullMethodName(string input) + { + var parts = input.Split('.', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries); + var method = parts[^1]; + var beforeMethod = parts.Take(parts.Length - 1) + .SelectMany(p => p.Split('+', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries)) + .ToList(); + return (beforeMethod, method); + } + + private static (string Namespace, List TypeChain) GetEnclosingNames(SyntaxNode node) + { + var typeNames = new List(); + string ns = string.Empty; + + for (var current = node.Parent; current != null; current = current.Parent) + { + switch (current) + { + case ClassDeclarationSyntax cd: + typeNames.Insert(0, cd.Identifier.ValueText); + break; + case StructDeclarationSyntax sd: + typeNames.Insert(0, sd.Identifier.ValueText); + break; + case RecordDeclarationSyntax rd: + typeNames.Insert(0, rd.Identifier.ValueText); + break; + case InterfaceDeclarationSyntax id: + typeNames.Insert(0, id.Identifier.ValueText); + break; + case NamespaceDeclarationSyntax nd: + ns = nd.Name.ToString(); + current = null; + break; + case FileScopedNamespaceDeclarationSyntax fsn: + ns = fsn.Name.ToString(); + current = null; + break; + } + if (current == null) + { + break; + } + } + return (ns, typeNames); + } + + private static bool SequenceEquals(List a, List b) + { + if (a.Count != b.Count) + { + return false; + } + for (int i = 0; i < a.Count; i++) + { + if (!string.Equals(a[i], b[i], StringComparison.Ordinal)) + { + return false; + } + } + return true; + } + + private static bool IsActiveIssueAttribute(AttributeSyntax attr) + { + string lastId = attr.Name switch + { + IdentifierNameSyntax ins => ins.Identifier.ValueText, + QualifiedNameSyntax qns => (qns.Right as IdentifierNameSyntax)?.Identifier.ValueText ?? qns.Right.ToString(), + AliasQualifiedNameSyntax aqn => (aqn.Name as IdentifierNameSyntax)?.Identifier.ValueText ?? aqn.Name.ToString(), + _ => attr.Name.ToString().Split('.').Last() + }; + return string.Equals(lastId, "ActiveIssue", StringComparison.Ordinal) + || string.Equals(lastId, "ActiveIssueAttribute", StringComparison.Ordinal); + } + + private static MethodDeclarationSyntax RemoveActiveIssueAttribute(MethodDeclarationSyntax method, out bool removed) + { + removed = false; + if (method.AttributeLists.Count == 0) + { + return method; + } + + var newLists = new List(); + foreach (var list in method.AttributeLists) + { + var remaining = list.Attributes.Where(a => !IsActiveIssueAttribute(a)).ToList(); + if (remaining.Count == list.Attributes.Count) + { + newLists.Add(list); + continue; + } + removed = true; + if (remaining.Count > 0) + { + var newList = list.WithAttributes(SyntaxFactory.SeparatedList(remaining)); + newLists.Add(newList); + } + } + + return removed ? method.WithAttributeLists(SyntaxFactory.List(newLists)) : method; + } + + private static MethodDeclarationSyntax AddActiveIssueAttribute(MethodDeclarationSyntax method, string issueUrl) + { + foreach (var list in method.AttributeLists) + { + if (list.Attributes.Any(IsActiveIssueAttribute)) + { + return method; + } + } + // Use short attribute name (ActiveIssue, not ActiveIssueAttribute) + var attrName = SyntaxFactory.ParseName("ActiveIssue"); + var attrArgs = string.IsNullOrWhiteSpace(issueUrl) + ? null + : SyntaxFactory.AttributeArgumentList( + SyntaxFactory.SingletonSeparatedList( + SyntaxFactory.AttributeArgument( + SyntaxFactory.LiteralExpression(SyntaxKind.StringLiteralExpression, SyntaxFactory.Literal(issueUrl))))); + + var attr = SyntaxFactory.Attribute(attrName, attrArgs); + var newList = SyntaxFactory.AttributeList(SyntaxFactory.SingletonSeparatedList(attr)); + + if (method.AttributeLists.Count > 0) + { + // Append after existing attributes ensuring exactly one newline between them. + var last = method.AttributeLists[method.AttributeLists.Count - 1]; + var indentation = SyntaxFactory.TriviaList(last.GetLeadingTrivia().Where(t => !t.IsKind(SyntaxKind.EndOfLineTrivia))); + bool lastEndsWithNewline = last.GetTrailingTrivia().Any(t => t.IsKind(SyntaxKind.EndOfLineTrivia)); + var leading = lastEndsWithNewline + ? indentation + : indentation.Add(SyntaxFactory.EndOfLine("\n")); + newList = newList + .WithLeadingTrivia(leading) + .WithTrailingTrivia(SyntaxFactory.EndOfLine("\n")); + } + else + { + var leading = method.GetLeadingTrivia(); + newList = newList.WithLeadingTrivia(leading) + .WithTrailingTrivia(SyntaxFactory.EndOfLine("\n")); + } + + var newLists = method.AttributeLists.Add(newList); + var updated = method.WithAttributeLists(newLists); + return updated; + } + + private static CompilationUnitSyntax EnsureUsingDirective(CompilationUnitSyntax root, string namespaceName) + { + // Check if using already exists anywhere in the tree (compilation unit or namespace level) + var allUsings = root.DescendantNodes().OfType() + .Where(u => u.Name != null && u.Name.ToString() == namespaceName); + + if (allUsings.Any()) + { + return root; + } + + var usingDirective = SyntaxFactory.UsingDirective(SyntaxFactory.ParseName(namespaceName)) + .WithUsingKeyword( + SyntaxFactory.Token(SyntaxKind.UsingKeyword) + .WithTrailingTrivia(SyntaxFactory.Space)) + .WithSemicolonToken( + SyntaxFactory.Token(SyntaxKind.SemicolonToken) + .WithTrailingTrivia(SyntaxFactory.EndOfLine("\n"))); + return root.WithUsings(root.Usings.Add(usingDirective)); + } + + private static string NormalizeNewlines(string text) => text.Replace("\r\n", "\n"); +} diff --git a/tools/QuarantineTools/Quarantine.cs b/tools/QuarantineTools/Quarantine.cs index f4e666ff8d0..d31cddc433e 100644 --- a/tools/QuarantineTools/Quarantine.cs +++ b/tools/QuarantineTools/Quarantine.cs @@ -39,18 +39,20 @@ public class Program { - private const string DefaultAttributeFullName = "Aspire.TestUtilities.QuarantinedTest"; + private const string DefaultQuarantinedTestAttributeFullName = "Aspire.TestUtilities.QuarantinedTest"; + private const string DefaultActiveIssueAttributeFullName = "Xunit.ActiveIssueAttribute"; public static Task Main(string[] args) { - var rootCommand = new RootCommand("Quarantine or unquarantine xUnit tests by adding/removing a QuarantinedTest attribute."); + var rootCommand = new RootCommand("Quarantine or unquarantine xUnit tests by adding/removing a QuarantinedTest or ActiveIssue attribute."); var optQuarantine = new Option("--quarantine", "-q") { Description = "Quarantine the specified test(s)." }; var optUnquarantine = new Option("--unquarantine", "-u") { Description = "Unquarantine the specified test(s)." }; var optUrl = new Option("--url", "-i") { Description = "Issue URL required for quarantining (http/https)." }; var optRoot = new Option("--root", "-r") { Description = "Tests root to scan (defaults to '/tests')." }; - var optAttribute = new Option("--attribute", "-a") { Description = "Fully-qualified attribute type to add/remove (default: Aspire.TestUtilities.QuarantinedTest)." }; - optAttribute.DefaultValueFactory = _ => DefaultAttributeFullName; + var optAttribute = new Option("--attribute", "-a") { Description = "Fully-qualified attribute type to add/remove. If not specified, defaults based on --mode." }; + var optMode = new Option("--mode", "-m") { Description = "Mode: 'quarantine' for QuarantinedTest or 'activeissue' for ActiveIssue (default: quarantine)." }; + optMode.DefaultValueFactory = _ => "quarantine"; var argTests = new Argument("tests") { Arity = ArgumentArity.ZeroOrMore, Description = "Fully-qualified test method name(s) like Namespace.Type.Method" }; @@ -59,6 +61,7 @@ public static Task Main(string[] args) rootCommand.Options.Add(optUrl); rootCommand.Options.Add(optRoot); rootCommand.Options.Add(optAttribute); + rootCommand.Options.Add(optMode); rootCommand.Arguments.Add(argTests); rootCommand.SetAction(static (parseResult, token) => @@ -82,7 +85,23 @@ public static Task Main(string[] args) var issueUrl = parseResult.GetValue("--url"); var scanRoot = parseResult.GetValue("--root"); - var attributeFullName = parseResult.GetValue("--attribute") ?? DefaultAttributeFullName; + var mode = parseResult.GetValue("--mode") ?? "quarantine"; + var attributeFullName = parseResult.GetValue("--attribute"); + + // Validate mode + if (mode != "quarantine" && mode != "activeissue") + { + Console.Error.WriteLine("Mode must be 'quarantine' or 'activeissue'."); + return Task.FromResult(1); + } + + // If attribute not explicitly provided, use default based on mode + if (string.IsNullOrWhiteSpace(attributeFullName)) + { + attributeFullName = mode == "activeissue" + ? DefaultActiveIssueAttributeFullName + : DefaultQuarantinedTestAttributeFullName; + } if (quarantine) { @@ -104,7 +123,7 @@ public static Task Main(string[] args) tests.ToList(), string.IsNullOrWhiteSpace(issueUrl) ? null : issueUrl, scanRoot, - string.IsNullOrWhiteSpace(attributeFullName) ? DefaultAttributeFullName : attributeFullName, + attributeFullName, token); }); diff --git a/tools/QuarantineTools/README.md b/tools/QuarantineTools/README.md index 0d6295cc25a..1c151951ed9 100644 --- a/tools/QuarantineTools/README.md +++ b/tools/QuarantineTools/README.md @@ -1,9 +1,9 @@ # Quarantine Tools -Roslyn based utility to add or remove `[QuarantinedTest]` attributes on test methods. +Roslyn based utility to add or remove `[QuarantinedTest]` or `[ActiveIssue]` attributes on test methods. -- Quarantine: adds `[QuarantinedTest("")]` -- Unquarantine: removes any `QuarantinedTest` attribute from the method +- Quarantine: adds `[QuarantinedTest("")]` or `[ActiveIssue("")]` +- Unquarantine: removes any `QuarantinedTest` or `ActiveIssue` attribute from the method ## Prerequisites @@ -15,16 +15,22 @@ Roslyn based utility to add or remove `[QuarantinedTest]` attributes on test met # Show help dotnet run --project tools/QuarantineTools -- --help -# Quarantine a test (adds attribute with issue URL) +# Quarantine a test with QuarantinedTest (default mode) dotnet run --project tools/QuarantineTools -- -q -i https://github.com/dotnet/aspire/issues/1234 Full.Namespace.Type.Method -# Unquarantine a test (removes attribute) +# Disable a test with ActiveIssue attribute +dotnet run --project tools/QuarantineTools -- -q -m activeissue -i https://github.com/dotnet/aspire/issues/1234 Full.Namespace.Type.Method + +# Unquarantine a test with QuarantinedTest (removes attribute) dotnet run --project tools/QuarantineTools -- -u Full.Namespace.Type.Method +# Re-enable a test with ActiveIssue (removes attribute) +dotnet run --project tools/QuarantineTools -- -u -m activeissue Full.Namespace.Type.Method + # Specify a custom tests root folder (defaults to /tests) dotnet run --project tools/QuarantineTools -- -q -r tests/MySubset -i https://github.com/org/repo/issues/1 N1.N2.C.M -# Use a custom attribute full name (defaults to Aspire.TestUtilities.QuarantinedTest) +# Use a custom attribute full name (overrides mode default) dotnet run --project tools/QuarantineTools -- -q -a MyCompany.Testing.CustomQuarantinedTest -i https://example.com/issue/1 N1.N2.C.M ``` @@ -32,7 +38,10 @@ dotnet run --project tools/QuarantineTools -- -q -a MyCompany.Testing.CustomQuar - `-q`/`--quarantine` and `-u`/`--unquarantine` are mutually exclusive (pick one). - `-i`/`--url ` is required when using `-q`. -- `-a`/`--attribute ` controls which attribute is added/removed. The tool accepts name with or without the `Attribute` suffix. If a namespace is supplied, the tool adds a `using` directive when inserting the short attribute name. +- `-m`/`--mode ` controls which attribute to add/remove: + - `quarantine` (default): Uses `Aspire.TestUtilities.QuarantinedTest` + - `activeissue`: Uses `Xunit.ActiveIssueAttribute` +- `-a`/`--attribute ` overrides the default attribute based on mode. The tool accepts name with or without the `Attribute` suffix. If a namespace is supplied, the tool adds a `using` directive when inserting the short attribute name. - `-r`/`--root ` overrides the scan root (by default, `/tests`). Relative paths are resolved against the repository root. - `-h`/`--help` prints usage information and exits. @@ -41,3 +50,5 @@ dotnet run --project tools/QuarantineTools -- -q -a MyCompany.Testing.CustomQuar - Methods are matched by fully-qualified name: `Namespace.Type.Method`. Nested types can use `+`, e.g. `Namespace.Outer+Inner.TestMethod`. - The tool scans the repo `tests/` folder and edits files in place, ignoring `bin/` and `obj/`. - Quarantine is idempotent (won't duplicate attributes); unquarantine only removes the attribute if present. +- When using `activeissue` mode, the tool adds `using Xunit;` if needed. +- When using `quarantine` mode (default), the tool adds `using Aspire.TestUtilities;` if needed. From 9ffff11ae7d14e9bc985817b2329092a5be2cb9f Mon Sep 17 00:00:00 2001 From: Ankit Jain Date: Fri, 31 Oct 2025 15:48:35 -0400 Subject: [PATCH 2/2] Add new test-disabler agent --- .github/agents/test-disabler.md | 197 ++++++++++++++++++++------------ 1 file changed, 126 insertions(+), 71 deletions(-) diff --git a/.github/agents/test-disabler.md b/.github/agents/test-disabler.md index e370ee0ff8c..7521ba1573f 100644 --- a/.github/agents/test-disabler.md +++ b/.github/agents/test-disabler.md @@ -1,23 +1,32 @@ --- name: test-disabler -description: Disables flaky or problematic tests by adding the [ActiveIssue] attribute with issue URLs -tools: ["read", "search", "edit", "bash", "create"] +description: Quarantines or disables flaky/problematic tests using the QuarantineTools utility +tools: ["bash", "view", "edit"] --- -You are a specialized test management agent for the dotnet/aspire repository. Your primary function is to disable broken tests by adding the `[ActiveIssue]` attribute with appropriate issue URLs. +You are a specialized test management agent for the dotnet/aspire repository. Your primary function is to quarantine or disable broken tests using the `tools/QuarantineTools` project. ## Understanding User Requests Parse user requests to extract: -1. **Test method name(s)** - the exact test method to disable -2. **Issue URL(s)** - GitHub issue URL(s) explaining why the test is being disabled -3. **Optional: Conditional clause** - platform detection conditions (e.g., "only on Azure DevOps") +1. **Test method name(s)** - the fully-qualified test method name(s) (Namespace.Type.Method) +2. **Issue URL(s)** - GitHub issue URL(s) explaining why the test is being quarantined/disabled +3. **Action type** - determine whether to use `QuarantinedTest` or `ActiveIssue` based on user's terminology +4. **Optional: Conditional clause** - platform detection conditions (e.g., "only on Azure DevOps") + +### Action Type Determination + +- **Use ActiveIssue** (`-m activeissue`) when user says: "disable", "enable", "re-enable" +- **Use QuarantinedTest** (default) when user says: "quarantine", "unquarantine" ### Example Requests -**Simple:** +**Disable with ActiveIssue:** > Disable CliOrphanDetectorAfterTheProcessWasRunningForAWhileThenStops with https://github.com/dotnet/aspire/issues/12314 +**Quarantine with QuarantinedTest:** +> Quarantine HealthChecksRegistersHealthCheckService with https://github.com/dotnet/aspire/issues/11820 + **Multiple tests:** > Disable these tests: > - HealthChecksRegistersHealthCheckService - https://github.com/dotnet/aspire/issues/11820 @@ -31,36 +40,49 @@ Parse user requests to extract: ### 1. Parse and Extract Information From the user's request, identify: -- Test method name(s) +- Test method name(s) - must be fully-qualified (Namespace.Type.Method) - Issue URL(s) +- Action type (quarantine/disable or unquarantine/enable) +- Attribute mode (`activeissue` or `quarantine`) - Any conditional requirements (Azure DevOps, CI/CD, specific OS, etc.) -### 2. Locate Test Methods +### 2. Locate Test Methods to Get Fully-Qualified Names -For each test method: +If the user provides only the method name without namespace/type, search for it: ```bash # Search for the test method in tests directory grep -r "public.*void.*TestMethodName\|public.*async.*Task.*TestMethodName" tests/ --include="*.cs" ``` -Record: -- File path -- Line number -- Test project name +Once located, determine the fully-qualified name (Namespace.Type.Method) by examining the file structure. -### 3. Determine Attribute Format +### 3. Run QuarantineTools for Each Test -**Simple (always disabled):** -```csharp -[ActiveIssue("https://github.com/dotnet/aspire/issues/12314")] +For **quarantining/disabling** tests, run QuarantineTools once per test: + +```bash +# For ActiveIssue (disable/enable terminology) +dotnet run --project tools/QuarantineTools -- -q -m activeissue -i + +# For QuarantinedTest (quarantine/unquarantine terminology) +dotnet run --project tools/QuarantineTools -- -q -i ``` -**Conditional (platform-specific):** -```csharp -[ActiveIssue("https://github.com/dotnet/aspire/issues/11820", typeof(PlatformDetection), nameof(PlatformDetection.IsRunningFromAzdo))] +For **unquarantining/re-enabling** tests: + +```bash +# For ActiveIssue +dotnet run --project tools/QuarantineTools -- -u -m activeissue + +# For QuarantinedTest +dotnet run --project tools/QuarantineTools -- -u ``` +### 4. Add Conditional Attributes (If Required) + +If the user specified conditional requirements (e.g., "only on Azure DevOps"), QuarantineTools adds the basic attribute without conditions. You must manually add the conditional parameters. + **Common PlatformDetection conditions:** - "on Azure DevOps" or "on CI" → `PlatformDetection.IsRunningFromAzdo` - "on build machines" → `PlatformDetection.IsRunningOnAzdoBuildMachine` @@ -68,25 +90,15 @@ Record: - "on Linux" → `PlatformDetection.IsLinux` - "on macOS" → `PlatformDetection.IsMacOS` -### 4. Add ActiveIssue Attribute - -**CRITICAL PLACEMENT:** Place the `[ActiveIssue]` attribute: -- **After** `[Fact]`, `[Theory]`, `[InlineData]`, `[RequiresDocker]`, etc. -- **Immediately before** the method declaration -- On its own line - -**Example for Fact test:** +**Steps to add conditions:** +1. QuarantineTools adds: `[ActiveIssue("https://github.com/dotnet/aspire/issues/12314")]` +2. Locate the file modified by QuarantineTools +3. Edit the attribute to add the conditional parameters: ```csharp -[Fact] -[RequiresDocker] -[ActiveIssue("https://github.com/dotnet/aspire/issues/12314")] -public async Task SomeTestMethod() -{ - // test code -} +[ActiveIssue("https://github.com/dotnet/aspire/issues/12314", typeof(PlatformDetection), nameof(PlatformDetection.IsRunningFromAzdo))] ``` -**Example for Theory test:** +**Example for Theory test with condition:** ```csharp [Theory] [InlineData(true)] @@ -117,7 +129,7 @@ Expected output should indicate the test is **Skipped** (not Passed or Failed). ### 6. Handle Errors Gracefully -If a test method is not found: +If QuarantineTools reports the test method is not found: - Continue processing remaining tests - Track the failure reason - Include in the PR description @@ -129,37 +141,48 @@ If build fails: ### 7. Create Commit and Pull Request -**Commit message format:** +**Commit message format (for quarantine/disable):** ``` -Disable flaky test(s) with ActiveIssue attribute +{Quarantine|Disable} flaky test(s) -- Disabled: TestMethod1 -- Disabled: TestMethod2 +- {Quarantined|Disabled}: TestMethod1 +- {Quarantined|Disabled}: TestMethod2 - Issue: https://github.com/dotnet/aspire/issues/XXXXX -These tests are being disabled due to {brief reason from issue}. +These tests are being {quarantined|disabled} due to {brief reason from issue}. +``` + +**Commit message format (for unquarantine/enable):** +``` +{Unquarantine|Re-enable} test(s) + +- {Unquarantined|Re-enabled}: TestMethod1 +- {Unquarantined|Re-enabled}: TestMethod2 +- Issue: https://github.com/dotnet/aspire/issues/XXXXX + +These tests are being {unquarantined|re-enabled} as the underlying issue has been resolved. ``` **PR Title:** ``` -Disable flaky test(s): {ShortTestName} +{Quarantine|Disable|Unquarantine|Re-enable} flaky test(s): {ShortTestName} ``` -**PR Description:** +**PR Description (for quarantine/disable):** ```markdown ## Summary -This PR disables the following test(s) by adding the `[ActiveIssue]` attribute: +This PR {quarantines|disables} the following test(s) by adding the `[{QuarantinedTest|ActiveIssue}]` attribute: -| Test Method | File | Issue | -|-------------|------|-------| +| Test Method | File | Issue | +|-------------|-----------------------------|--------| | TestMethod1 | tests/Project.Tests/File.cs | #XXXXX | | TestMethod2 | tests/Project.Tests/File.cs | #XXXXX | ## Changes -- Added `[ActiveIssue]` attribute to disable flaky/problematic tests -{- Conditional disabling on {Platform} only (if applicable)} +- Added `[{QuarantinedTest|ActiveIssue}]` attribute to {quarantine|disable} flaky/problematic tests +{- Conditional {quarantining|disabling} on {Platform} only (if applicable)} ## Verification @@ -175,17 +198,44 @@ Addresses #XXXXX **Note:** This PR does NOT close the related issue(s). The tests should be re-enabled once the underlying problems are fixed. ``` +**PR Description (for unquarantine/enable):** +```markdown +## Summary + +This PR {unquarantines|re-enables} the following test(s) by removing the `[{QuarantinedTest|ActiveIssue}]` attribute: + +| Test Method | File | Issue | +|-------------|------|-------| +| TestMethod1 | tests/Project.Tests/File.cs | #XXXXX | +| TestMethod2 | tests/Project.Tests/File.cs | #XXXXX | + +## Changes + +- Removed `[{QuarantinedTest|ActiveIssue}]` attribute to {unquarantine|re-enable} previously flaky tests + +## Verification + +✅ Built test project(s) successfully +✅ Verified test(s) run successfully + +## Related Issue + +Closes #XXXXX +``` + **PR Labels:** - `area-testing` -- `test-infrastructure` -**IMPORTANT:** Reference the issue using "Addresses #XXXXX" - do NOT use "Fixes" or "Closes" as the issue should remain open until the underlying problem is resolved and tests are re-enabled. +**IMPORTANT:** +- For quarantine/disable: Reference the issue using "Addresses #XXXXX" - do NOT use "Fixes" or "Closes" as the issue should remain open. +- For unquarantine/enable: Use "Closes #XXXXX" since the underlying problem has been resolved. ## Efficiency Optimizations ### Multiple Tests, Same Issue If multiple tests share the same issue: +- Run QuarantineTools once per test (tool does not support batch operations) - Process all tests together - Use a single commit - Single PR with all changes @@ -193,22 +243,22 @@ If multiple tests share the same issue: ### Batching Builds If multiple tests are in the same test project: -- Make all edits first -- Build once +- Run QuarantineTools for all tests first +- Build once after all modifications - Verify all tests in a single run ## Error Reporting -If any tests fail to be disabled, include in the PR description: +If any tests fail to be quarantined/disabled, include in the PR description: ```markdown -## ⚠️ Unable to Disable +## ⚠️ Unable to {Quarantine|Disable} -The following tests could not be disabled: +The following tests could not be {quarantined|disabled}: | Test Method | Reason | |-------------|--------| -| TestMethod | Test method not found in repository | +| TestMethod | Test method not found in repository (QuarantineTools exit code: X) | | TestMethod | Build failed after adding attribute | ``` @@ -217,14 +267,15 @@ The following tests could not be disabled: After completing the task, provide a summary: ```markdown -## Test Disabler Agent - Execution Summary +## Test Management Agent - Execution Summary -### ✅ Successfully Disabled +### ✅ Successfully {Quarantined|Disabled|Unquarantined|Re-enabled} - **TestMethod1** in `tests/Project.Tests/File.cs` - Issue: https://github.com/dotnet/aspire/issues/XXXXX + - Attribute: [{QuarantinedTest|ActiveIssue}] - Verification: Passed ✓ -### ❌ Failed to Disable +### ❌ Failed to {Quarantine|Disable|Unquarantine|Re-enable} - **TestMethod2** - Reason: {ErrorReason} @@ -235,23 +286,25 @@ After completing the task, provide a summary: ### 📊 Statistics - Total requested: {Total} -- Successfully disabled: {Success} +- Successfully {quarantined|disabled|unquarantined|re-enabled}: {Success} - Failed: {Failed} - Test projects modified: {ProjectCount} --- -**Note:** The related issue(s) remain open and should be closed once the underlying problems are fixed and tests are re-enabled. +**Note:** For quarantine/disable operations, the related issue(s) remain open and should be closed once the underlying problems are fixed and tests are re-enabled. ``` ## Important Constraints -- **Never modify test logic** - only add attributes -- **Never close the issue** - just reference it with "Addresses" -- **Always verify** - build and run tests -- **Preserve formatting** - follow .editorconfig rules -- **Issue URLs are mandatory** - never add `[ActiveIssue]` without a URL -- **Use surgical edits** - only change what's necessary -- **Follow repository conventions** - see .github/copilot-instructions.md for C# style +- **Use QuarantineTools** - always use `tools/QuarantineTools` to add/remove attributes, never manually edit +- **One test per QuarantineTools invocation** - call the tool once per test method +- **Never modify test logic** - only add/remove attributes via QuarantineTools +- **Never close the issue** for quarantine/disable operations - just reference it with "Addresses" +- **Close the issue** for unquarantine/enable operations - use "Closes" +- **Always verify** - build and run tests after QuarantineTools modifies files +- **Issue URLs are mandatory** - never quarantine/disable without a URL +- **Fully-qualified names required** - QuarantineTools needs Namespace.Type.Method format +- **Conditional attributes require manual editing** - QuarantineTools adds basic attributes only - **No placeholder values** - use actual issue numbers and test names ## Repository-Specific Notes @@ -260,5 +313,7 @@ After completing the task, provide a summary: - Test projects follow the naming pattern `ProjectName.Tests` - Use xUnit SDK v3 with Microsoft.Testing.Platform - Always exclude quarantined and outerloop tests during verification -- The repository uses .NET 10.0 RC - ensure compatibility - PlatformDetection class is in `tests/Aspire.Components.Common.TestUtilities/` +- QuarantineTools is located at `tools/QuarantineTools` and can be run via `dotnet run --project` +- QuarantineTools uses Roslyn to safely modify source files +- See `tools/QuarantineTools/README.md` for detailed tool documentation