Open titusfortner opened 3 weeks ago
⏱️ Estimated effort to review [1-5] | 3, because the PR introduces a new class `ContextManager` and deprecates an existing one, `BrowsingContext`. It also modifies navigation methods in `BiDiBridge` and adds integration tests. The changes are moderate in size and involve understanding the new navigation logic and its integration with the existing system. |
🧪 Relevant tests | Yes |
⚡ Possible issues | Possible Bug: The `navigate`, `traverse_history`, and `reload` methods in `ContextManager` assume that `@bridge.window_handle` will always provide a valid context ID. This might not be the case if the window has been closed or not initialized properly. |
Deprecation Concern: The deprecation of `BrowsingContext` needs a clear migration path and thorough documentation to ensure that existing users can transition smoothly without breaking their implementations. | |
🔒 Security concerns | No |
Category | Suggestion | Score |
Possible issue |
Add a default value for
___
**The | 8 |
Validation |
Add URL validation to the
___
**The | 7 |
Thread safety |
Use a mutex to ensure thread safety when initializing
___
**The | 7 |
Testing |
Add assertions to verify the
___
**The navigation tests could benefit from adding assertions to verify that the | 6 |
**Action:** Format / Check format script run |
**Failed stage:** [Run Bazel](https://github.com/SeleniumHQ/selenium/actions/runs/9587394378/job/26437333676) [❌] |
**Failure summary:**
The action failed because the process completed with exit code 1. This indicates that there was an error during the execution of the tests or commands. DidYouMean::SPELL_CHECKERS.merge! which should be replaced with DidYouMean.correct_error .describe BrowsingContext and reset_driver! . |
Relevant error logs:```yaml 1: ##[group]Operating System 2: Ubuntu ... 971: Package 'php-symfony-debug-bundle' is not installed, so not removed 972: Package 'php-symfony-dependency-injection' is not installed, so not removed 973: Package 'php-symfony-deprecation-contracts' is not installed, so not removed 974: Package 'php-symfony-discord-notifier' is not installed, so not removed 975: Package 'php-symfony-doctrine-bridge' is not installed, so not removed 976: Package 'php-symfony-doctrine-messenger' is not installed, so not removed 977: Package 'php-symfony-dom-crawler' is not installed, so not removed 978: Package 'php-symfony-dotenv' is not installed, so not removed 979: Package 'php-symfony-error-handler' is not installed, so not removed ... 1817: ../../../../../../../../../work/selenium/selenium/javascript/node/selenium-webdriver/io/exec.js 6ms (unchanged) 1818: ../../../../../../../../../work/selenium/selenium/javascript/node/selenium-webdriver/io/index.js 12ms (unchanged) 1819: ../../../../../../../../../work/selenium/selenium/javascript/node/selenium-webdriver/io/zip.js 15ms (unchanged) 1820: ../../../../../../../../../work/selenium/selenium/javascript/node/selenium-webdriver/jsdoc_conf.json 2ms (unchanged) 1821: ../../../../../../../../../work/selenium/selenium/javascript/node/selenium-webdriver/lib/atoms/make-atoms-module.js 3ms (unchanged) 1822: ../../../../../../../../../work/selenium/selenium/javascript/node/selenium-webdriver/lib/by.js 18ms (unchanged) 1823: ../../../../../../../../../work/selenium/selenium/javascript/node/selenium-webdriver/lib/capabilities.js 16ms (unchanged) 1824: ../../../../../../../../../work/selenium/selenium/javascript/node/selenium-webdriver/lib/command.js 7ms (unchanged) 1825: ../../../../../../../../../work/selenium/selenium/javascript/node/selenium-webdriver/lib/error.js 21ms (unchanged) ... 1897: ../../../../../../../../../work/selenium/selenium/javascript/node/selenium-webdriver/test/http/util_test.js 7ms (unchanged) 1898: ../../../../../../../../../work/selenium/selenium/javascript/node/selenium-webdriver/test/ie/options_test.js 5ms (unchanged) 1899: ../../../../../../../../../work/selenium/selenium/javascript/node/selenium-webdriver/test/io/io_test.js 35ms (unchanged) 1900: ../../../../../../../../../work/selenium/selenium/javascript/node/selenium-webdriver/test/io/zip_test.js 7ms (unchanged) 1901: ../../../../../../../../../work/selenium/selenium/javascript/node/selenium-webdriver/test/lib/api_test.js 6ms (unchanged) 1902: ../../../../../../../../../work/selenium/selenium/javascript/node/selenium-webdriver/test/lib/by_test.js 14ms (unchanged) 1903: ../../../../../../../../../work/selenium/selenium/javascript/node/selenium-webdriver/test/lib/capabilities_test.js 9ms (unchanged) 1904: ../../../../../../../../../work/selenium/selenium/javascript/node/selenium-webdriver/test/lib/credentials_test.js 10ms (unchanged) 1905: ../../../../../../../../../work/selenium/selenium/javascript/node/selenium-webdriver/test/lib/error_test.js 16ms (unchanged) ... 2061: [32m[1,136 / 1,850][0m Running bundle install (@bundle//:bundle); 16s linux-sandbox ... (4 actions running) 2062: [32m[1,194 / 1,850][0m Running bundle install (@bundle//:bundle); 17s linux-sandbox ... (4 actions, 3 running) 2063: [32m[1,251 / 1,850][0m Running bundle install (@bundle//:bundle); 18s linux-sandbox ... (4 actions, 3 running) 2064: [32m[1,301 / 1,850][0m Running bundle install (@bundle//:bundle); 19s linux-sandbox ... (4 actions, 3 running) 2065: [32m[1,349 / 1,850][0m Running bundle install (@bundle//:bundle); 20s linux-sandbox ... (4 actions running) 2066: [32m[1,361 / 1,850][0m Running bundle install (@bundle//:bundle); 21s linux-sandbox ... (4 actions, 3 running) 2067: [32mINFO: [0mFrom Running bundle install (@@rules_ruby~~ruby~bundle//:bundle): 2068: Installing rake 13.2.1 2069: Calling `DidYouMean::SPELL_CHECKERS.merge!(error_name => spell_checker)' has been deprecated. Please call `DidYouMean.correct_error(error_name, spell_checker)' instead. ... 2261: - describe BrowsingContext, exclusive: { bidi: true, reason: 'only executed when bidi is enabled' }, 2262: - only: { browser: %i[chrome edge firefox] } do 2263: + describe BrowsingContext, exclusive: {bidi: true, reason: 'only executed when bidi is enabled'}, 2264: + only: {browser: %i[chrome edge firefox]} do 2265: after { |example| reset_driver!(example: example) } 2266: + 2267: let(:bridge) { driver.instance_variable_get(:@bridge) } 2268: describe '#create' do 2269: ##[error]Process completed with exit code 1. ``` |
/improve
User description
Description
BrowsingContext
Considerations
NavigateResult
objects; should I return it as a Struct or just ignore it since we aren't doing anything with it and not promising a return value?BrowsingContext
instead of creating a new class and deprecating that one; do we need to be backwards compatible with this since we didn't mark it private api?Motivation and Context
Ruby implementation of #13995
PR Type
Enhancement, Tests
Description
ContextManager
class to handle navigation commands using BiDi.BrowsingContext
class in favor ofContextManager
.BiDiBridge
with new navigation methods:get
,go_back
,go_forward
, andrefresh
.Changes walkthrough 📝
bidi.rb
Add `ContextManager` to autoload list in BiDi module
rb/lib/selenium/webdriver/bidi.rb - Added `ContextManager` to autoload list.
browsing_context.rb
Deprecate `BrowsingContext` class in favor of `ContextManager`
rb/lib/selenium/webdriver/bidi/browsing_context.rb - Added deprecation warning for `BrowsingContext` class.
context_manager.rb
Implement `ContextManager` class with navigation methods
rb/lib/selenium/webdriver/bidi/context_manager.rb
ContextManager
class.navigate
,traverse_history
, andreload
.bidi_bridge.rb
Add navigation methods and `context_manager` to `BiDiBridge`
rb/lib/selenium/webdriver/remote/bidi_bridge.rb
get
,go_back
,go_forward
, andrefresh
.context_manager
method.navigation_spec.rb
Add integration tests for navigation methods
rb/spec/integration/selenium/webdriver/navigation_spec.rb - Added tests for navigation methods: back, forward, and refresh.