[rb] implement navigation commands with BiDi

[titusfortner]

User description

Description

• creates context manager class, constructed with bridge instance
• pulls page load strategy from capabilities and uses in required methods
• we eventually want to figure out how to store current browsing context as state on the bridge (I think)? For now this always gets the current window handle first
• deprecates BrowsingContext

Considerations

• navigate and reload return 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?
• Should these BiDi classes be marked private api?
• I want to implement this as 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

• Added ContextManager class to handle navigation commands using BiDi.
• Deprecated BrowsingContext class in favor of ContextManager.
• Enhanced BiDiBridge with new navigation methods: get, go_back, go_forward, and refresh.
• Added integration tests for navigation methods.

---

Changes walkthrough 📝

	Relevant files
Enhancement	bidi.rb Add `ContextManager` to autoload list in BiDi module          rb/lib/selenium/webdriver/bidi.rb Added ContextManager to autoload list. +1/-0      browsing_context.rb Deprecate `BrowsingContext` class in favor of `ContextManager` rb/lib/selenium/webdriver/bidi/browsing_context.rb Added deprecation warning for BrowsingContext class. +4/-0      context_manager.rb Implement `ContextManager` class with navigation methods  rb/lib/selenium/webdriver/bidi/context_manager.rb Created ContextManager class. Implemented navigation methods: navigate, traverse_history, and reload. +59/-0    bidi_bridge.rb Add navigation methods and `context_manager` to `BiDiBridge` rb/lib/selenium/webdriver/remote/bidi_bridge.rb Added navigation methods: get, go_back, go_forward, and refresh. Introduced private context_manager method. +22/-0
Tests	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. +33/-30

---

💡 PR-Agent usage:
Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

[codiumai-pr-agent-pro]

PR Review 🔍

⏱️ 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

[codiumai-pr-agent-pro]

PR Code Suggestions ✨

Category	Suggestion	Score
Possible issue	Add a default value for @readiness to handle cases where page_load_strategy is not present The @readiness instance variable is being set based on the page_load_strategy from bridge.capabilities. It would be prudent to handle cases where page_load_strategy might not be present in the capabilities, to avoid potential nil errors. rb/lib/selenium/webdriver/bidi/context_manager.rb [36-37] page_load_strategy = bridge.capabilities[:page_load_strategy] -@readiness = READINESS_STATE[page_load_strategy] +@readiness = READINESS_STATE.fetch(page_load_strategy, 'complete') Apply this suggestion Suggestion importance[1-10]: 8 Why: This suggestion correctly identifies a potential issue where page_load_strategy might be missing, which could lead to a nil error. Adding a default value is a crucial improvement for robustness.	8
Validation	Add URL validation to the navigate method to ensure the provided URL is valid The navigate method sends a command to navigate to a URL. It would be beneficial to validate the url parameter to ensure it is a valid URL before sending the command, to prevent potential errors. rb/lib/selenium/webdriver/bidi/context_manager.rb [40-43] +require 'uri' + def navigate(url, context_id: nil) + raise ArgumentError, 'Invalid URL' unless url =~ URI::DEFAULT_PARSER.make_regexp + context_id ||= @bridge.window_handle @bidi.send_cmd('browsingContext.navigate', context: context_id, url: url, wait: @readiness) end Apply this suggestion Suggestion importance[1-10]: 7 Why: Validating the URL before navigating helps prevent errors from invalid URLs, which is a good practice for robustness, though not as critical as handling potential nil errors.	7
Thread safety	Use a mutex to ensure thread safety when initializing @context_manager The context_manager method initializes a new ContextManager instance if it doesn't already exist. To ensure thread safety, consider using a mutex to avoid potential race conditions when initializing @context_manager. rb/lib/selenium/webdriver/remote/bidi_bridge.rb [60-62] def context_manager - @context_manager ||= WebDriver::BiDi::ContextManager.new(self) + @context_manager_mutex ||= Mutex.new + @context_manager_mutex.synchronize do + @context_manager ||= WebDriver::BiDi::ContextManager.new(self) + end end Apply this suggestion Suggestion importance[1-10]: 7 Why: Ensuring thread safety with a mutex is a good practice, especially in a multi-threaded environment, to avoid race conditions during the initialization of shared resources.	7
Testing	Add assertions to verify the navigate, back, and forward methods are called the expected number of times The navigation tests could benefit from adding assertions to verify that the navigate, back, forward, and refresh methods are called the expected number of times, ensuring the methods are invoked correctly. rb/spec/integration/selenium/webdriver/navigation_spec.rb [30-46] +expect(driver.navigate).to receive(:to).with(form_url).once driver.navigate.to form_url expect(driver.title).to eq(form_title) driver.find_element(id: 'imageButton').click wait.until { driver.title != form_title } expect(driver.current_url).to include(result_url) expect(driver.title).to eq(result_title) +expect(driver.navigate).to receive(:back).once driver.navigate.back expect(driver.current_url).to include(form_url) expect(driver.title).to eq(form_title) +expect(driver.navigate).to receive(:forward).once driver.navigate.forward expect(driver.current_url).to include(result_url) expect(driver.title).to eq(result_title) Apply this suggestion Suggestion importance[1-10]: 6 Why: Adding assertions for method calls enhances the test's ability to verify correct behavior, although it's more about refining tests than fixing a critical issue.	6

[codiumai-pr-agent-pro]

CI Failure Feedback 🧐

(Checks updated until commit c84615f)

Action: Format / Check format script run

Failed stage: Run Bazel [❌]

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. The log shows a deprecation warning related to DidYouMean::SPELL_CHECKERS.merge! which should be replaced with DidYouMean.correct_error. The error seems to occur after a code block involving describe BrowsingContext and reset_driver!.

Relevant error logs: 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.

---

✨ CI feedback usage guide:

---

The CI feedback tool (/checks) automatically triggers when a PR has a failed check.
The tool analyzes the failed checks and provides several feedbacks:

• Failed stage
• Failed test name
• Failure summary
• Relevant error logs

In addition to being automatically triggered, the tool can also be invoked manually by commenting on a PR:

/checks "https://github.com/{repo_name}/actions/runs/{run_number}/job/{job_number}"

where {repo_name} is the name of the repository, {run_number} is the run number of the failed check, and {job_number} is the job number of the failed check.

Configuration options

• enable_auto_checks_feedback - if set to true, the tool will automatically provide feedback when a check is failed. Default is true.
• excluded_checks_list - a list of checks to exclude from the feedback, for example: ["check1", "check2"]. Default is an empty list.
• enable_help_text - if set to true, the tool will provide a help message with the feedback. Default is true.
• persistent_comment - if set to true, the tool will overwrite a previous checks comment with the new feedback. Default is true.
• final_update_message - if persistent_comment is true and updating a previous checks message, the tool will also create a new message: "Persistent checks updated to latest commit". Default is true.

See more information about the checks tool in the docs.

[p0deje]

navigate and reload return 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?

I think it's fine not to return anything so far.

Should these BiDi classes be marked private api?

Yes, please.

I want to implement this as 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?

I think it should be fine to change it since it was part of BiDi which is not widely used.

[titusfortner]

/improve