diff --git a/doc/AG0042.md b/doc/AG0042.md index a2151d2..18d387e 100644 --- a/doc/AG0042.md +++ b/doc/AG0042.md @@ -1,61 +1,75 @@ -# AG0042: QuerySelector should not be used with Playwright +# AG0042: ElementHandle methods should not be used with Playwright ## Problem Description -Using `QuerySelectorAsync()` in Playwright tests can lead to brittle and unreliable tests. This method uses CSS selectors which can be fragile and may break when the UI structure changes. Instead, more reliable locator strategies like data-testid or role-based selectors should be used. +Using methods that return ElementHandle (such as `QuerySelectorAsync()`, `QuerySelectorAllAsync()`, `WaitForSelectorAsync()`, etc.) in Playwright tests can lead to brittle and unreliable tests. These methods often rely on CSS selectors which can be fragile and may break when the UI structure changes. Instead, more reliable locator strategies like data-testid or role-based selectors should be used. ## Rule Details -This rule raises an issue when `QuerySelectorAsync()` is called on Playwright `IPage` or `Page` objects. +This rule raises an issue when any method returning an `IElementHandle`, `ElementHandle`, or their nullable variants is called on Playwright `IPage`, `Page`, or `IElementHandle` objects. -### Noncompliant Code Example +### Noncompliant Code Examples ```csharp -public async Task ClickLoginButton(IPage page) +public async Task InteractWithElements(IPage page) { - // Noncompliant: Using QuerySelectorAsync with CSS selector + // Noncompliant: Using methods that return ElementHandle var loginButton = await page.QuerySelectorAsync(".login-button"); - await loginButton.ClickAsync(); + var menuItems = await page.QuerySelectorAllAsync(".menu-item"); + var dynamicElement = await page.WaitForSelectorAsync("#dynamic-content"); + + // Noncompliant: Chaining ElementHandle operations + var childElement = await loginButton.QuerySelectorAsync(".child"); } ``` ### Compliant Solution ```csharp -public async Task ClickLoginButton(IPage page) +public async Task InteractWithElements(IPage page) { // Compliant: Using Locator with data-testid - await page.Locator("[data-testid='login-button']").ClickAsync(); + await page.GetByTestId("login-button").ClickAsync(); // Compliant: Using role-based selector await page.GetByRole(AriaRole.Button, new() { Name = "Login" }).ClickAsync(); // Compliant: Using text content await page.GetByText("Login").ClickAsync(); + + // Compliant: Working with multiple elements + var menuItems = page.Locator("[data-testid='menu-item']"); + await menuItems.First.ClickAsync(); + + // Compliant: Waiting for elements + await page.Locator("#dynamic-content").WaitForAsync(); } ``` ## Why is this an Issue? -1. **Fragile Selectors**: CSS selectors are tightly coupled to the DOM structure and styling classes, making tests brittle when: - - CSS classes are renamed or removed - - DOM hierarchy changes - - Styling frameworks are updated +1. **ElementHandle Limitations**: ElementHandle references can become stale when the DOM changes, leading to flaky tests. -2. **Maintainability**: CSS selectors can be complex and hard to maintain, especially when dealing with nested elements or specific combinations of classes. +2. **Fragile Selectors**: CSS selectors are tightly coupled to the DOM structure and styling classes, making tests brittle when: + - CSS classes are renamed or removed + - DOM hierarchy changes + - Styling frameworks are updated -3. **Best Practices**: Playwright provides better alternatives that are: - - More resilient to changes - - More readable and maintainable - - Better aligned with testing best practices +3. **Maintainability**: ElementHandle-based selectors can be complex and hard to maintain, especially when dealing with nested elements. + +4. **Best Practices**: Playwright provides better alternatives through Locators that are: + - More resilient to changes + - More readable and maintainable + - Better aligned with testing best practices + - Auto-waiting and retry-able ## Better Alternatives -Playwright provides several better methods for selecting elements: +Playwright provides several better methods for selecting and interacting with elements: 1. **Data Test IDs**: ```csharp - await page.Locator("[data-testid='submit-button']").ClickAsync(); + await page.GetByTestId("submit-button").ClickAsync(); ``` 2. **Role-based Selectors**: @@ -75,49 +89,6 @@ Playwright provides several better methods for selecting elements: await page.GetByPlaceholder("Enter email").FillAsync("test@example.com"); ``` -## How to Fix It - -1. Replace `QuerySelectorAsync()` calls with more specific Playwright locators: - - ```csharp - // Before - var element = await page.QuerySelectorAsync(".submit-btn"); - - // After - var element = page.GetByRole(AriaRole.Button, new() { Name = "Submit" }); - ``` - -2. Add data-testid attributes to your application's elements: - ```html - - ``` - - ```csharp - await page.Locator("[data-testid='submit-button']").ClickAsync(); - ``` - -3. Use semantic HTML with ARIA roles and labels: - ```html - - ``` - - ```csharp - await page.GetByRole(AriaRole.Button, new() { Name = "Submit form" }).ClickAsync(); - ``` - -## Exceptions - -This rule might be relaxed in the following scenarios: -- Legacy test code that's pending migration -- Complex third-party components where other selectors aren't available -- Testing CSS-specific functionality - -## Benefits -- More reliable tests -- Better test maintenance -- Clearer test intentions -- Improved accessibility testing - ## References - [ElementHandle is Discouraged by official Documents](https://playwright.dev/dotnet/docs/api/class-elementhandle) - [Playwright Locators Documentation](https://playwright.dev/docs/locators) diff --git a/src/Agoda.Analyzers.Test/AgodaCustom/AG0042UnitTests.cs b/src/Agoda.Analyzers.Test/AgodaCustom/AG0042UnitTests.cs index 83a776a..c54b6d8 100644 --- a/src/Agoda.Analyzers.Test/AgodaCustom/AG0042UnitTests.cs +++ b/src/Agoda.Analyzers.Test/AgodaCustom/AG0042UnitTests.cs @@ -9,9 +9,144 @@ namespace Agoda.Analyzers.Test.AgodaCustom; class AG0042UnitTests : DiagnosticVerifier { - protected override DiagnosticAnalyzer DiagnosticAnalyzer => new AG0042QuerySelectorShouldNotBeUsed(); + protected override DiagnosticAnalyzer DiagnosticAnalyzer => new AG0042ElementHandlerShouldNotBeUsed(); - protected override string DiagnosticId => AG0042QuerySelectorShouldNotBeUsed.DIAGNOSTIC_ID; + protected override string DiagnosticId => AG0042ElementHandlerShouldNotBeUsed.DIAGNOSTIC_ID; + + [TestCase("QuerySelectorAsync")] + [TestCase("QuerySelectorAllAsync")] + [TestCase("WaitForSelectorAsync")] + public async Task AG0042_WhenUsingElementHandlerMethodWithPlaywrightPage_ShowWarning(string methodName) + { + var code = new CodeDescriptor + { + References = new[] { typeof(IPage).Assembly }, + Code = $@" + using System.Threading.Tasks; + using Microsoft.Playwright; + + class TestClass + {{ + public async Task TestMethod(IPage page) + {{ + await page.{methodName}(""#element""); + }} + }}" + }; + + await VerifyDiagnosticsAsync(code, new DiagnosticLocation(9, 35)); + } + + [Test] + public async Task AG0042_WhenUsingLocatorMethod_NoWarning() + { + var code = new CodeDescriptor + { + References = new[] { typeof(IPage).Assembly }, + Code = @" + using System.Threading.Tasks; + using Microsoft.Playwright; + + class TestClass + { + public void TestMethod(IPage page) + { + var element = page.Locator(""[data-testid='element']""); + element.GetByRole(AriaRole.Button).ClickAsync(); + } + }" + }; + + await VerifyDiagnosticsAsync(code, EmptyDiagnosticResults); + } + + [Test] + public async Task AG0042_WhenUsingMethodReturningElementHandleArray_ShowWarning() + { + var code = new CodeDescriptor + { + References = new[] { typeof(IPage).Assembly }, + Code = @" + using System.Threading.Tasks; + using Microsoft.Playwright; + + class TestClass + { + public async Task TestMethod(IPage page) + { + await page.QuerySelectorAllAsync(""#element""); + } + }" + }; + + await VerifyDiagnosticsAsync(code, new DiagnosticLocation(9, 27)); + } + + [Test] + public async Task AG0042_WhenUsingMethodReturningNonTaskType_NoWarning() + { + var code = new CodeDescriptor + { + References = new[] { typeof(IPage).Assembly }, + Code = @" + using System.Threading.Tasks; + using Microsoft.Playwright; + + class TestClass + { + public void TestMethod(IPage page) + { + var result = page.ToString(); + } + }" + }; + + await VerifyDiagnosticsAsync(code, EmptyDiagnosticResults); + } + + [Test] + public async Task AG0042_WhenUsingMethodWithGenericTaskButNotElementHandle_NoWarning() + { + var code = new CodeDescriptor + { + References = new[] { typeof(IPage).Assembly }, + Code = @" + using System.Threading.Tasks; + using Microsoft.Playwright; + + class TestClass + { + public async Task TestMethod(IPage page) + { + await page.EvaluateAsync(""() => 'test'""); + } + }" + }; + + await VerifyDiagnosticsAsync(code, EmptyDiagnosticResults); + } + + [Test] + public async Task AG0042_WhenMethodSymbolIsNull_NoWarning() + { + var code = new CodeDescriptor + { + Code = @" + using System.Threading.Tasks; + + class TestClass + { + public async Task TestMethod() + { + dynamic page = null; + // This will make methodSymbol null since it's dynamic + await page.NonExistentMethod(""#element""); + } + }" + }; + + await VerifyDiagnosticsAsync(code, EmptyDiagnosticResults); + } [Test] public async Task AG0042_WhenUsingQuerySelectorAsyncWithPlaywrightPage_ShowWarning() diff --git a/src/Agoda.Analyzers/AgodaCustom/AG0042ElementHandlerShouldNotBeUsed.cs b/src/Agoda.Analyzers/AgodaCustom/AG0042ElementHandlerShouldNotBeUsed.cs new file mode 100644 index 0000000..38912e4 --- /dev/null +++ b/src/Agoda.Analyzers/AgodaCustom/AG0042ElementHandlerShouldNotBeUsed.cs @@ -0,0 +1,134 @@ +using System.Collections.Immutable; +using Agoda.Analyzers.Helpers; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.Diagnostics; + +namespace Agoda.Analyzers.AgodaCustom +{ + [DiagnosticAnalyzer(LanguageNames.CSharp)] + public class AG0042ElementHandlerShouldNotBeUsed : DiagnosticAnalyzer + { + public const string DIAGNOSTIC_ID = "AG0042"; + + private static readonly LocalizableString Title = new LocalizableResourceString( + nameof(CustomRulesResources.AG0042Title), CustomRulesResources.ResourceManager, + typeof(CustomRulesResources)); + + private static readonly LocalizableString MessageFormat = new LocalizableResourceString( + nameof(CustomRulesResources.AG0042Title), CustomRulesResources.ResourceManager, + typeof(CustomRulesResources)); + + private static readonly LocalizableString Description + = DescriptionContentLoader.GetAnalyzerDescription(nameof(AG0042ElementHandlerShouldNotBeUsed)); + + public static readonly DiagnosticDescriptor Descriptor = new DiagnosticDescriptor( + DIAGNOSTIC_ID, + Title, + MessageFormat, + AnalyzerCategory.CustomQualityRules, + DiagnosticSeverity.Warning, + AnalyzerConstants.EnabledByDefault, + Description, + "https://github.com/agoda-com/AgodaAnalyzers/blob/master/doc/AG0042.md", + WellKnownDiagnosticTags.EditAndContinue); + + public override ImmutableArray SupportedDiagnostics => ImmutableArray.Create(Descriptor); + + public override void Initialize(AnalysisContext context) + { + context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); + context.EnableConcurrentExecution(); + context.RegisterSyntaxNodeAction(AnalyzeSyntaxNode, SyntaxKind.InvocationExpression); + } + + private static void AnalyzeSyntaxNode(SyntaxNodeAnalysisContext context) + { + var invocationExpression = (InvocationExpressionSyntax)context.Node; + + if (!(invocationExpression.Expression is MemberAccessExpressionSyntax memberAccess)) + return; + + var semanticModel = context.SemanticModel; + + // Check the caller type (should be IPage or IElementHandle) + var callerSymbolInfo = semanticModel.GetSymbolInfo(memberAccess.Expression); + var callerSymbol = callerSymbolInfo.Symbol; + if (callerSymbol == null) return; + + var callerTypeSymbol = GetTypeSymbol(callerSymbol); + if (callerTypeSymbol == null) return; + + if (!IsPlaywrightPageOrElementType(callerTypeSymbol)) return; + + // Check the method return type + var methodSymbol = (IMethodSymbol)semanticModel.GetSymbolInfo(invocationExpression).Symbol; + if (methodSymbol == null) return; + + // Get the return type + var returnType = methodSymbol.ReturnType; + if (!IsElementHandleReturnType(returnType)) return; + + var diagnostic = Diagnostic.Create(Descriptor, invocationExpression.GetLocation()); + context.ReportDiagnostic(diagnostic); + } + + private static bool IsElementHandleReturnType(ITypeSymbol returnType) + { + // Handle Task and Task + if (!(returnType is INamedTypeSymbol namedType) || !namedType.IsGenericType) + return false; + + var taskType = namedType.ConstructedFrom; + if (taskType.ToString() != "System.Threading.Tasks.Task" && taskType.ToString() != "System.Threading.Tasks.Task") + return false; + + var typeArg = namedType.TypeArguments[0]; + + // Handle nullable type + if (typeArg is INamedTypeSymbol nullableType && nullableType.IsGenericType) + { + typeArg = nullableType.TypeArguments[0]; + } + + return IsElementHandleType(typeArg); + + } + + private static bool IsElementHandleType(ITypeSymbol typeSymbol) + { + var typeName = typeSymbol.ToString(); + return typeName == "Microsoft.Playwright.IElementHandle" || + typeName == "Microsoft.Playwright.ElementHandle" || + typeName == "Microsoft.Playwright.IElementHandle?" || + typeName == "Microsoft.Playwright.ElementHandle?"; + } + + private static INamedTypeSymbol GetTypeSymbol(ISymbol symbol) + { + switch (symbol) + { + case ILocalSymbol localSymbol: + return localSymbol.Type as INamedTypeSymbol; + case IParameterSymbol parameterSymbol: + return parameterSymbol.Type as INamedTypeSymbol; + case IFieldSymbol fieldSymbol: + return fieldSymbol.Type as INamedTypeSymbol; + case IPropertySymbol propertySymbol: + return propertySymbol.Type as INamedTypeSymbol; + default: + return null; + } + } + + private static bool IsPlaywrightPageOrElementType(INamedTypeSymbol typeSymbol) + { + var typeName = typeSymbol.ToString(); + return typeName == "Microsoft.Playwright.IPage" || + typeName == "Microsoft.Playwright.Page" || + typeName == "Microsoft.Playwright.IElementHandle" || + typeName == "Microsoft.Playwright.ElementHandle"; + } + } +} \ No newline at end of file diff --git a/src/Agoda.Analyzers/AgodaCustom/AG0042QuerySelectorShouldNotBeUsed.cs b/src/Agoda.Analyzers/AgodaCustom/AG0042QuerySelectorShouldNotBeUsed.cs deleted file mode 100644 index 176f565..0000000 --- a/src/Agoda.Analyzers/AgodaCustom/AG0042QuerySelectorShouldNotBeUsed.cs +++ /dev/null @@ -1,97 +0,0 @@ -using System.Collections.Immutable; -using Agoda.Analyzers.Helpers; -using Microsoft.CodeAnalysis; -using Microsoft.CodeAnalysis.CSharp; -using Microsoft.CodeAnalysis.CSharp.Syntax; -using Microsoft.CodeAnalysis.Diagnostics; - -namespace Agoda.Analyzers.AgodaCustom -{ - [DiagnosticAnalyzer(LanguageNames.CSharp)] - public class AG0042QuerySelectorShouldNotBeUsed : DiagnosticAnalyzer - { - public const string DIAGNOSTIC_ID = "AG0042"; - - private static readonly LocalizableString Title = new LocalizableResourceString( - nameof(CustomRulesResources.AG0042Title), CustomRulesResources.ResourceManager, - typeof(CustomRulesResources)); - - private static readonly LocalizableString MessageFormat = new LocalizableResourceString( - nameof(CustomRulesResources.AG0042Title), CustomRulesResources.ResourceManager, - typeof(CustomRulesResources)); - - private static readonly LocalizableString Description - = DescriptionContentLoader.GetAnalyzerDescription(nameof(AG0042QuerySelectorShouldNotBeUsed)); - - public static readonly DiagnosticDescriptor Descriptor = new DiagnosticDescriptor( - DIAGNOSTIC_ID, - Title, - MessageFormat, - AnalyzerCategory.CustomQualityRules, - DiagnosticSeverity.Warning, - AnalyzerConstants.EnabledByDefault, - Description, - "https://github.com/agoda-com/AgodaAnalyzers/blob/master/doc/AG0042.md", - WellKnownDiagnosticTags.EditAndContinue); - - public override void Initialize(AnalysisContext context) - { - context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); - context.EnableConcurrentExecution(); - context.RegisterSyntaxNodeAction(AnalyzeSyntaxNode, SyntaxKind.InvocationExpression); - } - - public override ImmutableArray SupportedDiagnostics => ImmutableArray.Create(Descriptor); - - private static void AnalyzeSyntaxNode(SyntaxNodeAnalysisContext context) - { - var invocationExpression = (InvocationExpressionSyntax)context.Node; - - if (!(invocationExpression.Expression is MemberAccessExpressionSyntax memberAccess)) - return; - - var methodName = memberAccess.Name.Identifier.Text; - - if (methodName != "QuerySelectorAsync") - return; - - var semanticModel = context.SemanticModel; - - var symbolInfo = semanticModel.GetSymbolInfo(memberAccess.Expression, context.CancellationToken); - var symbol = symbolInfo.Symbol; - - if (symbol == null) - return; - - INamedTypeSymbol typeSymbol; - switch (symbol) - { - case ILocalSymbol localSymbol: - typeSymbol = localSymbol.Type as INamedTypeSymbol; - break; - case IParameterSymbol parameterSymbol: - typeSymbol = parameterSymbol.Type as INamedTypeSymbol; - break; - case IFieldSymbol fieldSymbol: - typeSymbol = fieldSymbol.Type as INamedTypeSymbol; - break; - case IPropertySymbol propertySymbol: - typeSymbol = propertySymbol.Type as INamedTypeSymbol; - break; - default: - typeSymbol = null; - break; - } - - if (typeSymbol == null) - return; - - if (typeSymbol.ToString() != "Microsoft.Playwright.IPage" && - typeSymbol.ToString() != "Microsoft.Playwright.Page") - return; - - var diagnostic = Diagnostic.Create(Descriptor, invocationExpression.GetLocation()); - context.ReportDiagnostic(diagnostic); - } - } -} \ No newline at end of file diff --git a/src/Agoda.Analyzers/AgodaCustom/CustomRulesResources.resx b/src/Agoda.Analyzers/AgodaCustom/CustomRulesResources.resx index b288e69..c861ae2 100644 --- a/src/Agoda.Analyzers/AgodaCustom/CustomRulesResources.resx +++ b/src/Agoda.Analyzers/AgodaCustom/CustomRulesResources.resx @@ -261,7 +261,7 @@ One exception is logging, where it can be useful to see the exact DC / cluster / You are using either an interpolated string or string concatenation in your logs, change these to the message template format to preserve structure in your logs - QuerySelectorAsync usage found Replace with Locator or GetByRole for more reliable tests + ElementHandler methods should not be used. Use ILocator based approaches instead. BuildServiceProvider detected in request processing code. This may cause memory leaks. Remove the BuildServiceProvider() call and let the framework manage the service provider lifecycle.