search

SeleniumHQ / selenium

A browser automation framework and ecosystem.
https://selenium.dev
Apache License 2.0
29.77k stars 8.02k forks source link

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

Open iampopovich opened 2 days ago

iampopovich commented 2 days 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 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

Checklist

PR Type

Bug fix, Tests

Description

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

    codiumai-pr-agent-pro[bot] commented 2 days ago

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review [1-5] 2
    🧪 Relevant tests Yes
    🔒 Security concerns No
    ⚡ Key issues to review None
    codiumai-pr-agent-pro[bot] commented 2 days ago

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Add null check for options to prevent NullPointerException ___ **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]](https://github.com/SeleniumHQ/selenium/pull/14217/files#diff-4133f16d6f27fa85995d3806efc8a9a862f9a073ecbf16fb03fc3657bb2a586cR341-R343) ```diff -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 ___ **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]](https://github.com/SeleniumHQ/selenium/pull/14217/files#diff-a2da4c56efda1ff71535e17798b3e48b6599280a2e215d9837d082182061a584R397-R397) ```diff -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
    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]](https://github.com/SeleniumHQ/selenium/pull/14217/files#diff-a2da4c56efda1ff71535e17798b3e48b6599280a2e215d9837d082182061a584R395-R395) ```diff 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 ___ **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]](https://github.com/SeleniumHQ/selenium/pull/14217/files#diff-4133f16d6f27fa85995d3806efc8a9a862f9a073ecbf16fb03fc3657bb2a586cR341-R343) ```diff -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 commented 2 days ago

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

    iampopovich commented 2 days ago

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