[java] custom duration for Actions constructor

iampopovich commented 4 weeks ago

User description

Thanks for contributing to Selenium! A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines. Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

according to #12118 feature request, I started to discover the best way to implement duration overriding

Motivation and Context

Types of changes

[ ] Bug fix (non-breaking change which fixes an issue)
[x] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist

[x] I have read the contributing document.
[ ] My change requires a change to the documentation.
[ ] I have updated the documentation accordingly.
[x] I have added tests to cover my changes.
[x] All new and existing tests passed.

PR Type

Enhancement

Description

Added a new constructor to the Actions class in Selenium that allows specifying a custom Duration for actions.
Introduced a new private member variable actionDuration to store the specified duration.

Changes walkthrough 📝

Relevant files

Enhancement

Actions.java

Add constructor with custom duration to Actions class

java/src/org/openqa/selenium/interactions/Actions.java

Added a new constructor to the Actions class that accepts a Duration
parameter.

Introduced a new private member variable actionDuration to store the
duration.

+6/-0

💡 PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

codiumai-pr-agent-pro[bot] commented 4 weeks ago

PR Review 🔍

⏱️ Estimated effort to review [1-5]	2, because the PR involves a straightforward addition of a new constructor to handle custom durations in the Actions class. The changes are localized and do not involve complex logic changes.
🧪 Relevant tests	No
⚡ Possible issues	Missing Initialization: The new field `actionDuration` is not initialized in the original constructor, which might lead to null issues if methods relying on this field are called without using the new constructor.
🔒 Security concerns	No

codiumai-pr-agent-pro[bot] commented 4 weeks ago

PR Code Suggestions ✨

Category	Suggestion	Score
Possible issue	Initialize the new variable in all constructors to prevent potential null pointer issues ___ Ensure that `actionDuration` is also initialized in the existing constructor to avoid potential `NullPointerException` when the duration is used in other methods. [java/src/org/openqa/selenium/interactions/Actions.java [60-61]](https://github.com/SeleniumHQ/selenium/pull/14085/files#diff-be22ba4639c55728723af6e60493d4bb5dfa2d9bab9d35e2fc2ba3b9076cc596R60-R61) ```diff this.driver = Require.nonNull("Driver", driver); +this.actionDuration = Duration.ofMillis(0); // or any default value ``` Suggestion importance[1-10]: 8 Why: This is a crucial suggestion to prevent potential runtime errors due to uninitialized `actionDuration` in the existing constructor.	8
Best practice	Add validation for the new constructor parameter to ensure it is not null ___ Validate the `duration` parameter in the new constructor to ensure it is not null, similar to how the `driver` parameter is validated. [java/src/org/openqa/selenium/interactions/Actions.java [64-65]](https://github.com/SeleniumHQ/selenium/pull/14085/files#diff-be22ba4639c55728723af6e60493d4bb5dfa2d9bab9d35e2fc2ba3b9076cc596R64-R65) ```diff this.driver = Require.nonNull("Driver", driver); -this.actionDuration = duration; +this.actionDuration = Require.nonNull("Duration", duration); ``` Suggestion importance[1-10]: 7 Why: Validating the `duration` parameter for null values is important for robustness, similar to the existing validation for the `driver` parameter.	7
Maintainability	Use constructor overloading to avoid code duplication and improve readability ___ To improve code readability, consider overloading the existing constructor instead of duplicating the code. [java/src/org/openqa/selenium/interactions/Actions.java [63-66]](https://github.com/SeleniumHQ/selenium/pull/14085/files#diff-be22ba4639c55728723af6e60493d4bb5dfa2d9bab9d35e2fc2ba3b9076cc596R63-R66) ```diff public Actions(WebDriver driver, Duration duration) { - this.driver = Require.nonNull("Driver", driver); + this(driver); // Call the existing constructor this.actionDuration = duration; } ``` Suggestion importance[1-10]: 7 Why: Overloading the constructor to reuse existing code improves maintainability and reduces the risk of future errors in constructor logic.	7
Enhancement	Add a default value for the new parameter to handle null values gracefully ___ Consider adding a default value for `actionDuration` in the new constructor if `duration` is null, to provide a fallback mechanism. [java/src/org/openqa/selenium/interactions/Actions.java [64-65]](https://github.com/SeleniumHQ/selenium/pull/14085/files#diff-be22ba4639c55728723af6e60493d4bb5dfa2d9bab9d35e2fc2ba3b9076cc596R64-R65) ```diff this.driver = Require.nonNull("Driver", driver); -this.actionDuration = duration; +this.actionDuration = (duration != null) ? duration : Duration.ofMillis(0); // or any default value ``` Suggestion importance[1-10]: 6 Why: Providing a default value for `actionDuration` if `duration` is null is a good enhancement for better fault tolerance.	6

titusfortner commented 4 weeks ago

Oh, thanks for taking on this one! I'm glad someone is finally looking into it.

Your code doesn't actually show actionDuration getting used anywhere, though? Need to make it so that the default is overridden and used in necessary places.

pujagani commented 4 weeks ago

Please add tests for the new constructor added.

iampopovich commented 3 weeks ago

@pujagani I reused two existing tests for the scrollToElement and scrollByAmount methods with a custom Duration. Additionally, I wrote two tests to check the setting of the default Duration and the custom Duration. Will this be enough?

codecov[bot] commented 2 weeks ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 57.76%. Comparing base (5c5487f) to head (0ef4c94). Report is 8 commits behind head on trunk.

:exclamation: Current head 0ef4c94 differs from pull request most recent head f090146

Please upload reports for the commit f090146 to get more accurate results.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## trunk #14085 +/- ## ======================================= Coverage 57.76% 57.76% ======================================= Files 87 87 Lines 5413 5413 Branches 228 228 ======================================= Hits 3127 3127 Misses 2058 2058 Partials 228 228 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

titusfortner commented 2 weeks ago

Haven't looked at the failures, pushing this one back to the next release

SeleniumHQ / selenium