Skip to content

Conversation

aguspe
Copy link
Contributor

@aguspe aguspe commented Sep 29, 2025

User description

This pull request makes a small change to the gem specification by adding a new dependency.

  • Added the prism gem (version ~> 1.0, < 1.5) as a dependency in rb/selenium-webdriver.gemspec to ensure compatibility with JRuby

🔄 Types of changes

  • Bug fix (backwards compatible)

PR Type

Bug fix


Description

  • Add prism dependency constraint to Ruby gemspec

  • Fix version compatibility issues in unit tests

  • Restrict prism to versions 1.0 to 1.5


Diagram Walkthrough

flowchart LR
  A["Ruby Gemspec"] --> B["Add prism dependency"] --> C["Fix Unit Tests"]
Loading

File Walkthrough

Relevant files
Bug fix
selenium-webdriver.gemspec
Add prism dependency constraint                                                   

rb/selenium-webdriver.gemspec

  • Add prism gem dependency with version constraint ~> 1.0, < 1.5
  • Fix dependency compatibility issues for Ruby unit tests
+1/-0     

@selenium-ci selenium-ci added the C-rb Ruby Bindings label Sep 29, 2025
Copy link
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

1234 - Partially compliant

Compliant requirements:

Non-compliant requirements:

  • Investigate and fix regression where click() no longer triggers JavaScript in link href in Firefox (2.48.x vs 2.47.1).
  • Ensure behavior works on Firefox 42.0 (32-bit) per report.
  • Provide tests or validation demonstrating the fix.

Requires further human verification:

  • Manual validation on affected Firefox version to confirm click() triggers JavaScript in href.
  • Cross-version/browser regression testing to ensure no behavior changes elsewhere.

5678 - Partially compliant

Compliant requirements:

Non-compliant requirements:

  • Diagnose "ConnectFailure (Connection refused)" when instantiating multiple ChromeDriver instances.
  • Provide a code fix or configuration change to prevent connection failures after the first instance.
  • Add tests or reproducible steps validating multiple instances work reliably.

Requires further human verification:

  • Runtime verification by launching multiple ChromeDriver instances across environments.
⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Version Constraint

The prism dependency is capped to < 1.5 while the description claims 1.0 to 1.5; verify whether 1.5.x should be allowed (<= 1.5) or intentionally excluded (< 1.5) to avoid unintended incompatibilities.

s.add_dependency 'prism', ['~> 1.0', '< 1.5']
s.add_dependency 'rexml', ['~> 3.2', '>= 3.2.5']
Dependency Impact

Adding prism as a runtime dependency may affect downstream consumers; confirm it is required at runtime vs development/test only, and ensure compatibility with supported Ruby versions.

s.add_dependency 'prism', ['~> 1.0', '< 1.5']
s.add_dependency 'rexml', ['~> 3.2', '>= 3.2.5']

Copy link
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Use development dependency for test-only gems

If the prism gem is only for testing, change its declaration from a runtime
dependency (add_dependency) to a development dependency
(add_development_dependency).

rb/selenium-webdriver.gemspec [54]

-s.add_dependency 'prism', ['~> 1.0', '< 1.5']
+s.add_development_dependency 'prism', ['~> 1.0', '< 1.5']
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies a potential issue where a development dependency is declared as a runtime dependency, which is an important best practice for gem maintainability and user experience.

Medium
  • More

@aguspe aguspe changed the title Rb fix unit tests [rb] fix unit tests Sep 29, 2025
@diemol diemol merged commit 4c603ec into SeleniumHQ:trunk Sep 29, 2025
22 of 25 checks passed
@exterm
Copy link

exterm commented Oct 3, 2025

why restrict prism to < 1.5 ? This means we need to downgrade prism in our app to upgrade selenium-webdriver.

@G-Rath
Copy link

G-Rath commented Oct 5, 2025

Similarly it looks like v4.36.0 (#16332) has done something similar with the json gem - neither of these should be needed by this gem as it is not responsible for ensuring the versions installed are compatible with specific platforms, that's the role of the downstream consumer and their bundler; instead the gem should just be saying what versions it is compatible with.

I can confirm that I've been using selenium-webdriver without issue on the latest versions of prism and json without issue on MRI Ruby for a while

@diemol
Copy link
Member

diemol commented Oct 6, 2025

You both have a point. The issue we faced is in our build (we need to use JRuby); the dependencies showed a conflict with Rubocop. We will check this again when releasing 4.37.

cgoldberg referenced this pull request Oct 7, 2025
* update devtools versions

* update selenium manager versions

* update maven dependency versions

* update authors file

* bump versions in preparation for release

* WIP - rough auto-update of changelog, please edit

* Update sha256 for signed selenium-manager-windows.exe artifact

* Updating tests and linting readme.

* Updating Gemfile.lock

* Rolling back json upgrade

* Pinning rust lock

* Pinning rust lock

* Removing guards and switching to macOS

[skip ci]

* Removing guards

---------

Co-authored-by: Selenium CI Bot <[email protected]>
Co-authored-by: Diego Molina <[email protected]>
Co-authored-by: Boni Garcia <[email protected]>
Co-authored-by: Diego Molina <[email protected]>
Earlopain added a commit to Earlopain/selenium that referenced this pull request Oct 15, 2025
This project does not use prism at all. At most it is being used by rubocop during local development.

It was added in SeleniumHQ#16357 but it is unclear to me why.
I just bumped it for this repo, perhaps that JRuby issue has since been fixed.
Either way, it should not be part of the gemspec.
diemol added a commit that referenced this pull request Oct 16, 2025
* [rb] Remove `prism` dependency

This project does not use prism at all. At most it is being used by rubocop during local development.

It was added in #16357 but it is unclear to me why.
I just bumped it for this repo, perhaps that JRuby issue has since been fixed.
Either way, it should not be part of the gemspec.

* Pin prism to 1.4.0 for the project

It fails to build with the following error:
src/prism.c: In function ‘context_terminator’:
src/prism.c:8651:62: error: conversion to ‘unsigned int’ from ‘int’ may change
the sign of the result [-Werror=sign-conversion]
8651 |     return token->type < 32 && (context_terminators[context] & (1 <<
token->type));
      |                                                              ^
cc1: all warnings being treated as errors

---------

Co-authored-by: Diego Molina <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants