Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 7 commits into from
Jul 12, 2024

Conversation

iampopovich
Copy link
Contributor

@iampopovich iampopovich commented Jul 1, 2024

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

    Copy link
    Contributor

    qodo-merge-pro bot commented Jul 1, 2024

    PR Reviewer Guide 🔍

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

    Copy link
    Contributor

    qodo-merge-pro bot commented Jul 1, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    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
    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
    Copy link
    Member

    diemol commented Jul 1, 2024

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

    @iampopovich
    Copy link
    Contributor Author

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

    Copy link
    Member

    @diemol diemol left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    @iampopovich
    Copy link
    Contributor Author

    @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
    

    Copy link
    Member

    @diemol diemol left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Thank you, @iampopovich!

    @diemol diemol merged commit 4a17ec3 into SeleniumHQ:trunk Jul 12, 2024
    27 of 29 checks passed
    dawid38 pushed a commit to dawid38/selenium that referenced this pull request Jul 21, 2024
    …Q#14217)
    
    * added androidOptions in merge method
    
    * applied suggestions
    
    * applying format.sh
    
    * import assertEquals
    
    * fix for importi Optional
    
    ---------
    
    Co-authored-by: Diego Molina <[email protected]>
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Oct 29, 2024
    …Q#14217)
    
    * added androidOptions in merge method
    
    * applied suggestions
    
    * applying format.sh
    
    * import assertEquals
    
    * fix for importi Optional
    
    ---------
    
    Co-authored-by: Diego Molina <[email protected]>
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    [🐛 Bug]: ChromiumOptions: android-specific options are removed after merge
    2 participants