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
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
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.
+* `--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.
-.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.
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
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.
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?
🔧 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
PR Type
Enhancement
Description
Changes default
pin_browsersflag from false to trueRemoves explicit
--pin_browsers=truefrom CI workflowsRemoves unused
use_pinned_browserconfig settingUpdates documentation to reflect new default behavior
Diagram Walkthrough
File Walkthrough
BUILD.bazel
Set pin_browsers default to truecommon/BUILD.bazel
pin_browsersbool_flagbuild_setting_defaultfromFalsetoTrueuse_pinned_browserconfig_setting that checked the flagvalue
.bazelrc.remote
Remove redundant remote pin_browsers config.bazelrc.remote
build:rbe --//common:pin_browsersline from remote buildconfiguration
ci-dotnet.yml
Remove explicit pin_browsers flag from dotnet tests.github/workflows/ci-dotnet.yml
--pin_browsers=trueflag from dotnet browser test commandci-java.yml
Remove explicit pin_browsers flags from java tests.github/workflows/ci-java.yml
--pin_browsers=trueflag from three separate test commandsjobs
ci-python.yml
Remove explicit pin_browsers flags from python tests.github/workflows/ci-python.yml
--pin_browsers=trueflag from three Python integration testcommands
ci-ruby.yml
Remove explicit pin_browsers flags from ruby tests.github/workflows/ci-ruby.yml
--pin_browsersflag from two Ruby test commandsREADME.md
Update documentation for new pin_browsers defaultREADME.md
--pin_browsersto--pin_browsers=falseinstead
--pin_browsers=truefrom all dotnet test examples