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

Remove 'browserName' capability from stereotype when using RelaySession #14247

Merged
merged 5 commits into from
Jul 12, 2024

Conversation

bhecquet
Copy link
Contributor

@bhecquet bhecquet commented Jul 10, 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

Remove browserName capability from stereotype before merging requested capabilities and stereotype.

Motivation and Context

When using Selenium Grid relay with appium and trying to test a mobile application, if node declares a browserName in stereotype, this browserName is added to capabilities sent to appium
Solves issue #14216

Moreover, if the session would need a browser, browserName capability would be provided in session request

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. (There are no tests for this class)
  • All new and existing tests passed.

I've tested the change in the situation where the problem occured, and it's solved.


PR Type

Bug fix


Description

  • Removed browserName capability from the stereotype in RelaySessionFactory to prevent conflicts with Appium tests when an app is provided.
  • Ensured that for any other test, the browser should be provided in the session request.
  • Imported necessary classes MutableCapabilities and CapabilityType to facilitate these changes.

Changes walkthrough 📝

Relevant files
Bug fix
RelaySessionFactory.java
Remove browserName capability from stereotype in RelaySessionFactory

java/src/org/openqa/selenium/grid/node/relay/RelaySessionFactory.java

  • Imported MutableCapabilities and CapabilityType.
  • Removed browserName capability from the stereotype to avoid conflicts
    with Appium tests.
  • Merged the filtered stereotype with the session capabilities.
  • +9/-1     

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

    Sorry, something went wrong.

    …resent is SessionRequest
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Possible Bug
    Setting CapabilityType.BROWSER_NAME to null might lead to unexpected behavior or errors downstream. It's generally safer to remove the capability if it's not needed rather than setting it to null.

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Add null check for stereotype to prevent potential null pointer exceptions

    To avoid potential null pointer exceptions and ensure type safety, consider checking
    if stereotype is non-null before creating MutableCapabilities from it.

    java/src/org/openqa/selenium/grid/node/relay/RelaySessionFactory.java [154]

    -MutableCapabilities filteredStereotype = new MutableCapabilities(stereotype);
    +if (stereotype != null) {
    +    MutableCapabilities filteredStereotype = new MutableCapabilities(stereotype);
    +    filteredStereotype.setCapability(CapabilityType.BROWSER_NAME, (String)null);
    +}
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Adding a null check for stereotype is important to prevent potential null pointer exceptions, which can cause runtime errors. This improves the robustness of the code.

    9
    Best practice
    Use removeCapability instead of setting to null for clarity

    Instead of explicitly setting the BROWSER_NAME capability to null, consider using a
    method that removes the capability if it exists. This approach might be more
    intuitive and align with common practices for modifying capabilities.

    java/src/org/openqa/selenium/grid/node/relay/RelaySessionFactory.java [155]

    -filteredStereotype.setCapability(CapabilityType.BROWSER_NAME, (String)null);
    +filteredStereotype.removeCapability(CapabilityType.BROWSER_NAME);
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Using removeCapability is a clearer and more intuitive way to remove a capability, aligning with common practices and improving code readability.

    8
    Maintainability
    Improve variable naming for better readability

    Consider using a more descriptive variable name for the filteredStereotype to
    reflect its purpose after removing the browserName capability. This will improve
    code readability and maintainability.

    java/src/org/openqa/selenium/grid/node/relay/RelaySessionFactory.java [154-155]

    -MutableCapabilities filteredStereotype = new MutableCapabilities(stereotype);
    -filteredStereotype.setCapability(CapabilityType.BROWSER_NAME, (String)null);
    +MutableCapabilities capabilitiesWithoutBrowserName = new MutableCapabilities(stereotype);
    +capabilitiesWithoutBrowserName.setCapability(CapabilityType.BROWSER_NAME, (String)null);
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion improves code readability and maintainability by using a more descriptive variable name, which is beneficial for understanding the code's purpose. However, it is not crucial for functionality.

    7
    Enhancement
    Add debug logging after merging capabilities for better traceability

    To ensure that the capabilities are correctly merged and no unintended modifications
    occur, consider adding a debug log statement after merging the capabilities. This
    can help in troubleshooting and verifying that the merge operation behaves as
    expected.

    java/src/org/openqa/selenium/grid/node/relay/RelaySessionFactory.java [157]

     capabilities = capabilities.merge(filteredStereotype);
    +LOG.debug("Capabilities after merge: " + capabilities);
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Adding a debug log statement can help with troubleshooting and verifying the merge operation, but it is not essential for the functionality of the code.

    6

    Sorry, something went wrong.

    Verified

    This commit was created on GitHub.com and signed with GitHub’s verified signature.
    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, @bhecquet!

    Verified

    This commit was created on GitHub.com and signed with GitHub’s verified signature.
    @diemol diemol merged commit 9d6131f into SeleniumHQ:trunk Jul 12, 2024
    4 checks passed
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Oct 29, 2024
    …on (SeleniumHQ#14247)
    
    * Remove 'browserName' capability from stereotype. We expect it to be present is SessionRequest
    
    * Only remove 'browserName' when 'appium:app' is present
    
    * Formatting file
    
    ---------
    
    Co-authored-by: Diego Molina <diemol@users.noreply.github.com>
    Co-authored-by: Diego Molina <diemol@gmail.com>
    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]: Cannot start mobile application with appium when slot also specifies a 'browserName' capability
    2 participants