[adr] Selenium Manager released API#17741
Open
titusfortner wants to merge 3 commits into
Open
Conversation
Contributor
PR Summary by QodoAdd ADR for independently released Selenium Manager API contract
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+1
to
+5
| # 17741. Selenium Manager ships as an official, independently released tool | ||
|
|
||
| - Status: Proposed | ||
| - Discussion: https://github.com/SeleniumHQ/selenium/pull/17741 | ||
|
|
Contributor
There was a problem hiding this comment.
1. Missing decision date 🐞 Bug ⚙ Maintainability
The decision record omits the required - Date: YYYY-MM-DD metadata line, so the record doesn’t state when its status last changed and doesn’t conform to the decision template used in this repo.
Agent Prompt
### Issue description
The new decision record is missing the required `Date` metadata line (date the status last changed).
### Issue Context
The decision template includes `Status`, `Date`, and `Discussion` fields at the top of the file.
### Fix Focus Areas
- docs/decisions/17741-selenium-manager-released-api.md[1-5]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+82
to
+85
| ## Consequences | ||
|
|
||
| - Once released, the contract constrains every `rust/` change: no switch, value, or output field | ||
| changes without a deprecation cycle, and the current defaults are locked. |
Contributor
There was a problem hiding this comment.
2. Missing binding status table 🐞 Bug ⚙ Maintainability
The record lacks a ## Binding status section/table, which the documented process uses to track per-binding implementation and which is the only section intended to be editable after acceptance.
Agent Prompt
### Issue description
The decision record is missing the `## Binding status` section and tracking table.
### Issue Context
The template includes a binding-status table, and the process doc says bindings track convergence there and that updating that table is the only post-acceptance edit allowed (besides the status line).
### Fix Focus Areas
- docs/decisions/17741-selenium-manager-released-api.md[82-85]
- docs/decisions/0000-template.md[39-51]
- docs/decisions/README.md[28-45]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
Code Review by Qodo
Context used✅ Compliance rules (platform):
11 rules 1. Missing binding status table
|
titusfortner
force-pushed
the
c/wizardly-blackwell-68a46f
branch
from
f87abc8 to
e90b495
Compare
Comment on lines
+37
to
+38
| - Current defaults must not change once released, and must all be documented | ||
| ([configuration reference](https://www.selenium.dev/documentation/selenium_manager/#configuration)). |
Contributor
There was a problem hiding this comment.
1. Defaults not enumerated 🐞 Bug ⚙ Maintainability
The ADR says “current defaults” are locked as part of the stable contract, but it doesn’t actually list which defaults are being locked and instead points to an external reference. This conflicts with the repo’s rule that decision records must be understandable without following links and leaves the contract ambiguous when defaults change.
Agent Prompt
### Issue description
The ADR locks “current defaults” but doesn’t enumerate them, requiring readers to follow an external link to know what is actually committed.
### Issue Context
`docs/decisions/README.md` states decision records must stand alone without requiring links. The ADR currently references an external configuration page instead of listing the defaults it intends to lock.
### Fix Focus Areas
- docs/decisions/17741-selenium-manager-released-api.md[8-12]
- docs/decisions/17741-selenium-manager-released-api.md[37-38]
- docs/decisions/17741-selenium-manager-released-api.md[86-87]
- docs/decisions/README.md[63-64]
### Suggested change
Add an explicit, in-record list of the defaults that are being made part of the immutable contract (and keep the link as “background/further reading” if desired).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
Code review by qodo was updated up to the latest commit e90b495 |
titusfortner
force-pushed
the
c/wizardly-blackwell-68a46f
branch
from
e90b495 to
b2cec39
Compare
Contributor
|
Code review by qodo was updated up to the latest commit b2cec39 |
titusfortner
force-pushed
the
c/wizardly-blackwell-68a46f
branch
2 times, most recently
from
f158ddb to
f79fbc5
Compare
Contributor
|
Code review by qodo was updated up to the latest commit f79fbc5 |
titusfortner
force-pushed
the
c/wizardly-blackwell-68a46f
branch
from
f79fbc5 to
9a1e7da
Compare
Comment on lines
+40
to
+47
| **Resolution.** | ||
|
|
||
| - When provided a driver location, SM validates the correct browser and downloads it if necessary, | ||
| instead of being bypassed. This is not backward compatible, so a toggle is necessary to control | ||
| desired behavior. | ||
| - A `PATH` driver whose version does not match the browser is not used; SM fetches a matching one. | ||
|
|
||
| **Telemetry.** Each time SM sends data, it logs what was sent and how to disable it. |
Contributor
There was a problem hiding this comment.
1. Unnamed contract flags 🐞 Bug ⚙ Maintainability
The ADR says behavior changes must be controlled by a “toggle” when a driver path is provided, and that telemetry logging explains how to disable it, but it never specifies the actual CLI/config knob names or defaults. This leaves the “stable contract” underspecified relative to the decision-record rules and existing implementations (e.g., --avoid-stats, and bindings currently bypass SM when a driver path is set).
Agent Prompt
## Issue description
The ADR states that (a) providing a driver location requires a toggle to control whether SM is bypassed, and (b) telemetry logging tells users how to disable telemetry, but it does not define the concrete flag/config names (and their defaults) that implement those commitments.
## Issue Context
Decision records in this repo are intended to be a standalone, normative contract. Today the SM CLI already has a concrete telemetry-disable flag (`--avoid-stats`), and bindings already implement “skip SM when driver path is provided”, so the ADR should lock the exact knobs that govern those behaviors.
## Fix Focus Areas
- docs/decisions/17741-selenium-manager-released-api.md[40-47]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
Code review by qodo was updated up to the latest commit 9a1e7da |
titusfortner
force-pushed
the
c/wizardly-blackwell-68a46f
branch
from
9a1e7da to
fc5a3b1
Compare
Comment on lines
+16
to
+51
| **Versioning and distribution.** SM takes its own semantic version from `1.0.0` and is released | ||
| standalone as well as bundled. Bindings ship and default to a known-good SM, but already let a user | ||
| point at a different binary, so a binding and the SM it runs may differ in version. | ||
|
|
||
| **Output.** The default follows standard CLI convention — result on stdout, logs on stderr — so | ||
| `$(selenium-manager …)` captures only the result. Bindings set `--output` explicitly and are | ||
| unaffected, so only the standalone audience is impacted. | ||
|
|
||
| **Forward compatibility.** Neither the bindings nor the binary may fail solely because of version | ||
| skew. How an older SM treats an input it does not recognize depends on the input: | ||
|
|
||
| - *Unknown switch* → warn (tell the user to update SM) and ignore; the binding still gets valid | ||
| paths. A switch only adjusts resolution, so dropping it is safe. | ||
| - *Unknown value of a switch that has a default* (output format, log level) → use the default and warn. | ||
| - *Unknown value of a switch with no default* → error, naming the value and telling the user to | ||
| update SM (e.g. `Selenium Manager 1.2 does not support browser 'chrome-for-testing'`). There is | ||
| nothing to fall back to; for `--browser` these are distinct browsers where substituting one would | ||
| be the wrong result. Erroring is safe because such values are only ever requested explicitly. | ||
| - New switches must be optional — an older binding will omit them. | ||
| - Nothing in the contract — switches, values, output fields — is removed or renamed without a | ||
| deprecation cycle. | ||
| - Current defaults must not change once released, and must all be documented | ||
| ([configuration reference](https://www.selenium.dev/documentation/selenium_manager/#configuration)). | ||
|
|
||
| **Resolution.** | ||
|
|
||
| - When provided a driver location, SM validates the correct browser and downloads it if necessary, | ||
| instead of being bypassed. This is not backward compatible, so a toggle is necessary to control | ||
| desired behavior. | ||
| - A `PATH` driver whose version does not match the browser is not used; SM fetches a matching one. | ||
|
|
||
| **Telemetry.** | ||
|
|
||
| - The binding must pass in the Selenium version and language (name and version). | ||
| - SM must pass in the SM version. | ||
| - Each time SM sends data, it logs what was sent and how to disable it. |
Contributor
There was a problem hiding this comment.
1. No cross-binding comparison evidence 📘 Rule violation ≡ Correctness
This ADR defines user-visible contract details impacting bindings (e.g., output behavior and telemetry requirements) but does not identify any other binding used for comparison or provide evidence of a cross-binding search. Without an explicit comparison reference, the PR does not meet the checklist requirement to validate consistency across bindings when specifying user-visible behavior.
Agent Prompt
## Issue description
The ADR specifies binding-related, user-visible behavior (e.g., `--output` handling and telemetry fields) but does not document any comparison to at least one other language binding or provide evidence of a project-wide search.
## Issue Context
Compliance requires showing which other binding (Java/Python/Ruby/.NET/JS) was checked to ensure the specified behavior is logically consistent across bindings, or documenting any intentional divergence.
## Fix Focus Areas
- docs/decisions/17741-selenium-manager-released-api.md[16-22]
- docs/decisions/17741-selenium-manager-released-api.md[49-51]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
Code review by qodo was updated up to the latest commit fc5a3b1 |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
📄 The decision, rationale, options, and consequences are in the record file this PR adds
(
docs/decisions/17741-selenium-manager-released-api.md). This body is proposal notes and logistics only.🔗 Related
📝 Proposal notes
so a binding and the Selenium Manager it invokes can differ in version. This record fixes the
forwards- and backwards-compatible contract, and the defaults, that requires.
committed default — and the obligations releasing independently creates: independent versioning and
distribution. Additive features and bugs stay ordinary issues, not contract.
🗣 Discussion
📌 Tracking
Tracking issue: (linked on acceptance)