You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
[🚀 Feature]: Unifying Select Class Across All Bindings #15265
💥 What does this PR do?
I implemented unifying the Select class across all bindings for the Java package.
I also extended the unit tests to cover this functionality by shadowing PR for python #15135
🔧 Implementation Notes
The selectByContainsVisibleText method already had the necessary implementation, so I just added the missing exception throwing for the selectByVisibleText method.
All tests from the following packages are passed locally: //java/test/org/openqa/selenium/support/ui/...
💡 Additional Considerations
No
🔄 Types of changes
Bug fix (backwards compatible)
PR Type
Bug fix
Description
Fix selectByVisibleText to throw exception for hidden options
Add visibility checks to prevent selecting invisible elements
Enhance test coverage for hidden option scenarios
Update Java language level to JDK 17
Diagram Walkthrough
flowchart LR
A["selectByVisibleText method"] --> B["Add visibility check"]
B --> C["Throw NoSuchElementException"]
C --> D["Enhanced test coverage"]
E["Java language level"] --> F["Update to JDK 17"]
Throwing NoSuchElementException for hidden options changes behavior; confirm this aligns with established Selenium API semantics and does not mask other cases (e.g., disabled vs hidden) or break backward compatibility across bindings.
assertSelectIsEnabled();
assertSelectIsVisible();
// try to find the option via XPATH ...List<WebElement> options =
element.findElements(
By.xpath(".//option[normalize-space(.) = " + Quotes.escape(text) + "]"));
for (WebElementoption : options) {
if (!hasCssPropertyAndVisible(option))
thrownewNoSuchElementException("Invisible option with text: " + text);
setSelected(option, true);
if (!isMultiple()) {
return;
}
Validate that hasCssPropertyAndVisible(option) accurately reflects visibility for CSS-hidden options and shadow DOM cases, and that it is performant when iterating over multiple options.
element.findElements(
By.xpath(".//option[contains(., " + Quotes.escape(subStringWithoutSpace) + ")]"));
}
Stringtrimmed = text.trim();
for (WebElementoption : candidates) {
if (trimmed.equals(option.getText().trim())) {
if (!hasCssPropertyAndVisible(option))
thrownewNoSuchElementException("Invisible option with text: " + text);
setSelected(option, true);
if (!isMultiple()) {
return;
}
This PR mixes behavioral changes with IDE-specific files and a Java language level bump to 17, which can unintentionally force build/toolchain upgrades for all consumers. Separate the Select behavior change and tests from project configuration changes, and ensure the runtime/source compatibility remains aligned with the repository’s supported JDK matrix to prevent breaking downstream users.
Why: The suggestion correctly identifies that the PR improperly bundles a functional change with a significant JDK version bump and IDE configuration changes, which can break downstream builds.
High
Possible issue
Use correct exception type
Selecting an invisible option should signal an operation not permitted, not that the element does not exist. Replace the thrown exception with an UnsupportedOperationException to align with the existing disabled-option behavior and avoid misleading error handling. Keep the message explicit about invisibility.
for (WebElement option : options) {
- if (!hasCssPropertyAndVisible(option))- throw new NoSuchElementException("Invisible option with text: " + text);+ if (!hasCssPropertyAndVisible(option)) {+ throw new UnsupportedOperationException("You may not select an invisible option with text: " + text);+ }
setSelected(option, true);
if (!isMultiple()) {
return;
}
...
for (WebElement option : candidates) {
if (trimmed.equals(option.getText().trim())) {
- if (!hasCssPropertyAndVisible(option))- throw new NoSuchElementException("Invisible option with text: " + text);+ if (!hasCssPropertyAndVisible(option)) {+ throw new UnsupportedOperationException("You may not select an invisible option with text: " + text);+ }
setSelected(option, true);
if (!isMultiple()) {
return;
}
[To ensure code accuracy, apply this suggestion manually]
Suggestion importance[1-10]: 7
__
Why: The suggestion correctly points out that throwing UnsupportedOperationException for an invisible option is more consistent with existing behavior for disabled options, improving API design.
Throwing NoSuchElementException for invisible options changes behavior; confirm this aligns with contract across bindings and does not mask cases where options exist but are not interactable. Ensure message includes enough context (value/text) and consider a dedicated exception or consistent type with other selection paths.
for (WebElementoption : options) {
if (!hasCssPropertyAndVisible(option))
thrownewNoSuchElementException("Invisible option with text: " + text);
setSelected(option, true);
Visibility checks are performed in both the exact-text and fallback loops; consider extracting to a helper to avoid duplication and ensure consistent messaging and behavior between branches.
for (WebElementoption : candidates) {
if (trimmed.equals(option.getText().trim())) {
if (!hasCssPropertyAndVisible(option))
thrownewNoSuchElementException("Invisible option with text: " + text);
setSelected(option, true);
if (!isMultiple()) {
return;
}
Avoid throwing on the first invisible match; instead, skip invisible options and only throw if no visible option matched. This aligns with typical selection semantics and avoids breaking cases where duplicates exist with only some visible.
+boolean matchedVisible = false;
for (WebElement option : options) {
- if (!hasCssPropertyAndVisible(option))- throw new NoSuchElementException("Invisible option with text: " + text);+ if (!hasCssPropertyAndVisible(option)) {+ continue;+ }
setSelected(option, true);
+ matchedVisible = true;
if (!isMultiple()) {
return;
}
}
+if (!matchedVisible) {+ throw new NoSuchElementException("No visible option with text: " + text);+}
Apply / Chat
Suggestion importance[1-10]: 9
__
Why: This suggestion correctly identifies a significant flaw in the PR's logic; throwing an exception on the first invisible option is incorrect if other visible options with the same text exist, and this change fixes the behavior to correctly handle such cases.
High
Consistent invisible-option skipping in fallback
Mirror the same skip-then-fail logic for the fallback exact-text comparison loop. This ensures consistent behavior across both XPath and manual text matching paths.
+boolean matchedVisibleFallback = false;
for (WebElement option : candidates) {
if (trimmed.equals(option.getText().trim())) {
- if (!hasCssPropertyAndVisible(option))- throw new NoSuchElementException("Invisible option with text: " + text);+ if (!hasCssPropertyAndVisible(option)) {+ continue;+ }
setSelected(option, true);
+ matchedVisibleFallback = true;
if (!isMultiple()) {
return;
}
}
}
+if (!matchedVisibleFallback) {+ throw new NoSuchElementException("No visible option with text: " + text);+}
[To ensure code accuracy, apply this suggestion manually]
Suggestion importance[1-10]: 9
__
Why: This suggestion correctly identifies a significant flaw in the PR's logic for the fallback text matching and proposes a fix that aligns with the expected behavior of selecting a visible option if one exists, thus preventing incorrect exceptions.
High
High-level
Avoid IDE config changes
This PR updates IntelliJ project files to JDK 17, which can unintentionally affect contributors’ environments and CI without aligning the entire repo’s build tooling (Gradle/Maven, toolchains) and release matrix. Move the Java version bump to the build system and repo-wide policy first, then drop IDE-specific files from the PR to prevent configuration drift.
// PR includes changes to IDE-specific configuration files
// to update the Java version.
// .idea/misc.xml
<componentname="ProjectRootManager"languageLevel="JDK_17" ...>
// java/java.iml
<componentname="NewModuleRootManager"LANGUAGE_LEVEL="JDK_17">
After:
// IDE configuration files are removed from the PR.
// The Java version update should be handled in the
// project's central build file (e.g., pom.xml, build.gradle)
// in a separate, dedicated PR.
// This PR should only contain the logic changes:
// java/src/org/openqa/selenium/support/ui/Select.java
// java/test/org/openqa/selenium/support/ui/SelectElementTest.java
Suggestion importance[1-10]: 7
__
Why: The suggestion correctly identifies that IDE-specific configuration files like .idea/misc.xml and java.iml should not be versioned, as this can cause inconsistencies for contributors; such changes should be managed centrally in build configuration files.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
selenium-ci
added
C-java
vicky-iv
changed the title
User description
🔗 Related Issues
[🚀 Feature]: Unifying Select Class Across All Bindings #15265
💥 What does this PR do?
I implemented unifying the Select class across all bindings for the Java package.
I also extended the unit tests to cover this functionality by shadowing PR for python #15135
🔧 Implementation Notes
The
selectByContainsVisibleTextmethod already had the necessary implementation, so I just added the missing exception throwing for theselectByVisibleTextmethod.All tests from the following packages are passed locally:
//java/test/org/openqa/selenium/support/ui/...💡 Additional Considerations
No
🔄 Types of changes
Bug fix (backwards compatible)
PR Type
Bug fix
Description
Fix
selectByVisibleTextto throw exception for hidden optionsAdd visibility checks to prevent selecting invisible elements
Enhance test coverage for hidden option scenarios
Update Java language level to JDK 17
Diagram Walkthrough
File Walkthrough
Select.java
Add visibility validation to selectByVisibleTextjava/src/org/openqa/selenium/support/ui/Select.java
assertSelectIsVisible()call toselectByVisibleTextmethodhasCssPropertyAndVisible()for optionsNoSuchElementExceptionwhen trying to select invisible optionsSelectElementTest.java
Enhance tests for hidden option validationjava/test/org/openqa/selenium/support/ui/SelectElementTest.java
selectByVisibleTextinstead ofselectByContainsVisibleTextshouldThrowExceptionOnSelectByVisibleTextIfOptionHiddenmisc.xml
Update IntelliJ project to JDK 17.idea/misc.xml
java.iml
Update module language level to JDK 17java/java.iml