From 43abe5f30c07fa9e82e41f4ff615cd75ddb30118 Mon Sep 17 00:00:00 2001 From: Rannes Date: Wed, 30 Oct 2024 02:31:14 +0700 Subject: [PATCH 1/2] feat: add AG0044 rule --- doc/AG0044.md | 152 +++++++++++++++ .../AgodaCustom/AG0044UnitTests.cs | 181 ++++++++++++++++++ .../AG0044ForceOptionShouldNotBeUsed.cs | 91 +++++++++ .../CustomRulesResources.Designer.cs | 21 +- .../AgodaCustom/CustomRulesResources.resx | 9 + .../AnalyzerReleases.Unshipped.md | 1 + 6 files changed, 452 insertions(+), 3 deletions(-) create mode 100644 doc/AG0044.md create mode 100644 src/Agoda.Analyzers.Test/AgodaCustom/AG0044UnitTests.cs create mode 100644 src/Agoda.Analyzers/AgodaCustom/AG0044ForceOptionShouldNotBeUsed.cs diff --git a/doc/AG0044.md b/doc/AG0044.md new file mode 100644 index 0000000..4bb704f --- /dev/null +++ b/doc/AG0044.md @@ -0,0 +1,152 @@ +# AG0044: Force option should not be used in Locator methods + +## Problem Description +Using `Force = true` in Playwright's Locator methods bypasses important built-in validation checks. This can lead to brittle tests, inconsistent behavior, and difficulty in identifying real UI/UX issues. + +## Rule Details +This rule raises an issue when `Force = true` is set in options objects passed to ILocator action methods. + +### Noncompliant Code Examples + +#### Using Force in Click Actions +```csharp +public async Task ClickButtonAsync(ILocator button) +{ + // Noncompliant: Forces click without proper element state validation + await button.ClickAsync(new() { Force = true }); +} +``` + +#### Using Force in Multiple Actions +```csharp +public async Task FillFormAsync(ILocator form) +{ + var textbox = form.Locator(".textbox"); + var checkbox = form.Locator(".checkbox"); + var dropdown = form.Locator(".select"); + + // Noncompliant: Multiple forced interactions + await textbox.FillAsync("text", new() { Force = true }); + await checkbox.CheckAsync(new() { Force = true }); + await dropdown.SelectOptionAsync("option", new() { Force = true }); +} +``` + +#### Pre-defined Options Object +```csharp +public async Task InteractWithElementAsync(ILocator element) +{ + // Noncompliant: Force option in pre-defined options + var options = new LocatorClickOptions { Force = true }; + await element.ClickAsync(options); +} +``` + +### Compliant Solutions + +#### Using Default Behavior +```csharp +public async Task ClickButtonAsync(ILocator button) +{ + // Compliant: Uses Playwright's built-in waiting and validation + await button.ClickAsync(); +} +``` + +#### Using Proper Wait Strategies +```csharp +public async Task FillFormAsync(ILocator form) +{ + var textbox = form.Locator(".textbox"); + var checkbox = form.Locator(".checkbox"); + var dropdown = form.Locator(".select"); + + // Compliant: Uses appropriate waiting and state checks + await textbox.WaitForAsync(); + await textbox.FillAsync("text"); + + await checkbox.WaitForAsync(new() { State = WaitForSelectorState.Attached }); + await checkbox.CheckAsync(); + + await dropdown.WaitForAsync(new() { State = WaitForSelectorState.Visible }); + await dropdown.SelectOptionAsync("option"); +} +``` + +#### Using Custom Timeout When Needed +```csharp +public async Task InteractWithSlowElementAsync(ILocator element) +{ + // Compliant: Adjusts timeout instead of forcing interaction + await element.ClickAsync(new() { Timeout = 10000 }); +} +``` + +## Why is this an Issue? +1. **Bypasses Important Checks**: Force option bypasses Playwright's built-in validations for: + - Element visibility + - Element being attached to DOM + - Element being enabled + - Element being stable (not moving) + +2. **Masks Real Issues**: Using Force can hide actual problems in the application: + - Elements that are not properly visible to users + - Race conditions in UI rendering + - Timing issues in dynamic content loading + - Accessibility problems + +3. **Unreliable Tests**: Tests using Force: + - May pass locally but fail in CI/CD + - Don't accurately reflect real user interactions + - Can become flaky or inconsistent + - Are harder to debug when they fail + +## How to Fix It +1. **Use Proper Wait Strategies**: + ```csharp + // Instead of Force = true + await element.WaitForAsync(new() + { + State = WaitForSelectorState.Visible, + Timeout = 5000 + }); + await element.ClickAsync(); + ``` + +2. **Adjust Timeouts When Needed**: + ```csharp + await element.ClickAsync(new() { Timeout = 10000 }); + ``` + +3. **Use State Assertions**: + ```csharp + await element.WaitForAsync(new() { State = WaitForSelectorState.Enabled }); + await Assertions.Expect(element).ToBeVisibleAsync(); + await element.ClickAsync(); + ``` + +4. **Handle Dynamic Content**: + ```csharp + await page.WaitForLoadStateAsync(LoadState.NetworkIdle); + await element.WaitForAsync(); + await element.ClickAsync(); + ``` + +## Benefits +- More reliable and stable tests +- Better representation of real user interactions +- Easier debugging when tests fail +- Catches actual UI/UX issues +- More maintainable test code + +## Exceptions +Force option might be acceptable in very specific scenarios: +- Testing error cases where elements are intentionally obscured +- Dealing with known browser-specific edge cases +- Temporary workarounds while investigating underlying issues + +However, these should be rare exceptions and well-documented. + +## References +- [Playwright Actionability](https://playwright.dev/dotnet/docs/actionability) +- [Playwright Forcing Actions](https://playwright.dev/dotnet/docs/actionability#forcing-actions) diff --git a/src/Agoda.Analyzers.Test/AgodaCustom/AG0044UnitTests.cs b/src/Agoda.Analyzers.Test/AgodaCustom/AG0044UnitTests.cs new file mode 100644 index 0000000..a6ea1c9 --- /dev/null +++ b/src/Agoda.Analyzers.Test/AgodaCustom/AG0044UnitTests.cs @@ -0,0 +1,181 @@ +using System.Threading.Tasks; +using Agoda.Analyzers.AgodaCustom; +using Agoda.Analyzers.Test.Helpers; +using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.Playwright; +using NUnit.Framework; + +namespace Agoda.Analyzers.Test.AgodaCustom; + +class AG0044UnitTests : DiagnosticVerifier +{ + protected override DiagnosticAnalyzer DiagnosticAnalyzer => new AG0044ForceOptionShouldNotBeUsed(); + + protected override string DiagnosticId => AG0044ForceOptionShouldNotBeUsed.DIAGNOSTIC_ID; + + [Test] + public async Task AG0044_WhenUsingForceOptionInClickAsync_ShowWarning() + { + var code = new CodeDescriptor + { + References = new[] { typeof(ILocator).Assembly }, + Code = @" + using System.Threading.Tasks; + using Microsoft.Playwright; + + class TestClass + { + public async Task TestMethod(ILocator locator) + { + await locator.ClickAsync(new() { Force = true }); + } + }" + }; + + await VerifyDiagnosticsAsync(code, new DiagnosticLocation(9, 58)); + } + + [Test] + public async Task AG0044_WhenUsingPreDefinedForceOption_ShowWarning() + { + var code = new CodeDescriptor + { + References = new[] { typeof(ILocator).Assembly }, + Code = @" + using System.Threading.Tasks; + using Microsoft.Playwright; + + class TestClass + { + public async Task TestMethod(ILocator locator) + { + var options = new LocatorClickOptions { Force = true }; + await locator.ClickAsync(options); + } + }" + }; + + await VerifyDiagnosticsAsync(code, new DiagnosticLocation(9, 65)); + } + + [TestCase("HoverAsync", 58, TestName = "AG0044_WhenUsingForceInHoverAsync_ShowWarning")] + [TestCase("DblClickAsync", 61, TestName = "AG0044_WhenUsingForceInDblClickAsync_ShowWarning")] + [TestCase("TapAsync", 56, TestName = "AG0044_WhenUsingForceInTapAsync_ShowWarning")] + [TestCase("CheckAsync", 58, TestName = "AG0044_WhenUsingForceInCheckAsync_ShowWarning")] + [TestCase("UncheckAsync", 60, TestName = "AG0044_WhenUsingForceInUncheckAsync_ShowWarning")] + public async Task AG0044_WhenUsingForceOptionWithoutParams_ShowWarning(string methodName, int column) + { + var code = new CodeDescriptor + { + References = new[] { typeof(ILocator).Assembly }, + Code = $@" + using System.Threading.Tasks; + using Microsoft.Playwright; + + class TestClass + {{ + public async Task TestMethod(ILocator locator) + {{ + await locator.{methodName}(new() {{ Force = true }}); + }} + }}" + }; + + await VerifyDiagnosticsAsync(code, new DiagnosticLocation(9, column)); + } + + [TestCase("FillAsync", "\"text\"", 65, TestName = "AG0044_WhenUsingForceInFillAsync_ShowWarning")] + [TestCase("SelectOptionAsync", "\"option\"", 75, TestName = "AG0044_WhenUsingForceInSelectOptionAsync_ShowWarning")] + public async Task AG0044_WhenUsingForceOptionWithParams_ShowWarning(string methodName, string parameter, int column) + { + var code = new CodeDescriptor + { + References = new[] { typeof(ILocator).Assembly }, + Code = $@" + using System.Threading.Tasks; + using Microsoft.Playwright; + + class TestClass + {{ + public async Task TestMethod(ILocator locator) + {{ + await locator.{methodName}({parameter}, new() {{ Force = true }}); + }} + }}" + }; + + await VerifyDiagnosticsAsync(code, new DiagnosticLocation(9, column)); + } + + [Test] + public async Task AG0044_WhenNotUsingForceOption_NoWarning() + { + var code = new CodeDescriptor + { + References = new[] { typeof(ILocator).Assembly }, + Code = @" + using System.Threading.Tasks; + using Microsoft.Playwright; + + class TestClass + { + public async Task TestMethod(ILocator locator) + { + await locator.ClickAsync(); + await locator.ClickAsync(new() { Timeout = 5000 }); + var options = new LocatorClickOptions { Timeout = 5000 }; + await locator.ClickAsync(options); + } + }" + }; + + await VerifyDiagnosticsAsync(code, EmptyDiagnosticResults); + } + + [Test] + public async Task AG0044_WhenUsingCustomType_NoWarning() + { + var code = new CodeDescriptor + { + Code = @" + using System.Threading.Tasks; + + class CustomLocator + { + public async Task ClickAsync(object options) { } + } + + class TestClass + { + public async Task TestMethod() + { + var locator = new CustomLocator(); + await locator.ClickAsync(new { Force = true }); + } + }" + }; + + await VerifyDiagnosticsAsync(code, EmptyDiagnosticResults); + } + + [Test] + public async Task AG0044_WhenSymbolIsNull_NoWarning() + { + var code = new CodeDescriptor + { + Code = @" + using System.Threading.Tasks; + + class TestClass + { + public async Task TestMethod() + { + dynamic locator = null; + await locator.ClickAsync(new { Force = true }); + } + }" + }; + + await VerifyDiagnosticsAsync(code, EmptyDiagnosticResults); + } +} \ No newline at end of file diff --git a/src/Agoda.Analyzers/AgodaCustom/AG0044ForceOptionShouldNotBeUsed.cs b/src/Agoda.Analyzers/AgodaCustom/AG0044ForceOptionShouldNotBeUsed.cs new file mode 100644 index 0000000..b73ff06 --- /dev/null +++ b/src/Agoda.Analyzers/AgodaCustom/AG0044ForceOptionShouldNotBeUsed.cs @@ -0,0 +1,91 @@ +using System.Collections.Immutable; +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 AG0044ForceOptionShouldNotBeUsed : DiagnosticAnalyzer + { + public const string DIAGNOSTIC_ID = "AG0044"; + private const string Category = "Usage"; + + private static readonly LocalizableString Title = new LocalizableResourceString( + nameof(CustomRulesResources.AG0044Title), + CustomRulesResources.ResourceManager, + typeof(CustomRulesResources)); + + private static readonly LocalizableString MessageFormat = new LocalizableResourceString( + nameof(CustomRulesResources.AG0044MessageFormat), + CustomRulesResources.ResourceManager, + typeof(CustomRulesResources)); + + private static readonly LocalizableString Description = new LocalizableResourceString( + nameof(CustomRulesResources.AG0044Description), + CustomRulesResources.ResourceManager, + typeof(CustomRulesResources)); + + private static readonly DiagnosticDescriptor Rule = new DiagnosticDescriptor( + DIAGNOSTIC_ID, + Title, + MessageFormat, + Category, + DiagnosticSeverity.Warning, + isEnabledByDefault: true, + description: Description, + "https://github.com/agoda-com/AgodaAnalyzers/blob/master/doc/AG0044.md", + WellKnownDiagnosticTags.EditAndContinue); + + public override ImmutableArray SupportedDiagnostics => ImmutableArray.Create(Rule); + + public override void Initialize(AnalysisContext context) + { + context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); + context.EnableConcurrentExecution(); + context.RegisterSyntaxNodeAction(AnalyzeNode, SyntaxKind.ObjectInitializerExpression); + } + + private void AnalyzeNode(SyntaxNodeAnalysisContext context) + { + var initializerExpression = (InitializerExpressionSyntax)context.Node; + if (initializerExpression.Parent == null) + return; + + // Check if this is initializing an options object for ILocator methods + var typeInfo = context.SemanticModel.GetTypeInfo(initializerExpression.Parent); + if (typeInfo.Type == null) return; + + var typeName = typeInfo.Type.Name; + if (!IsLocatorOptionsType(typeName)) return; + + // Look for Force = true in the initializer + foreach (var expression in initializerExpression.Expressions) + { + if (!(expression is AssignmentExpressionSyntax assignment) || + !(assignment.Left is IdentifierNameSyntax identifier) || + identifier.Identifier.ValueText != "Force") continue; + + if (!(assignment.Right is LiteralExpressionSyntax literal) || + literal.Token.ValueText != "true") continue; + + var diagnostic = Diagnostic.Create(Rule, expression.GetLocation()); + context.ReportDiagnostic(diagnostic); + } + } + + private static bool IsLocatorOptionsType(string typeName) + { + return typeName.EndsWith("Options") && ( + typeName.Contains("LocatorClick") || + typeName.Contains("LocatorFill") || + typeName.Contains("LocatorHover") || + typeName.Contains("LocatorDblClick") || + typeName.Contains("LocatorTap") || + typeName.Contains("LocatorCheck") || + typeName.Contains("LocatorUncheck") || + typeName.Contains("LocatorSelectOption")); + } + } +} \ No newline at end of file diff --git a/src/Agoda.Analyzers/AgodaCustom/CustomRulesResources.Designer.cs b/src/Agoda.Analyzers/AgodaCustom/CustomRulesResources.Designer.cs index cb52c81..cadc860 100644 --- a/src/Agoda.Analyzers/AgodaCustom/CustomRulesResources.Designer.cs +++ b/src/Agoda.Analyzers/AgodaCustom/CustomRulesResources.Designer.cs @@ -315,9 +315,6 @@ public static string AG0040MessageFormat { } } - /// - /// Looks up a localized string similar to You are using either an interpolated string or concatenation string in your logs, change these to the message template format to preserve structure in your logs. - /// public static string AG0041Title { get { return ResourceManager.GetString("AG0041Title", resourceCulture); @@ -335,5 +332,23 @@ public static string AG0043Title { return ResourceManager.GetString("AG0043Title", resourceCulture); } } + + public static string AG0044Title { + get { + return ResourceManager.GetString("AG0044Title", resourceCulture); + } + } + + public static string AG0044MessageFormat { + get { + return ResourceManager.GetString("AG0044MessageFormat", resourceCulture); + } + } + + public static string AG0044Description { + get { + return ResourceManager.GetString("AG0044Description", resourceCulture); + } + } } } diff --git a/src/Agoda.Analyzers/AgodaCustom/CustomRulesResources.resx b/src/Agoda.Analyzers/AgodaCustom/CustomRulesResources.resx index b288e69..b35854f 100644 --- a/src/Agoda.Analyzers/AgodaCustom/CustomRulesResources.resx +++ b/src/Agoda.Analyzers/AgodaCustom/CustomRulesResources.resx @@ -266,4 +266,13 @@ One exception is logging, where it can be useful to see the exact DC / cluster / BuildServiceProvider detected in request processing code. This may cause memory leaks. Remove the BuildServiceProvider() call and let the framework manage the service provider lifecycle. + + Force option should not be used in Locator methods + + + Avoid using Force option in Locator methods as it bypasses important element state validations + + + Force options bypass Playwright's built-in checks and can lead to unstable tests. Use proper waiting and interaction patterns instead. + diff --git a/src/Agoda.Analyzers/AnalyzerReleases.Unshipped.md b/src/Agoda.Analyzers/AnalyzerReleases.Unshipped.md index c59537d..e370556 100644 --- a/src/Agoda.Analyzers/AnalyzerReleases.Unshipped.md +++ b/src/Agoda.Analyzers/AnalyzerReleases.Unshipped.md @@ -25,6 +25,7 @@ AG0039 | Documentation | Hidden | AG0039MethodLineLengthAnalyzer, [Documentation AG0041 | Best Practices | Warning | AG0041LogTemplateAnalyzer, [Documentation](https://github.com/agoda-com/AgodaAnalyzers/blob/master/doc/AG0041.md) AG0042 | Agoda.CSharp.CustomQualityRules | Warning | AG0042QuerySelectorShouldNotBeUsed, [Documentation](https://github.com/agoda-com/AgodaAnalyzers/blob/master/doc/AG0042.md) AG0043 | Agoda.CSharp.CustomQualityRules | Error | AG0043NoBuildServiceProvider, [Documentation](https://github.com/agoda-com/AgodaAnalyzers/blob/master/doc/AG0043.md) +AG0044 | Usage | Warning | AG0044ForceOptionShouldNotBeUsed SA1106 | StyleCop.CSharp.ReadabilityRules | Warning | SA1106CodeMustNotContainEmptyStatements SA1107 | StyleCop.CSharp.ReadabilityRules | Warning | SA1107CodeMustNotContainMultipleStatementsOnOneLine SA1123 | StyleCop.CSharp.ReadabilityRules | Warning | SA1123DoNotPlaceRegionsWithinElements \ No newline at end of file From adff3a1963a4da9b43498397d798d134efb02b70 Mon Sep 17 00:00:00 2001 From: Rannes Date: Thu, 31 Oct 2024 00:58:17 +0700 Subject: [PATCH 2/2] docs: make docs a bit shorter, so it's more consumable --- doc/AG0044.md | 46 ---------------------------------------------- 1 file changed, 46 deletions(-) diff --git a/doc/AG0044.md b/doc/AG0044.md index 4bb704f..7b83e1b 100644 --- a/doc/AG0044.md +++ b/doc/AG0044.md @@ -101,52 +101,6 @@ public async Task InteractWithSlowElementAsync(ILocator element) - Can become flaky or inconsistent - Are harder to debug when they fail -## How to Fix It -1. **Use Proper Wait Strategies**: - ```csharp - // Instead of Force = true - await element.WaitForAsync(new() - { - State = WaitForSelectorState.Visible, - Timeout = 5000 - }); - await element.ClickAsync(); - ``` - -2. **Adjust Timeouts When Needed**: - ```csharp - await element.ClickAsync(new() { Timeout = 10000 }); - ``` - -3. **Use State Assertions**: - ```csharp - await element.WaitForAsync(new() { State = WaitForSelectorState.Enabled }); - await Assertions.Expect(element).ToBeVisibleAsync(); - await element.ClickAsync(); - ``` - -4. **Handle Dynamic Content**: - ```csharp - await page.WaitForLoadStateAsync(LoadState.NetworkIdle); - await element.WaitForAsync(); - await element.ClickAsync(); - ``` - -## Benefits -- More reliable and stable tests -- Better representation of real user interactions -- Easier debugging when tests fail -- Catches actual UI/UX issues -- More maintainable test code - -## Exceptions -Force option might be acceptable in very specific scenarios: -- Testing error cases where elements are intentionally obscured -- Dealing with known browser-specific edge cases -- Temporary workarounds while investigating underlying issues - -However, these should be rare exceptions and well-documented. - ## References - [Playwright Actionability](https://playwright.dev/dotnet/docs/actionability) - [Playwright Forcing Actions](https://playwright.dev/dotnet/docs/actionability#forcing-actions)