Skip to content

Conversation

@titusfortner
Copy link
Member

@titusfortner titusfortner commented Dec 16, 2025

User description

PR Reason

Everything in the CI is already pinning browsers (except remote python tests, where I think it was just an oversight, not intentional). Better to make it the default for bazel, and have users override if they want different behavior.

💥 What does this PR do?

  • Changes default for pin_browsers from false to true
  • Removes all unnecessary references for setting it

🔧 Implementation Notes

Could have removed this entirely and used a new flag for use_selenium_manager or something, but this is more backwards compatible

💡 Additional Considerations

I can't think of a good reason not to set this at this point.

🔄 Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)

PR Type

Enhancement


Description

  • Changes default pin_browsers flag from false to true

  • Removes explicit --pin_browsers=true from CI workflows

  • Removes unused use_pinned_browser config setting

  • Updates documentation to reflect new default behavior


Diagram Walkthrough

flowchart LR
  A["pin_browsers default: false"] -->|"Change default"| B["pin_browsers default: true"]
  B -->|"Remove explicit flags"| C["CI workflows simplified"]
  B -->|"Remove config setting"| D["Unused use_pinned_browser removed"]
  B -->|"Update docs"| E["Documentation reflects new behavior"]
Loading

File Walkthrough

Relevant files
Configuration changes
BUILD.bazel
Set pin_browsers default to true                                                 

common/BUILD.bazel

  • Changes pin_browsers bool_flag build_setting_default from False to
    True
  • Removes unused use_pinned_browser config_setting that checked the flag
    value
+1/-8     
.bazelrc.remote
Remove redundant remote pin_browsers config                           

.bazelrc.remote

  • Removes build:rbe --//common:pin_browsers line from remote build
    configuration
  • No longer needed since pinning is now the default behavior
+0/-3     
ci-dotnet.yml
Remove explicit pin_browsers flag from dotnet tests           

.github/workflows/ci-dotnet.yml

  • Removes --pin_browsers=true flag from dotnet browser test command
  • Flag is now redundant with new default behavior
+1/-1     
ci-java.yml
Remove explicit pin_browsers flags from java tests             

.github/workflows/ci-java.yml

  • Removes --pin_browsers=true flag from three separate test commands
  • Affects local browser tests, remote browser tests, and remote test
    jobs
+3/-3     
ci-python.yml
Remove explicit pin_browsers flags from python tests         

.github/workflows/ci-python.yml

  • Removes --pin_browsers=true flag from three Python integration test
    commands
  • Affects Linux, Windows, and macOS test configurations
+3/-3     
ci-ruby.yml
Remove explicit pin_browsers flags from ruby tests             

.github/workflows/ci-ruby.yml

  • Removes --pin_browsers flag from two Ruby test commands
  • Affects both local and remote integration test configurations
+0/-2     
Documentation
README.md
Update documentation for new pin_browsers default               

README.md

  • Updates documentation example from --pin_browsers to
    --pin_browsers=false
  • Changes description to clarify that false uses Selenium Manager
    instead
  • Removes --pin_browsers=true from all dotnet test examples
+4/-4     

@selenium-ci selenium-ci added the B-build Includes scripting, bazel and CI integrations label Dec 16, 2025
@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Dec 16, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Dec 16, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Document default flag behavior

Update the documentation to explicitly mention that --pin_browsers is the
default for using pinned versions, alongside the existing entry for disabling it
with --pin_browsers=false.

README.md [300]

+* `--pin_browsers` (default) - pin browsers to provided versions  
 * `--pin_browsers=false` - use Selenium Manager to locate browsers/drivers
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: This is a good suggestion that improves documentation clarity by explicitly stating the new default behavior for --pin_browsers, making it easier for users to understand the flag's usage.

Medium
Clarify .NET pinning default

Update the documentation for .NET tests to clarify that browser pinning is now
the default behavior and does not require an additional flag.

README.md [489]

-.NET tests currently only work with pinned browsers, so make sure to include that.
+.NET tests currently only work with pinned browsers, which is now the default behavior, so no additional flag is needed.

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 7

__

Why: This suggestion correctly identifies outdated documentation and proposes a change that aligns it with the new default behavior, improving clarity and accuracy for users running .NET tests.

Medium
  • Update

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B-build Includes scripting, bazel and CI integrations Review effort 2/5

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants