[java] minor performance improvements and code cleanup

iampopovich commented 4 months 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.

changes have been made to improve performance in various parts of the code.

Description

These changes help optimize the code and make it more efficient. I also replaced some deprecated method invocations

Replaced empty string checks with isEmpty().
Used System.arraycopy() instead of manual array copying.
Replaced String.format calls with printf.
Replaced some deprecated method invocations.

Motivation and Context

Types of changes

[ ] Bug fix (non-breaking change which fixes an issue)
[ ] 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.
[ ] I have added tests to cover my changes.
[x] All new and existing tests passed.

PR Type

Enhancement, Tests

Description

Replaced empty string checks with isEmpty() in multiple classes for better readability and performance.
Used System.arraycopy() instead of manual array copying in PrintOptions and EventFiringDecorator.
Replaced String.format calls with printf in CompletionCommand.
Replaced deprecated setScriptTimeout with scriptTimeout in various test files.
Replaced Arrays.asList with List.of for immutable list creation in test files.
Simplified map initialization in ChromiumOptions by using constructor initialization.
Optimized list addition in RemoteLogs by using addAll.

Changes walkthrough 📝

Relevant files

Enhancement

10 files

Cookie.java Optimize empty string checks in `Cookie` class. java/src/org/openqa/selenium/Cookie.java - Replaced empty string checks with `isEmpty()`.	+2/-2
ChromiumOptions.java Simplify map initialization in `ChromiumOptions`. java/src/org/openqa/selenium/chromium/ChromiumOptions.java - Replaced manual map population with constructor initialization.	+1/-2
FileExtension.java Optimize empty string checks in `FileExtension` class. java/src/org/openqa/selenium/firefox/FileExtension.java - Replaced empty string checks with `isEmpty()`.	+1/-1
CompletionCommand.java Use `printf` instead of `String.format` in `CompletionCommand`. java/src/org/openqa/selenium/grid/commands/CompletionCommand.java - Replaced `String.format` calls with `printf`.	+10/-12
ConcatenatingConfig.java Optimize empty string checks in `ConcatenatingConfig` class. java/src/org/openqa/selenium/grid/config/ConcatenatingConfig.java - Replaced empty string checks with `isEmpty()`.	+1/-1
LogLevelMapping.java Optimize empty string checks in `LogLevelMapping` class. java/src/org/openqa/selenium/logging/LogLevelMapping.java - Replaced empty string checks with `isEmpty()`.	+1/-1
PrintOptions.java Use `System.arraycopy` in `PrintOptions`. java/src/org/openqa/selenium/print/PrintOptions.java - Replaced manual array copy with `System.arraycopy`.	+2/-3
RemoteLogs.java Simplify list addition in `RemoteLogs`. java/src/org/openqa/selenium/remote/RemoteLogs.java - Replaced manual list addition with `addAll`.	+1/-3
EventFiringDecorator.java Use `System.arraycopy` in `EventFiringDecorator`. java/src/org/openqa/selenium/support/events/EventFiringDecorator.java - Replaced manual array copy with `System.arraycopy`.	+3/-6
BazelBuild.java Optimize empty string checks in `BazelBuild` class. java/test/org/openqa/selenium/build/BazelBuild.java - Replaced empty string checks with `isEmpty()`.	+1/-1

Tests

5 files

ExecutingAsyncJavascriptTest.java `Update deprecated methods and optimize list creation in tests.` java/test/org/openqa/selenium/ExecutingAsyncJavascriptTest.java Replaced deprecated `setScriptTimeout` with `scriptTimeout`. Replaced `Arrays.asList` with `List.of`.	+12/-12
ExecutingJavascriptTest.java Optimize list creation in `ExecutingJavascriptTest`. java/test/org/openqa/selenium/ExecutingJavascriptTest.java - Replaced `Arrays.asList` with `List.of`.	+1/-1
SessionQueueGridWithTimeoutTest.java Optimize list creation in `SessionQueueGridWithTimeoutTest`. java/test/org/openqa/selenium/grid/router/SessionQueueGridWithTimeoutTest.java - Replaced `Arrays.asList` with `List.of`.	+1/-1
InternetExplorerDriverTest.java Optimize empty string checks in `InternetExplorerDriverTest`. java/test/org/openqa/selenium/ie/InternetExplorerDriverTest.java - Replaced empty string checks with `isEmpty()`.	+1/-1
RemoteWebDriverUnitTest.java Update deprecated methods in `RemoteWebDriverUnitTest`. java/test/org/openqa/selenium/remote/RemoteWebDriverUnitTest.java - Replaced deprecated `setScriptTimeout` with `scriptTimeout`.	+1/-1

💡 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 months ago

PR Review 🔍

⏱️ Estimated effort to review [1-5]	2, because the changes are straightforward, mostly involving syntax improvements and method updates across several files. The PR is well-organized and the changes are consistent, making it relatively easy to review.
🧪 Relevant tests	Yes
⚡ Possible issues	Possible Bug: The change in `PrintOptions.java` from a manual array copy to `System.arraycopy` might introduce an off-by-one error. The original code seems to have an incorrect loop condition (`i < ranges.length` should be `i < ranges.length + 1`), and the new code might not correctly handle all entries if not adjusted properly.
🔒 Security concerns	No

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

PR Code Suggestions ✨

Category	Suggestion	Score
Readability	Simplify the condition to improve clarity ___ The condition `ranges.length - 1 >= 0` is always true if `ranges.length` is non-negative. Consider simplifying the condition to `ranges.length > 0` for clarity. [java/src/org/openqa/selenium/print/PrintOptions.java [70]](https://github.com/SeleniumHQ/selenium/pull/14054/files#diff-8b6352fe3693861c620fbe50178f01456b689e913b164d0ff4b788cce7c67414R70-R70) ```diff -if (ranges.length - 1 >= 0) +if (ranges.length > 0) ``` Suggestion importance[1-10]: 7 Why: The suggestion correctly identifies a simplification in the condition check. Changing `ranges.length - 1 >= 0` to `ranges.length > 0` improves readability and understanding of the condition.	7

codecov[bot] commented 4 months ago

Codecov Report

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

Project coverage is 58.72%. Comparing base (57f8398) to head (1181d73). Report is 528 commits behind head on trunk.

:exclamation: Current head 1181d73 differs from pull request most recent head 0dd4c1e

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

Additional details and impacted files

```diff @@ Coverage Diff @@ ## trunk #14054 +/- ## ========================================== + Coverage 58.48% 58.72% +0.23% ========================================== Files 86 86 Lines 5270 5298 +28 Branches 220 227 +7 ========================================== + Hits 3082 3111 +29 + Misses 1968 1960 -8 - Partials 220 227 +7 ```

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

titusfortner commented 4 months ago

Looks like the event firing decorator tests are failing. Can you investigate?

iampopovich commented 4 months ago

Based on the trace of the failed tests, I suspect that the requireNonNull check is breaking the test. System.arraycopy(Objects.requireNonNull(args), 0, args2, 1, argsLength); The original code did not include a check that args could be null. I will try removing the null check, and if the test continues to fail, I will revert the changes back to the original code.

iampopovich commented 3 months ago

In the code segment that led to the test failure, there is a tricky mechanism for handling the situation where args == null

 int argsLength = args != null ? args.length : 0;
    Object[] args2 = new Object[argsLength + 1];
    args2[0] = target.getOriginal();
    for (int i = 0; i < argsLength; i++) {
      args2[i + 1] = args[i];
    }

if args is null, the args2 array is created with a size sufficient only to store target.getOriginal(). Since argsLength will be zero, the for loop will not be executed, and there will be no errors in accessing the elements of args. I replaced it witharraycopy()and explicit check if(args != null) if(args != null) System.arraycopy(args, 0, args2, 1, argsLength);

SeleniumHQ / selenium