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 capabilities of slot with the new session request capabilities #11369

Merged
merged 11 commits into from
Jan 30, 2023

Conversation

pujagani
Copy link
Contributor

@pujagani pujagani commented Dec 5, 2022

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

Merge capabilities of slot with the new session request capabilities

Motivation and Context

Node config allows the Grid user to set driver configuration.
Example: java -jar selenium-server-4.6.0.jar standalone --port 50859 --detect-drivers false --driver-configuration display-name="Chrome Custom" stereotype='{"browserName":"chrome","goog:chromeOptions":{"args":[\"incognito\",\"window-size=500,500\"]},"pageLoadStrategy":"normal"}'

The configuration is applied to the session slots.
This intends to provide the configuration that all matching incoming session requests should ideally consider and pass it along to the respective web driver.
Currently, the new session request capabilities are passed as it is without considering if the session slot has additional capabilities that need to be considered.

The changes help fix this by giving merging the capabilities of the slot and the incoming session request. The capabilities of the incoming session request override the capabilities of the slot case of custom driver-configuration.

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.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Dec 5, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@diemol
Copy link
Member

diemol commented Dec 5, 2022

We do something similar on the LocalNode. Does this PR improve that code? Should we centralize that logic and do it in a single place?

@pujagani
Copy link
Contributor Author

pujagani commented Dec 7, 2022

Thank you for helping take a look. The LocalNode logic, is for sending the result back to the user. The PR does something similar but to send to the driver. Mainly, if the sessionSlot is configured with custom capabilities, the PR merges that with incoming session capabilities and sends it to the driver factory. My thought process was more around the session slot being the owner of the stereotype, the logic should sit there. LocaNode is more of a meditator and is aware of the session slots. What do you think?

@titusfortner titusfortner requested a review from diemol January 14, 2023 04:11
@pujagani pujagani force-pushed the merge-node-config-caps branch from bdbe3a0 to a51bda4 Compare January 17, 2023 05:52
@pujagani
Copy link
Contributor Author

@diemol After giving your comments a thought, it made sense to move the logic to SessionCapabilitiesMutator since that is where we make some checks on the capabilities and add some capabilities as required. Additionally, as you correctly pointed out, I think it makes sense to give the user-sent capabilities priority over the stereotype capability to avoid any breaking changes. I have updated the changes accordingly and reverted the previous ones. Will appreciate your review. Thank you so much.

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, I left a few comments.

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.

Added missing comment for the Chromium case.

@codecov-commenter
Copy link

codecov-commenter commented Jan 27, 2023

Codecov Report

Base: 54.65% // Head: 54.65% // No change to project coverage 👍

Coverage data is based on head (e4d1af0) compared to base (398524f).
Patch has no changes to coverable lines.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@           Coverage Diff           @@
##            trunk   #11369   +/-   ##
=======================================
  Coverage   54.65%   54.65%           
=======================================
  Files          85       85           
  Lines        5643     5643           
  Branches      244      244           
=======================================
  Hits         3084     3084           
  Misses       2315     2315           
  Partials      244      244           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

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, @pujagani!

@pujagani pujagani merged commit 6bc5a58 into SeleniumHQ:trunk Jan 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants