-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add AG0044 rule - Add Analyzers for Force option methods #199
Merged
Merged
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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) |
181 changes: 181 additions & 0 deletions
181
src/Agoda.Analyzers.Test/AgodaCustom/AG0044UnitTests.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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); | ||
} | ||
} |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove from this line down. But keep references