[java] show browser version and available CDP versions

[asolntsev]

... in the error message "You are using a no-op implementation of the CDP".

Otherwise, it's very hard to understand what CDP version was expected, any why it was not found.

🔗 Related Issues

💥 What does this PR do?

This PR improves errror/warning "You are using a no-op implementation of the CDP".

🤖 AI assistance

• No substantial AI assistance used
• AI assisted (complete below)
  • Tool(s):
  • What was generated:
  • I reviewed all AI output and can explain the change

🔄 Types of changes

• New feature (non-breaking change which adds functionality and tests!)

[qodo-code-review]

PR Summary by Qodo

Improve no-op CDP warning with browser and available CDP versions
✨ Enhancement 🕐 10-20 Minutes

Description

• Include browser version and discovered CDP implementations in no-op CDP exceptions.
• Centralize CDP matching + no-op fallback creation in CdpVersionFinder.
• Deprecate context-free no-op constructors in favor of contextual variants.

Diagram

graph TD
  A["ChromiumDriver / DevToolsProvider"] --> B["CdpVersionFinder"] --> C{"CDP match found?"}
  C -->|"yes"| D["CdpInfo (real)"] --> E["DevTools"]
  C -->|"no"| F["NoOpCdpInfo (context)"] --> G["NoOpDomains"] --> H["DevToolsException (detailed)"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Keep warning construction at call sites (driver/provider)

• ➕ Avoids passing extra context through NoOpCdpInfo/NoOpDomains constructors
• ➕ Limits changes to fewer classes
• ➖ Duplicates fallback logic across multiple entry points
• ➖ Harder to keep messaging consistent across ChromiumDriver vs DevToolsProvider

2. Log diagnostics once during version resolution (plus short exception)

• ➕ Preserves a concise exception message while still emitting actionable data
• ➕ Avoids repeating long formatted strings on every domain accessor
• ➖ Users may miss logs in some environments (CI, remote grids)
• ➖ Still needs a reliable way to surface the info when logs aren’t captured

Recommendation: Centralizing the fallback in CdpVersionFinder is the right direction because it ensures consistent behavior across all DevTools entry points and keeps the contextual data close to where it’s available (the loaded CdpInfo set). One thing to double-check during review: the new formatted WARNING string in NoOpDomains has three %s placeholders, but message() currently passes arguments in an order that appears mismatched (browser version vs Selenium release label), which could make the emitted message incorrect.

Files changed (5) +60 / -34

Enhancement (3) +58 / -14

CdpVersionFinder.javaAdd findMatchingVersion() helper with contextual no-op fallback +9/-0

Add findMatchingVersion() helper with contextual no-op fallback

• Introduces findMatchingVersion(String) to return a matching CdpInfo or a NoOpCdpInfo that includes the browser version and the set of available implementations. This consolidates the match-or-fallback pattern into a single API.

java/src/org/openqa/selenium/devtools/CdpVersionFinder.java

NoOpCdpInfo.javaIntroduce contextual NoOpCdpInfo constructor and deprecate no-arg +6/-0

Introduce contextual NoOpCdpInfo constructor and deprecate no-arg

• Marks the no-arg NoOpCdpInfo() constructor as deprecated and adds a new constructor that passes browser version and available CDP implementations through to NoOpDomains for richer error messages.

java/src/org/openqa/selenium/devtools/noop/NoOpCdpInfo.java

NoOpDomains.javaEnhance no-op CDP exception message with version context +43/-14

Enhance no-op CDP exception message with version context

• Expands the no-op CDP warning to include the detected browser version and a list of available CDP major versions. Adds stateful constructors to carry this context and routes all domain accessors to a shared message() formatter.

java/src/org/openqa/selenium/devtools/noop/NoOpDomains.java

Refactor (2) +2 / -20

ChromiumDriver.javaUse centralized CDP version resolution for DevTools setup +1/-18

Use centralized CDP version resolution for DevTools setup

• Replaces inline CDP matching and warning/no-op construction with CdpVersionFinder.findMatchingVersion(). Removes now-unneeded imports tied to the old warning and NoOpCdpInfo fallback.

java/src/org/openqa/selenium/chromium/ChromiumDriver.java

DevToolsProvider.javaAdopt centralized CDP version matching in DevTools augmenter +1/-2

Adopt centralized CDP version matching in DevTools augmenter

• Switches DevToolsProvider to use CdpVersionFinder.findMatchingVersion(version) instead of manually constructing a NoOpCdpInfo fallback. Keeps behavior consistent with ChromiumDriver initialization.

java/src/org/openqa/selenium/devtools/DevToolsProvider.java

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (5) 📘 Rule violations (7) 📜 Skill insights (0)

Context used

✅ Compliance rules (platform): 18 rules

1. Null collection triggers NPE 🐞 Bug ☼ Reliability

Description

NoOpDomains(String, Collection) stores availableCdpImplementations without a null check, but
message() unconditionally calls availableCdpImplementations.stream(), which can throw a
NullPointerException instead of the intended DevToolsException diagnostic. This is a new public
API footgun introduced by this PR (the internal call path currently passes a non-null set, but
external callers can hit this).

Code

java/src/org/openqa/selenium/devtools/noop/NoOpDomains.java[R61-64]

+  public NoOpDomains(String browserVersion, Collection<CdpInfo> availableCdpImplementations) {
+    this.browserVersion = browserVersion;
+    this.availableCdpImplementations = availableCdpImplementations;
+  }

Evidence

The constructor stores the collection without validation, and message() always streams it; this
will NPE if null is provided. Nearby code (e.g., CdpVersionFinder constructors) demonstrates the
project’s established pattern of Require.nonNull + Set.copyOf for similar inputs.

java/src/org/openqa/selenium/devtools/noop/NoOpDomains.java[61-64]
java/src/org/openqa/selenium/devtools/noop/NoOpDomains.java[96-100]
java/src/org/openqa/selenium/devtools/CdpVersionFinder.java[51-57]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`NoOpDomains(String, Collection<CdpInfo>)` assigns incoming arguments directly to fields. `message()` later dereferences the collection via `.stream()`, so a null collection will cause an NPE and mask the intended no-op CDP guidance.

### Issue Context
This constructor is public and therefore callable by library users.

### Fix Focus Areas
- Add `Require.nonNull(...)` checks for `browserVersion` and `availableCdpImplementations` (or normalize null to `emptyList()` if you want to allow nulls).
- Consider defensive copying (e.g., `List.copyOf(...)` / `Set.copyOf(...)`) to avoid surprises if a mutable collection is passed.

- java/src/org/openqa/selenium/devtools/noop/NoOpDomains.java[61-64]
- java/src/org/openqa/selenium/devtools/noop/NoOpDomains.java[96-100]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. NoOpDomains() missing forRemoval 📘 Rule violation ✧ Quality

Description

NoOpDomains() is annotated with @Deprecated but does not set forRemoval = true. This violates
the deprecation annotation requirement.

Code

java/src/org/openqa/selenium/devtools/noop/NoOpDomains.java[R56-59]

+  @Deprecated
+  public NoOpDomains() {
+    this("?", emptyList());
+  }

Evidence

The rule requires all deprecated elements to use @Deprecated(forRemoval = true). The
NoOpDomains() constructor is annotated with @Deprecated without the required attribute.

Rule 330196: Use @Deprecated(forRemoval = true) for all deprecated elements
java/src/org/openqa/selenium/devtools/noop/NoOpDomains.java[53-59]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`NoOpDomains()` uses `@Deprecated` without `forRemoval = true`.

## Issue Context
Compliance requires every `@Deprecated` usage to explicitly set `forRemoval = true`.

## Fix Focus Areas
- java/src/org/openqa/selenium/devtools/noop/NoOpDomains.java[56-59]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. NoOpCdpInfo() deprecation incomplete 📘 Rule violation ✧ Quality

Description

NoOpCdpInfo() is annotated with @Deprecated without forRemoval = true, and the deprecation
provides no guidance on the replacement API. This violates the deprecation annotation requirements
and makes migrations harder for users.

Code

java/src/org/openqa/selenium/devtools/noop/NoOpCdpInfo.java[R25-28]

+  @Deprecated
  public NoOpCdpInfo() {
    super(1, dt -> new NoOpDomains());
  }

Evidence

The checklist requires @Deprecated(forRemoval = true) for deprecated elements and requires
guidance naming the alternative for deprecated public APIs. The constructor NoOpCdpInfo() is
annotated with bare @Deprecated and has no Javadoc @deprecated guidance.

Rule 330196: Use @Deprecated(forRemoval = true) for all deprecated elements
Rule 389271: Deprecate public APIs with guidance before removal
java/src/org/openqa/selenium/devtools/noop/NoOpCdpInfo.java[23-28]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`NoOpCdpInfo()` is deprecated without `forRemoval = true` and without deprecation guidance pointing to the replacement.

## Issue Context
The compliance checklist requires all `@Deprecated` annotations to set `forRemoval = true`, and deprecated public APIs to include guidance (typically via Javadoc `@deprecated`) naming the alternative.

## Fix Focus Areas
- java/src/org/openqa/selenium/devtools/noop/NoOpCdpInfo.java[25-28]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

View more (4)

4. NoOpDomains constructor missing Javadoc 📘 Rule violation ✧ Quality

Description

The new public constructor NoOpDomains(String, Collection<CdpInfo>) is added without any Javadoc.
This violates the requirement to document public API constructors, including parameters.

Code

java/src/org/openqa/selenium/devtools/noop/NoOpDomains.java[R61-64]

+  public NoOpDomains(String browserVersion, Collection<CdpInfo> availableCdpImplementations) {
+    this.browserVersion = browserVersion;
+    this.availableCdpImplementations = availableCdpImplementations;
+  }

Evidence

The rule requires Javadoc immediately preceding public API constructors. The new public
NoOpDomains(String, Collection<CdpInfo>) constructor has no Javadoc comment.

Rule 330200: Require Javadoc for all public API types and methods
java/src/org/openqa/selenium/devtools/noop/NoOpDomains.java[61-64]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new public constructor `NoOpDomains(String browserVersion, Collection<CdpInfo> availableCdpImplementations)` has no Javadoc.

## Issue Context
Compliance requires Javadoc for public API constructors, and for constructors with parameters it must include `@param` tags for each parameter.

## Fix Focus Areas
- java/src/org/openqa/selenium/devtools/noop/NoOpDomains.java[61-64]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

5. findMatchingVersion Javadoc missing tags 📘 Rule violation ✧ Quality

Description

The new public method findMatchingVersion(String browserVersion) has Javadoc but lacks required
@param and @return tags. This violates the requirement for complete Javadoc on changed public
methods.

Code

java/src/org/openqa/selenium/devtools/CdpVersionFinder.java[R97-103]

+  /**
+   * Takes a `browserVersion` from a {@link org.openqa.selenium.Capabilities} instance and returns
+   * the matching CDP version.
+   */
+  public CdpInfo findMatchingVersion(String browserVersion) {
+    return match(browserVersion).orElseGet(() -> new NoOpCdpInfo(browserVersion, infos));
+  }

Evidence

The rule requires public methods to have complete Javadoc including @param tags for parameters and
@return for non-void returns. The added Javadoc for findMatchingVersion contains only free text
and no tags despite having a parameter and non-void return type.

Rule 330201: Require complete Javadoc on public API methods
java/src/org/openqa/selenium/devtools/CdpVersionFinder.java[97-103]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`findMatchingVersion(String browserVersion)` is public and has Javadoc, but the Javadoc is missing required `@param browserVersion` and `@return` tags.

## Issue Context
Compliance requires complete Javadoc on changed public API methods, including free-text plus `@param` for each parameter and `@return` for non-void return types.

## Fix Focus Areas
- java/src/org/openqa/selenium/devtools/CdpVersionFinder.java[97-103]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

6. NoOpCdpInfo constructor missing Javadoc 📘 Rule violation ✧ Quality

Description

The new public constructor NoOpCdpInfo(String, Collection<CdpInfo>) is added without any Javadoc.
This violates the requirement to document public API constructors, including parameters.

Code

java/src/org/openqa/selenium/devtools/noop/NoOpCdpInfo.java[R30-32]

+  public NoOpCdpInfo(String browserVersion, Collection<CdpInfo> availableCdpImplementations) {
+    super(1, dt -> new NoOpDomains(browserVersion, availableCdpImplementations));
+  }

Evidence

The rule requires Javadoc immediately preceding public API constructors. The new public
NoOpCdpInfo(String, Collection<CdpInfo>) constructor has no Javadoc comment.

Rule 330200: Require Javadoc for all public API types and methods
java/src/org/openqa/selenium/devtools/noop/NoOpCdpInfo.java[29-32]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new public constructor `NoOpCdpInfo(String browserVersion, Collection<CdpInfo> availableCdpImplementations)` has no Javadoc.

## Issue Context
Compliance requires Javadoc for public API methods/constructors, and for constructors with parameters it must include `@param` tags for each parameter.

## Fix Focus Areas
- java/src/org/openqa/selenium/devtools/noop/NoOpCdpInfo.java[30-32]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

7. Wrong format arg order 🐞 Bug ≡ Correctness

Description

NoOpDomains.message() passes String.format arguments in the wrong order, causing the thrown
DevToolsException to show Selenium’s release label as the browser version and the CDP versions list
as the Maven artifact version. This makes the new diagnostic message misleading and defeats the PR’s
purpose.

Code

java/src/org/openqa/selenium/devtools/noop/NoOpDomains.java[R96-100]

+  private String message() {
+    List<Integer> cdpVersions =
+        availableCdpImplementations.stream().map(info -> info.getMajorVersion()).collect(toList());
+    return String.format(WARNING, INFO.getReleaseLabel(), browserVersion, cdpVersions);
  }

Evidence

The WARNING template has %s placeholders in the order: browserVersion, available implementations,
then Selenium release label for the dependency hint; however message() supplies
INFO.getReleaseLabel() first, shifting all values into the wrong slots.

java/src/org/openqa/selenium/devtools/noop/NoOpDomains.java[39-49]
java/src/org/openqa/selenium/devtools/noop/NoOpDomains.java[96-100]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`NoOpDomains.WARNING` defines placeholders in this order:
1) browser version, 2) available CDP implementations, 3) Selenium release label (for the dependency suggestion).

But `NoOpDomains.message()` currently calls `String.format` with the arguments in a different order, producing a misleading message.

### Issue Context
This message is thrown via `DevToolsException` whenever the no-op CDP domains are used, so it’s the primary diagnostic users will see when CDP support is missing.

### Fix Focus Areas
- java/src/org/openqa/selenium/devtools/noop/NoOpDomains.java[96-100]

### What to change
Update the `String.format` call to match the placeholder order, e.g.:
- `String.format(WARNING, browserVersion, cdpVersions, INFO.getReleaseLabel())`

(Optionally add/adjust a unit test if one exists for this message formatting.)

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

8. findMatchingVersion lacks test coverage 📘 Rule violation ▣ Testability

Description

The new behavior is centralized in CdpVersionFinder.findMatchingVersion(...), but the existing
unit tests continue to exercise match(...) and do not cover the new method or its no-op fallback
path. This increases the risk of regressions in the new error-message behavior.

Code

java/src/org/openqa/selenium/devtools/CdpVersionFinder.java[R101-103]

+  public CdpInfo findMatchingVersion(String browserVersion) {
+    return match(browserVersion).orElseGet(() -> new NoOpCdpInfo(browserVersion, infos));
+  }

Evidence

The rule requires tests for new behavior. The PR adds findMatchingVersion(...), but the existing
test class continues to call match(...) and contains no assertions around
findMatchingVersion(...) or the new fallback behavior.

Rule 389273: Require tests for all new functionality and bug fixes
java/src/org/openqa/selenium/devtools/CdpVersionFinder.java[97-103]
java/test/org/openqa/selenium/devtools/CdpVersionFinderTest.java[71-119]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`findMatchingVersion(...)` is newly added and used by production code, but there are no unit tests exercising it (especially the `NoOpCdpInfo(...)` fallback path).

## Issue Context
Compliance requires tests for new functionality/behavior changes, with assertions that would fail if the change were reverted.

## Fix Focus Areas
- java/src/org/openqa/selenium/devtools/CdpVersionFinder.java[97-103]
- java/test/org/openqa/selenium/devtools/CdpVersionFinderTest.java[71-119]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

9. Unsorted CDP versions list 🐞 Bug ⚙ Maintainability

Description

NoOpDomains.message() builds the displayed list of available CDP versions from a set-backed
collection without sorting, so the output order can vary between runs. This reduces the usefulness
of the new diagnostic message when users try to quickly scan supported versions.

Code

java/src/org/openqa/selenium/devtools/noop/NoOpDomains.java[R96-99]

+  private String message() {
+    List<Integer> cdpVersions =
+        availableCdpImplementations.stream().map(info -> info.getMajorVersion()).collect(toList());
+    return String.format(WARNING, INFO.getReleaseLabel(), browserVersion, cdpVersions);

Evidence

message() collects major versions without sorting, and the source collection originates from
CdpVersionFinder which stores implementations in a Set, so iteration order is not guaranteed.

java/src/org/openqa/selenium/devtools/noop/NoOpDomains.java[96-99]
java/src/org/openqa/selenium/devtools/CdpVersionFinder.java[36-39]
java/src/org/openqa/selenium/devtools/CdpVersionFinder.java[51-57]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The warning message prints `Available CDP implementations: %s` by collecting major versions into a `List<Integer>` without sorting. Because `availableCdpImplementations` is typically set-backed, the list order is nondeterministic and can be harder to read.

### Issue Context
`CdpVersionFinder` stores discovered CDP implementations in a `Set` (`Set.copyOf(...)`), and the no-op warning message is intended to aid debugging.

### Fix Focus Areas
- java/src/org/openqa/selenium/devtools/noop/NoOpDomains.java[96-99]

### What to change
In `message()`, build a stable list, e.g.:
- `availableCdpImplementations.stream()`
 - `.map(CdpInfo::getMajorVersion)`
 - `.distinct()`
 - `.sorted()`
 - `.collect(toList())`

This yields deterministic, easier-to-scan output.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

10. No cross-binding comparison noted 📘 Rule violation ≡ Correctness

Description

A user-visible error message was changed to include browser/CDP version details, but no in-code
documentation indicates it was compared against other language bindings or intentionally diverges.
This risks inconsistent UX across bindings.

Code

java/src/org/openqa/selenium/devtools/noop/NoOpDomains.java[R40-48]

+      "You are using a no-op implementation of the CDP. The most likely reason"
+          + " for this is that Selenium was unable to find an implementation of the "
+          + "CDP protocol that matches your browser. "
+          + "Browser version: %s.%n"
+          + "Available CDP implementations: %s.%n"
+          + "Please be sure to include an "
+          + "implementation on the classpath, possibly by adding a new (maven) "
+          + "dependency of `org.seleniumhq.selenium:selenium-devtools-vNN:%s` where "
+          + "`NN` matches the major version of the browser you're using.";

Evidence

The rule requires evidence of comparing analogous user-visible behavior across bindings or
documenting intentional divergence. The changed warning string is user-visible, and the modified
code does not include any comment/reference indicating a cross-binding comparison.

Rule 389265: Compare cross-language bindings when changing user-visible behavior
java/src/org/openqa/selenium/devtools/noop/NoOpDomains.java[39-48]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The PR changes a user-visible error message, but there is no nearby documentation indicating cross-language binding comparison or an intentional divergence.

## Issue Context
Compliance requires comparing cross-language bindings (or documenting intentional differences) when changing user-visible behavior such as error messages.

## Fix Focus Areas
- java/src/org/openqa/selenium/devtools/noop/NoOpDomains.java[39-48]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

11. Mutable collection retained 🐞 Bug ☼ Reliability ⭐ New

Description

NoOpDomains stores the caller-provided availableCdpImplementations collection by reference and later
iterates it in message(), so external mutation after construction can change or break the diagnostic
generation path.
This is avoidable by snapshotting the collection (e.g., Set.copyOf/List.copyOf) in the constructor
so the no-op error message is stable and mutation-safe.

Code

java/src/org/openqa/selenium/devtools/noop/NoOpDomains.java[R62-66]

+  public NoOpDomains(String browserVersion, Collection<CdpInfo> availableCdpImplementations) {
+    this.browserVersion = browserVersion;
+    this.availableCdpImplementations =
+        Require.nonNull("Available CDP implementations", availableCdpImplementations);
+  }

Evidence

The constructor assigns the provided collection directly to a field, and message() later iterates
that same field via stream to build the displayed versions; there is no defensive copy between those
steps.

java/src/org/openqa/selenium/devtools/noop/NoOpDomains.java[51-66]
java/src/org/openqa/selenium/devtools/noop/NoOpDomains.java[98-102]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`NoOpDomains(String, Collection<CdpInfo>)` stores the passed `Collection<CdpInfo>` directly and later streams it in `message()`. Because the constructor is public, callers can pass a mutable collection; if that collection is modified after `NoOpDomains` is created, the generated diagnostic can become inconsistent or fail unexpectedly.

### Issue Context
This constructor is part of the no-op CDP fallback path and is executed before the `DevToolsException` is thrown; the formatted message is computed later when a domains method is invoked.

### Fix Focus Areas
- java/src/org/openqa/selenium/devtools/noop/NoOpDomains.java[62-66]
- java/src/org/openqa/selenium/devtools/noop/NoOpDomains.java[98-101]

### Suggested fix
In the constructor, defensively snapshot the passed collection (e.g., `this.availableCdpImplementations = List.copyOf(...)` or `Set.copyOf(...)`) so later iteration is stable and not dependent on external mutation.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

12. Unnecessary CdpInfo retention 🐞 Bug ⚙ Maintainability

Description

CdpVersionFinder.findMatchingVersion passes the entire internal infos set into
NoOpCdpInfo/NoOpDomains, but the no-op error message only uses CdpInfo.getMajorVersion(). This
retains the full Collection<CdpInfo> (and re-iterates it when formatting the message) when storing
just the major versions would be sufficient.

Code

java/src/org/openqa/selenium/devtools/CdpVersionFinder.java[R105-107]

+  public CdpInfo findMatchingVersion(String browserVersion) {
+    return match(browserVersion).orElseGet(() -> new NoOpCdpInfo(browserVersion, infos));
+  }

Evidence

The fallback wires infos into NoOpCdpInfo, and NoOpDomains.message() immediately maps each
element to getMajorVersion(), showing only major versions are needed for the new message content.

java/src/org/openqa/selenium/devtools/CdpVersionFinder.java[105-107]
java/src/org/openqa/selenium/devtools/noop/NoOpCdpInfo.java[33-35]
java/src/org/openqa/selenium/devtools/noop/NoOpDomains.java[50-52]
java/src/org/openqa/selenium/devtools/noop/NoOpDomains.java[96-100]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The no-op diagnostic only needs the set of available CDP *major versions*, but the code currently threads and stores the full `Collection<CdpInfo>`.

### Issue Context
This only affects the no-op fallback path, but it’s easy to tighten the design by passing a `Set<Integer>` (or preformatted string) instead of full `CdpInfo` instances.

### Fix Focus Areas
- Change `NoOpCdpInfo`/`NoOpDomains` to accept `Collection<Integer>` (or `Set<Integer>`) of major versions (or a precomputed display string), rather than `Collection<CdpInfo>`.
- Precompute the displayed version set once in the constructor to avoid re-iterating.

- java/src/org/openqa/selenium/devtools/CdpVersionFinder.java[105-107]
- java/src/org/openqa/selenium/devtools/noop/NoOpCdpInfo.java[33-35]
- java/src/org/openqa/selenium/devtools/noop/NoOpDomains.java[50-52]
- java/src/org/openqa/selenium/devtools/noop/NoOpDomains.java[96-100]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Previous review results

Review updated until commit 8b85011

Results up to commit b5b3025

🐞 Bugs (2) 📘 Rule violations (7) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. NoOpCdpInfo() deprecation incomplete 📘 Rule violation ✧ Quality

Description

NoOpCdpInfo() is annotated with @Deprecated without forRemoval = true, and the deprecation
provides no guidance on the replacement API. This violates the deprecation annotation requirements
and makes migrations harder for users.

Code

java/src/org/openqa/selenium/devtools/noop/NoOpCdpInfo.java[R25-28]

+  @Deprecated
  public NoOpCdpInfo() {
    super(1, dt -> new NoOpDomains());
  }

Evidence

The checklist requires @Deprecated(forRemoval = true) for deprecated elements and requires
guidance naming the alternative for deprecated public APIs. The constructor NoOpCdpInfo() is
annotated with bare @Deprecated and has no Javadoc @deprecated guidance.

Rule 330196: Use @Deprecated(forRemoval = true) for all deprecated elements
Rule 389271: Deprecate public APIs with guidance before removal
java/src/org/openqa/selenium/devtools/noop/NoOpCdpInfo.java[23-28]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`NoOpCdpInfo()` is deprecated without `forRemoval = true` and without deprecation guidance pointing to the replacement.

## Issue Context
The compliance checklist requires all `@Deprecated` annotations to set `forRemoval = true`, and deprecated public APIs to include guidance (typically via Javadoc `@deprecated`) naming the alternative.

## Fix Focus Areas
- java/src/org/openqa/selenium/devtools/noop/NoOpCdpInfo.java[25-28]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. NoOpDomains() missing forRemoval 📘 Rule violation ✧ Quality

Description

NoOpDomains() is annotated with @Deprecated but does not set forRemoval = true. This violates
the deprecation annotation requirement.

Code

java/src/org/openqa/selenium/devtools/noop/NoOpDomains.java[R56-59]

+  @Deprecated
+  public NoOpDomains() {
+    this("?", emptyList());
+  }

Evidence

The rule requires all deprecated elements to use @Deprecated(forRemoval = true). The
NoOpDomains() constructor is annotated with @Deprecated without the required attribute.

Rule 330196: Use @Deprecated(forRemoval = true) for all deprecated elements
java/src/org/openqa/selenium/devtools/noop/NoOpDomains.java[53-59]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`NoOpDomains()` uses `@Deprecated` without `forRemoval = true`.

## Issue Context
Compliance requires every `@Deprecated` usage to explicitly set `forRemoval = true`.

## Fix Focus Areas
- java/src/org/openqa/selenium/devtools/noop/NoOpDomains.java[56-59]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. findMatchingVersion Javadoc missing tags 📘 Rule violation ✧ Quality

Description

The new public method findMatchingVersion(String browserVersion) has Javadoc but lacks required
@param and @return tags. This violates the requirement for complete Javadoc on changed public
methods.

Code

java/src/org/openqa/selenium/devtools/CdpVersionFinder.java[R97-103]

+  /**
+   * Takes a `browserVersion` from a {@link org.openqa.selenium.Capabilities} instance and returns
+   * the matching CDP version.
+   */
+  public CdpInfo findMatchingVersion(String browserVersion) {
+    return match(browserVersion).orElseGet(() -> new NoOpCdpInfo(browserVersion, infos));
+  }

Evidence

The rule requires public methods to have complete Javadoc including @param tags for parameters and
@return for non-void returns. The added Javadoc for findMatchingVersion contains only free text
and no tags despite having a parameter and non-void return type.

Rule 330201: Require complete Javadoc on public API methods
java/src/org/openqa/selenium/devtools/CdpVersionFinder.java[97-103]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`findMatchingVersion(String browserVersion)` is public and has Javadoc, but the Javadoc is missing required `@param browserVersion` and `@return` tags.

## Issue Context
Compliance requires complete Javadoc on changed public API methods, including free-text plus `@param` for each parameter and `@return` for non-void return types.

## Fix Focus Areas
- java/src/org/openqa/selenium/devtools/CdpVersionFinder.java[97-103]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

View more (3)

4. NoOpCdpInfo constructor missing Javadoc 📘 Rule violation ✧ Quality

Description

The new public constructor NoOpCdpInfo(String, Collection<CdpInfo>) is added without any Javadoc.
This violates the requirement to document public API constructors, including parameters.

Code

java/src/org/openqa/selenium/devtools/noop/NoOpCdpInfo.java[R30-32]

+  public NoOpCdpInfo(String browserVersion, Collection<CdpInfo> availableCdpImplementations) {
+    super(1, dt -> new NoOpDomains(browserVersion, availableCdpImplementations));
+  }

Evidence

The rule requires Javadoc immediately preceding public API constructors. The new public
NoOpCdpInfo(String, Collection<CdpInfo>) constructor has no Javadoc comment.

Rule 330200: Require Javadoc for all public API types and methods
java/src/org/openqa/selenium/devtools/noop/NoOpCdpInfo.java[29-32]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new public constructor `NoOpCdpInfo(String browserVersion, Collection<CdpInfo> availableCdpImplementations)` has no Javadoc.

## Issue Context
Compliance requires Javadoc for public API methods/constructors, and for constructors with parameters it must include `@param` tags for each parameter.

## Fix Focus Areas
- java/src/org/openqa/selenium/devtools/noop/NoOpCdpInfo.java[30-32]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

5. NoOpDomains constructor missing Javadoc 📘 Rule violation ✧ Quality

Description

The new public constructor NoOpDomains(String, Collection<CdpInfo>) is added without any Javadoc.
This violates the requirement to document public API constructors, including parameters.

Code

java/src/org/openqa/selenium/devtools/noop/NoOpDomains.java[R61-64]

+  public NoOpDomains(String browserVersion, Collection<CdpInfo> availableCdpImplementations) {
+    this.browserVersion = browserVersion;
+    this.availableCdpImplementations = availableCdpImplementations;
+  }

Evidence

The rule requires Javadoc immediately preceding public API constructors. The new public
NoOpDomains(String, Collection<CdpInfo>) constructor has no Javadoc comment.

Rule 330200: Require Javadoc for all public API types and methods
java/src/org/openqa/selenium/devtools/noop/NoOpDomains.java[61-64]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new public constructor `NoOpDomains(String browserVersion, Collection<CdpInfo> availableCdpImplementations)` has no Javadoc.

## Issue Context
Compliance requires Javadoc for public API constructors, and for constructors with parameters it must include `@param` tags for each parameter.

## Fix Focus Areas
- java/src/org/openqa/selenium/devtools/noop/NoOpDomains.java[61-64]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

6. Wrong format arg order 🐞 Bug ≡ Correctness

Description

NoOpDomains.message() passes String.format arguments in the wrong order, causing the thrown
DevToolsException to show Selenium’s release label as the browser version and the CDP versions list
as the Maven artifact version. This makes the new diagnostic message misleading and defeats the PR’s
purpose.

Code

java/src/org/openqa/selenium/devtools/noop/NoOpDomains.java[R96-100]

+  private String message() {
+    List<Integer> cdpVersions =
+        availableCdpImplementations.stream().map(info -> info.getMajorVersion()).collect(toList());
+    return String.format(WARNING, INFO.getReleaseLabel(), browserVersion, cdpVersions);
  }

Evidence

The WARNING template has %s placeholders in the order: browserVersion, available implementations,
then Selenium release label for the dependency hint; however message() supplies
INFO.getReleaseLabel() first, shifting all values into the wrong slots.

java/src/org/openqa/selenium/devtools/noop/NoOpDomains.java[39-49]
java/src/org/openqa/selenium/devtools/noop/NoOpDomains.java[96-100]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`NoOpDomains.WARNING` defines placeholders in this order:
1) browser version, 2) available CDP implementations, 3) Selenium release label (for the dependency suggestion).

But `NoOpDomains.message()` currently calls `String.format` with the arguments in a different order, producing a misleading message.

### Issue Context
This message is thrown via `DevToolsException` whenever the no-op CDP domains are used, so it’s the primary diagnostic users will see when CDP support is missing.

### Fix Focus Areas
- java/src/org/openqa/selenium/devtools/noop/NoOpDomains.java[96-100]

### What to change
Update the `String.format` call to match the placeholder order, e.g.:
- `String.format(WARNING, browserVersion, cdpVersions, INFO.getReleaseLabel())`

(Optionally add/adjust a unit test if one exists for this message formatting.)

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

7. No cross-binding comparison noted 📘 Rule violation ≡ Correctness

Description

A user-visible error message was changed to include browser/CDP version details, but no in-code
documentation indicates it was compared against other language bindings or intentionally diverges.
This risks inconsistent UX across bindings.

Code

java/src/org/openqa/selenium/devtools/noop/NoOpDomains.java[R40-48]

+      "You are using a no-op implementation of the CDP. The most likely reason"
+          + " for this is that Selenium was unable to find an implementation of the "
+          + "CDP protocol that matches your browser. "
+          + "Browser version: %s.%n"
+          + "Available CDP implementations: %s.%n"
+          + "Please be sure to include an "
+          + "implementation on the classpath, possibly by adding a new (maven) "
+          + "dependency of `org.seleniumhq.selenium:selenium-devtools-vNN:%s` where "
+          + "`NN` matches the major version of the browser you're using.";

Evidence

The rule requires evidence of comparing analogous user-visible behavior across bindings or
documenting intentional divergence. The changed warning string is user-visible, and the modified
code does not include any comment/reference indicating a cross-binding comparison.

Rule 389265: Compare cross-language bindings when changing user-visible behavior
java/src/org/openqa/selenium/devtools/noop/NoOpDomains.java[39-48]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The PR changes a user-visible error message, but there is no nearby documentation indicating cross-language binding comparison or an intentional divergence.

## Issue Context
Compliance requires comparing cross-language bindings (or documenting intentional differences) when changing user-visible behavior such as error messages.

## Fix Focus Areas
- java/src/org/openqa/selenium/devtools/noop/NoOpDomains.java[39-48]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

8. findMatchingVersion lacks test coverage 📘 Rule violation ▣ Testability

Description

The new behavior is centralized in CdpVersionFinder.findMatchingVersion(...), but the existing
unit tests continue to exercise match(...) and do not cover the new method or its no-op fallback
path. This increases the risk of regressions in the new error-message behavior.

Code

java/src/org/openqa/selenium/devtools/CdpVersionFinder.java[R101-103]

+  public CdpInfo findMatchingVersion(String browserVersion) {
+    return match(browserVersion).orElseGet(() -> new NoOpCdpInfo(browserVersion, infos));
+  }

Evidence

The rule requires tests for new behavior. The PR adds findMatchingVersion(...), but the existing
test class continues to call match(...) and contains no assertions around
findMatchingVersion(...) or the new fallback behavior.

Rule 389273: Require tests for all new functionality and bug fixes
java/src/org/openqa/selenium/devtools/CdpVersionFinder.java[97-103]
java/test/org/openqa/selenium/devtools/CdpVersionFinderTest.java[71-119]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`findMatchingVersion(...)` is newly added and used by production code, but there are no unit tests exercising it (especially the `NoOpCdpInfo(...)` fallback path).

## Issue Context
Compliance requires tests for new functionality/behavior changes, with assertions that would fail if the change were reverted.

## Fix Focus Areas
- java/src/org/openqa/selenium/devtools/CdpVersionFinder.java[97-103]
- java/test/org/openqa/selenium/devtools/CdpVersionFinderTest.java[71-119]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

9. Unsorted CDP versions list 🐞 Bug ⚙ Maintainability

Description

NoOpDomains.message() builds the displayed list of available CDP versions from a set-backed
collection without sorting, so the output order can vary between runs. This reduces the usefulness
of the new diagnostic message when users try to quickly scan supported versions.

Code

java/src/org/openqa/selenium/devtools/noop/NoOpDomains.java[R96-99]

+  private String message() {
+    List<Integer> cdpVersions =
+        availableCdpImplementations.stream().map(info -> info.getMajorVersion()).collect(toList());
+    return String.format(WARNING, INFO.getReleaseLabel(), browserVersion, cdpVersions);

Evidence

message() collects major versions without sorting, and the source collection originates from
CdpVersionFinder which stores implementations in a Set, so iteration order is not guaranteed.

java/src/org/openqa/selenium/devtools/noop/NoOpDomains.java[96-99]
java/src/org/openqa/selenium/devtools/CdpVersionFinder.java[36-39]
java/src/org/openqa/selenium/devtools/CdpVersionFinder.java[51-57]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The warning message prints `Available CDP implementations: %s` by collecting major versions into a `List<Integer>` without sorting. Because `availableCdpImplementations` is typically set-backed, the list order is nondeterministic and can be harder to read.

### Issue Context
`CdpVersionFinder` stores discovered CDP implementations in a `Set` (`Set.copyOf(...)`), and the no-op warning message is intended to aid debugging.

### Fix Focus Areas
- java/src/org/openqa/selenium/devtools/noop/NoOpDomains.java[96-99]

### What to change
In `message()`, build a stable list, e.g.:
- `availableCdpImplementations.stream()`
 - `.map(CdpInfo::getMajorVersion)`
 - `.distinct()`
 - `.sorted()`
 - `.collect(toList())`

This yields deterministic, easier-to-scan output.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Results up to commit 226f532

🐞 Bugs (2) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Null collection triggers NPE 🐞 Bug ☼ Reliability

Description

NoOpDomains(String, Collection) stores availableCdpImplementations without a null check, but
message() unconditionally calls availableCdpImplementations.stream(), which can throw a
NullPointerException instead of the intended DevToolsException diagnostic. This is a new public
API footgun introduced by this PR (the internal call path currently passes a non-null set, but
external callers can hit this).

Code

java/src/org/openqa/selenium/devtools/noop/NoOpDomains.java[R61-64]

+  public NoOpDomains(String browserVersion, Collection<CdpInfo> availableCdpImplementations) {
+    this.browserVersion = browserVersion;
+    this.availableCdpImplementations = availableCdpImplementations;
+  }

Evidence

The constructor stores the collection without validation, and message() always streams it; this
will NPE if null is provided. Nearby code (e.g., CdpVersionFinder constructors) demonstrates the
project’s established pattern of Require.nonNull + Set.copyOf for similar inputs.

java/src/org/openqa/selenium/devtools/noop/NoOpDomains.java[61-64]
java/src/org/openqa/selenium/devtools/noop/NoOpDomains.java[96-100]
java/src/org/openqa/selenium/devtools/CdpVersionFinder.java[51-57]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`NoOpDomains(String, Collection<CdpInfo>)` assigns incoming arguments directly to fields. `message()` later dereferences the collection via `.stream()`, so a null collection will cause an NPE and mask the intended no-op CDP guidance.

### Issue Context
This constructor is public and therefore callable by library users.

### Fix Focus Areas
- Add `Require.nonNull(...)` checks for `browserVersion` and `availableCdpImplementations` (or normalize null to `emptyList()` if you want to allow nulls).
- Consider defensive copying (e.g., `List.copyOf(...)` / `Set.copyOf(...)`) to avoid surprises if a mutable collection is passed.

- java/src/org/openqa/selenium/devtools/noop/NoOpDomains.java[61-64]
- java/src/org/openqa/selenium/devtools/noop/NoOpDomains.java[96-100]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Unnecessary CdpInfo retention 🐞 Bug ⚙ Maintainability

Description

CdpVersionFinder.findMatchingVersion passes the entire internal infos set into
NoOpCdpInfo/NoOpDomains, but the no-op error message only uses CdpInfo.getMajorVersion(). This
retains the full Collection<CdpInfo> (and re-iterates it when formatting the message) when storing
just the major versions would be sufficient.

Code

[java/src/org/openqa/selenium/devtools/CdpVersionFinder.java[R105-107]](https://github.com/SeleniumHQ/selenium/pull/17708/files#diff-89348ba054af6b23e5144acb53224308ce18ef409713e16705fa6a8ecff6836bR...

[qodo-code-review]

Code review by qodo was updated up to the latest commit 226f532

[qodo-code-review]

Code review by qodo was updated up to the latest commit 8b85011

[diemol]

Thank you, this makes sense. Once CI is green, please merge!