[java] Merge android specific parameters in chrome options by iampopovich · Pull Request #14217 · SeleniumHQ/selenium

iampopovich · 2024-07-01T13:04:51Z

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 bug #14133 I added androidOptions to merge method ChromiumOptions.mergeInPlace
I also added a test for the new functionality.

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

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.
All new and existing tests passed.

PR Type

Bug fix, Tests

Description

Added merging of androidOptions in ChromiumOptions.mergeInPlace method to handle Android-specific parameters.
Introduced a new test in ChromeOptionsTest to verify the correct merging of Android-specific options.

Changes walkthrough 📝

Relevant files

Bug fix

ChromiumOptions.java `Add merging of Android-specific options in ChromiumOptions` java/src/org/openqa/selenium/chromium/ChromiumOptions.java Added merging of `androidOptions` in `mergeInPlace` method. Included a check for `androidOptions` before merging.	+4/-0

Tests

ChromeOptionsTest.java `Add test for merging Android-specific options in ChromeOptions` java/test/org/openqa/selenium/chrome/ChromeOptionsTest.java Added a test to verify merging of Android-specific options. Ensured the test checks for equality of options after merging.	+15/-0

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

qodo-code-review · 2024-07-01T13:05:11Z

PR Reviewer Guide 🔍

⏱️ Estimated effort to review [1-5]	2
🧪 Relevant tests	Yes
🔒 Security concerns	No
⚡ Key issues to review	None

qodo-code-review · 2024-07-01T13:05:43Z

PR Code Suggestions ✨

Category	Suggestion	Score
Possible bug	✅ Add null check for options to prevent NullPointerException Suggestion Impact: The commit added a null check for `options` before accessing `options.androidOptions`, which aligns with the suggestion to prevent potential NullPointerExceptions. code diff: - if (options.androidOptions != null) { - options.androidOptions.forEach(this::setAndroidCapability); - } + Optional.ofNullable(options.androidOptions) + .ifPresent(opts -> opts.forEach(this::setAndroidCapability)); Consider checking for nullity of `options` before accessing its properties to avoid potential NullPointerExceptions. This is crucial especially when dealing with optional capabilities that might not be initialized. java/src/org/openqa/selenium/chromium/ChromiumOptions.java [341-343] -if (options.androidOptions != null) { +if (options != null && options.androidOptions != null) { options.androidOptions.forEach(this::setAndroidCapability); } Apply this suggestion Suggestion importance[1-10]: 9 Why: Adding a null check for `options` is crucial to prevent potential NullPointerExceptions, especially when dealing with optional capabilities that might not be initialized. This improves the robustness of the code.	9
Enhancement	✅ Improve assertion to provide detailed error messages Suggestion Impact: The commit replaced the assert statement using equals with assertEquals, which aligns with the suggestion to provide detailed error messages for assertion failures. code diff: - assert original.asMap().equals(merged.asMap()); - + assertEquals(original.asMap(), merged.asMap()); Use `assertEquals` instead of `equals` inside the `assert` statement to ensure that any assertion failure provides a clear and detailed message about the discrepancy between the expected and actual results. java/test/org/openqa/selenium/chrome/ChromeOptionsTest.java [397] -assert original.asMap().equals(merged.asMap()); +assertEquals(original.asMap(), merged.asMap()); Apply this suggestion Suggestion importance[1-10]: 8 Why: Using `assertEquals` instead of `equals` inside the `assert` statement ensures that any assertion failure provides a clear and detailed message about the discrepancy between the expected and actual results, improving test diagnostics.	8
Enhancement	Add detailed assertions for each property to ensure correct merging Add assertions to check individual properties of the merged capabilities to ensure that each property is correctly merged, especially focusing on Android-specific options. java/test/org/openqa/selenium/chrome/ChromeOptionsTest.java [395] var merged = original.merge(caps); +assertEquals("co_activity", merged.getAndroidActivity()); +assertEquals("co_package", merged.getAndroidPackage()); +assertEquals("co_experimental", merged.getExperimentalOption("experimental")); +assertTrue(merged.getArguments().contains("--co_argument")); Apply this suggestion Suggestion importance[1-10]: 7 Why: Adding assertions for individual properties of the merged capabilities ensures that each property is correctly merged, especially focusing on Android-specific options. This enhances the thoroughness of the test.	7
Best practice	✅ Use Optional for null-safe handling of androidOptions Suggestion Impact: The suggestion to use Optional for handling androidOptions was implemented in the commit. The code now uses Optional.ofNullable to check for null and process androidOptions if present. code diff: - if (options.androidOptions != null) { - options.androidOptions.forEach(this::setAndroidCapability); - } + Optional.ofNullable(options.androidOptions) + .ifPresent(opts -> opts.forEach(this::setAndroidCapability)); Consider using `Optional` for handling `androidOptions` to make the code more robust and idiomatic to Java 8 and above, avoiding direct null checks. java/src/org/openqa/selenium/chromium/ChromiumOptions.java [341-343] -if (options.androidOptions != null) { - options.androidOptions.forEach(this::setAndroidCapability); -} +Optional.ofNullable(options.androidOptions).ifPresent(opts -> opts.forEach(this::setAndroidCapability)); Apply this suggestion Suggestion importance[1-10]: 6 Why: Using `Optional` for handling `androidOptions` makes the code more robust and idiomatic to Java 8 and above, avoiding direct null checks. However, it is a stylistic improvement rather than a critical fix.	6

diemol · 2024-07-01T14:01:38Z

@iampopovich seems the tests cannot be built https://github.com/SeleniumHQ/selenium/actions/runs/9745320822/job/26892961031?pr=14217#step:15:725

iampopovich · 2024-07-01T15:09:29Z

@diemol my bad , i forgot to import assertEquals
fix's on the way

diemol

Did you run the build before?

Still getting errors: https://github.com/SeleniumHQ/selenium/actions/runs/9913402348/job/27390332113?pr=14217#step:15:482

iampopovich · 2024-07-12T20:38:07Z

@diemol i guess something happened with my working environment, and I missed another import. I also did not check the build in Gitpod.

Now i ran build with command bazel build //java/...
the build result is attached in the log below.
I apologize for the time wasted on such elementary mistakes.

INFO: Found 1725 targets...
INFO: Elapsed time: 175.258s, Critical Path: 126.66s
INFO: 5138 processes: 3723 internal, 548 darwin-sandbox, 1 local, 866 worker.
INFO: Build completed successfully, 5138 total actions

diemol

Thank you, @iampopovich!

…Q#14217) * added androidOptions in merge method * applied suggestions * applying format.sh * import assertEquals * fix for importi Optional --------- Co-authored-by: Diego Molina <diemol@users.noreply.github.com>

added androidOptions in merge method

31fe6a3

qodo-code-review bot added P-bug fix PR addresses a known issue tests Review effort [1-5]: 2 labels Jul 1, 2024

iampopovich and others added 3 commits July 1, 2024 20:18

applied suggestions

4b61350

applying format.sh

8ae61f3

Merge branch 'trunk' into fix-14133-merge-capabilities

025a40c

diemol linked an issue Jul 1, 2024 that may be closed by this pull request

[🐛 Bug]: ChromiumOptions: android-specific options are removed after merge #14133

Closed

iampopovich and others added 2 commits July 1, 2024 22:10

import assertEquals

9f9e464

Merge branch 'trunk' into fix-14133-merge-capabilities

d569f80

diemol reviewed Jul 12, 2024

View reviewed changes

fix for importi Optional

1c26267

diemol approved these changes Jul 12, 2024

View reviewed changes

diemol merged commit 4a17ec3 into SeleniumHQ:trunk Jul 12, 2024

x-way mentioned this pull request Aug 10, 2024

[Snyk] Upgrade selenium-webdriver from 4.18.1 to 4.23.0 x-way/cal-heatmap#544

Closed

MiguelPelegrina mentioned this pull request Nov 8, 2024

[Snyk] Upgrade selenium-webdriver from 4.22.0 to 4.25.0 MiguelPelegrina/java_school_online_store_frontend#21

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[java] Merge android specific parameters in chrome options#14217

[java] Merge android specific parameters in chrome options#14217
diemol merged 7 commits intoSeleniumHQ:trunkfrom
iampopovich:fix-14133-merge-capabilities

iampopovich commented Jul 1, 2024 •

edited by qodo-code-review bot

Loading

Uh oh!

qodo-code-review bot commented Jul 1, 2024

Uh oh!

qodo-code-review bot commented Jul 1, 2024 •

edited

Loading

Uh oh!

diemol commented Jul 1, 2024

Uh oh!

iampopovich commented Jul 1, 2024

Uh oh!

diemol left a comment

Uh oh!

iampopovich commented Jul 12, 2024

Uh oh!

diemol left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

iampopovich commented Jul 1, 2024 • edited by qodo-code-review bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User description

Description

Motivation and Context

Types of changes

Checklist

PR Type

Description

Changes walkthrough 📝

Uh oh!

qodo-code-review bot commented Jul 1, 2024

PR Reviewer Guide 🔍

Uh oh!

qodo-code-review bot commented Jul 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Uh oh!

diemol commented Jul 1, 2024

Uh oh!

iampopovich commented Jul 1, 2024

Uh oh!

diemol left a comment

Choose a reason for hiding this comment

Uh oh!

iampopovich commented Jul 12, 2024

Uh oh!

diemol left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

iampopovich commented Jul 1, 2024 •

edited by qodo-code-review bot

Loading

qodo-code-review bot commented Jul 1, 2024 •

edited

Loading