agoda-com / agoda-com/AgodaAnalyzers
Proposal: Static Analysis Rule Against Redundant ScrollIntoViewIfNeededAsync Before ClickAsync (AG0047)
- Vorherrschende Sprache
- C#
- Sterne
- 25
- Forks
- 15
- PR-Merge-Kennzahlen
- Keine gemergten PRs in 30 T.
Beschreibung
## Overview
This proposal suggests implementing a Roslyn-based static code analyzer rule that detects when developers redundantly call `ScrollIntoViewIfNeededAsync()` immediately before `ClickAsync()` on Playwright locators. This pattern is unnecessary as `ClickAsync()` already performs scrolling internally and may actually hide underlying test issues.
## Background
Playwright's `ClickAsync()` method automatically performs several actionability checks before clicking, including:
1. Ensuring the element is visible
2. Scrolling the element into view if needed
3. Waiting for the element to be enabled
4. Waiting for the element to be stable
Explicitly calling `ScrollIntoViewIfNeededAsync()` right before `ClickAsync()` is redundant and can sometimes mask potential issues that would normally be caught by Playwright's built-in actionability checks.
## The Problem
When developers use `ScrollIntoViewIfNeededAsync()` before `ClickAsync()`:
1. **Redundancy** - The scroll operation is performed twice unnecessarily
2. **Masks issues** - Tests may pass locally but fail in different environments due to hidden timing problems
3. **False security** - Gives a false impression that the extra step is needed for reliability
4. **Reduced maintainability** - Creates more complex code with no benefits
5. **Inconsistent patterns** - Leads to inconsistent testing practices across the codebase
## Bad Examples
```csharp
// BAD: Redundant scroll before click
await element.ScrollIntoViewIfNeededAsync();
await element.ClickAsync();
// BAD: Redundant scroll with options before click
await element.ScrollIntoViewIfNeededAsync(new() { Timeout = 5000 });
await element.ClickAsync();
// BAD: Redundant scroll with awaited operation in between
await element.ScrollIntoViewIfNeededAsync();
await Task.Delay(100); // Small delay doesn't change the redundancy
await element.ClickAsync();
```
## Good Examples
```csharp
// GOOD: Let ClickAsync handle scrolling automatically
await element.ClickAsync();
// GOOD: Use timeout options if needed
await element.ClickAsync(new() { Timeout = 10000 });
// GOOD: If you need to specifically verify visibility before clicking
await element.WaitForAsync(new() { State = WaitForSelectorState.Visible });
await element.ClickAsync();
// GOOD: If scrolling is needed for a different operation before clicking
await element.ScrollIntoViewIfNeededAsync();
await element.HoverAsync(); // Non-click operation that benefits from scrolling
// ... other operations
await element.ClickAsync(); // Later click is fine
```
## Detection Strategy
The analyzer will identify:
1. Sequential method calls where `ScrollIntoViewIfNeededAsync()` is followed by `ClickAsync()` on the same locator
2. Minimal or no meaningful operations between these method calls
3. Patterns where the same locator variable is used for both operations
## Proposed Implementation
Create a Roslyn analyzer that:
1. Identifies invocation of `ScrollIntoViewIfNeededAsync()` method on Playwright locator objects
2. Tracks when the same locator object is used for a subsequent `ClickAsync()` call
3. Checks if there are significant operations between these calls
4. Reports a diagnostic when the redundant pattern is detected
5. Provides a code fix to remove the redundant scroll operation
## Diagnostic Information
- **ID**: AG0047
- **Category**: Playwright
- **Default Severity**: Warning
- **Title**: Redundant ScrollIntoViewIfNeededAsync before ClickAsync
- **Message**: ScrollIntoViewIfNeededAsync is redundant before ClickAsync as clicking already performs scrolling automatically.
## Code Fix Provider
The code fix provider will:
1. Remove the redundant `ScrollIntoViewIfNeededAsync()` call
2. Remove any intervening non-essential statements if appropriate
3. Preserve any comments by moving them to the remaining `ClickAsync()` line
## Benefits
1. Cleaner, more concise test code
2. More reliable tests that properly utilize Playwright's built-in capabilities
3. Reduced test execution time by eliminating redundant operations
4. Consistent application of best practices across test projects
5. Better visibility of actual underlying issues in tests
## Implementation Details
The analyzer will need to:
1. Identify method invocation syntax for `ScrollIntoViewIfNeededAsync()`
2. Track the receiver of this method (the locator variable)
3. Scan subsequent statements for `ClickAsync()` calls on the same receiver
4. Consider the context of surrounding operations to avoid false positives
5. Handle both synchronous code and async/await patterns
## Complexity Considerations
1. Need to handle cases where the locator is re-assigned or modified between calls
2. Need to distinguish between meaningful intervening operations and simple delays
3. Handle cases with nested scopes and control structures
## Questions for Discussion
1. Should the rule be extended to cover other similar patterns, like `HoverAsync()` followed immediately by `ClickAsync()`?
2. Should we consider specific exceptions where redundant scrolling might be justified?
3. What severity level is most appropriate - warning or error?
## References
- [[Playwright Actionability Documentation](https://playwright.dev/dotnet/docs/actionability)](https://playwright.dev/dotnet/docs/actionability)
- [[Locator.ClickAsync Method](https://playwright.dev/dotnet/docs/api/class-locator#locator-click)](https://playwright.dev/dotnet/docs/api/class-locator#locator-click)
- [[Locator.ScrollIntoViewIfNeededAsync Method](https://playwright.dev/dotnet/docs/api/class-locator#locator-scroll-into-view-if-needed)](https://playwright.dev/dotnet/docs/api/class-locator#locator-scroll-into-view-if-needed)
Beitragsleitfaden
Bewertung
Dieses Issue wurde noch nicht bewertet.