Closed titusfortner closed 3 weeks ago
β±οΈ Estimated effort to review [1-5] | 3, because the PR introduces a significant number of changes across multiple test files and configurations. The changes involve adding exclusive conditions for BiDi in various test files and modifying after hooks. Additionally, the Bazel configuration updates to include BiDi flags increase the complexity. Reviewing this PR requires a good understanding of the existing test suite and the implications of the BiDi feature on it. |
π§ͺ Relevant tests | Yes |
β‘ Possible issues | Possible Bug: The exclusive condition `{bidi: false, reason: 'Not yest implemented with BiDi'}` is used in multiple places. There's a typo in the word "yet". This should be corrected to maintain professionalism and clarity in the codebase. |
Redundancy Concern: The PR includes a lot of repetitive changes for adding BiDi exclusivity across various test files. It might be beneficial to abstract some of this functionality or configuration to reduce redundancy and improve maintainability. | |
π Security concerns | No |
Category | Suggestion | Score |
Best practice |
Add a description to the
___
**To ensure consistency and avoid potential issues, consider adding a description to the | 8 |
Add an
___
**To ensure that the | 5 | |
Possible issue |
Add a timeout value to
___
**To avoid potential issues with | 7 |
Maintainability |
Group similar
___
**To improve readability and maintainability, consider grouping the | 7 |
Extract repeated log entry attributes into a shared method or variable to reduce duplication___ **To improve readability and maintainability, consider extracting the repeated log entryattributes into a shared method or variable. This will reduce duplication and make it easier to update the expected attributes in the future.** [rb/spec/integration/selenium/webdriver/bidi/log_inspector_spec.rb [37-46]](https://github.com/SeleniumHQ/selenium/pull/14083/files#diff-5aee15543760fb2d003b41805a79bc3a8d2c23d3c37f4b47db1725899a259fb9R37-R46) ```diff -expect(log_entry).to have_attributes( +expected_attributes = { text: 'Hello, world!', realm: nil, type: 'console', level: LogInspector::LOG_LEVEL[:INFO], method: 'log', args: [{ 'type' => 'string', 'value' => 'Hello, world!' }] -) +} +expect(log_entry).to have_attributes(expected_attributes) ``` Suggestion importance[1-10]: 6Why: Extracting repeated attributes into a shared method or variable reduces code duplication and improves maintainability, though it's not a critical change. | 6 | |
Typo |
Correct a typo in the reason for the
___
**The reason for the | 7 |
Enhancement |
Place the new
___
**To maintain alphabetical order and improve readability, consider placing the new | 6 |
Performance |
Add a guard clause to the
___
**To avoid potential issues with the | 4 |
**Action:** Format / Check format script run |
**Failed stage:** [Run Bazel](https://github.com/SeleniumHQ/selenium/actions/runs/9392398331/job/25866553857) [β] |
**Failed test name:** it 'is skipped without provided reason', exclusive: {foo: true} |
**Failure summary:**
The action failed because the test it 'is skipped without provided reason', exclusive: {foo: true} failed. The failure occurred due to an expectation mismatch in the guards.disposition value. |
Relevant error logs:```yaml 1: ##[group]Operating System 2: Ubuntu ... 975: Package 'php-symfony-debug-bundle' is not installed, so not removed 976: Package 'php-symfony-dependency-injection' is not installed, so not removed 977: Package 'php-symfony-deprecation-contracts' is not installed, so not removed 978: Package 'php-symfony-discord-notifier' is not installed, so not removed 979: Package 'php-symfony-doctrine-bridge' is not installed, so not removed 980: Package 'php-symfony-doctrine-messenger' is not installed, so not removed 981: Package 'php-symfony-dom-crawler' is not installed, so not removed 982: Package 'php-symfony-dotenv' is not installed, so not removed 983: Package 'php-symfony-error-handler' is not installed, so not removed ... 1800: ../../../../../../../../../work/selenium/selenium/javascript/node/selenium-webdriver/io/exec.js 7ms (unchanged) 1801: ../../../../../../../../../work/selenium/selenium/javascript/node/selenium-webdriver/io/index.js 15ms (unchanged) 1802: ../../../../../../../../../work/selenium/selenium/javascript/node/selenium-webdriver/io/zip.js 11ms (unchanged) 1803: ../../../../../../../../../work/selenium/selenium/javascript/node/selenium-webdriver/jsdoc_conf.json 3ms (unchanged) 1804: ../../../../../../../../../work/selenium/selenium/javascript/node/selenium-webdriver/lib/atoms/make-atoms-module.js 2ms (unchanged) 1805: ../../../../../../../../../work/selenium/selenium/javascript/node/selenium-webdriver/lib/by.js 18ms (unchanged) 1806: ../../../../../../../../../work/selenium/selenium/javascript/node/selenium-webdriver/lib/capabilities.js 15ms (unchanged) 1807: ../../../../../../../../../work/selenium/selenium/javascript/node/selenium-webdriver/lib/command.js 9ms (unchanged) 1808: ../../../../../../../../../work/selenium/selenium/javascript/node/selenium-webdriver/lib/error.js 30ms (unchanged) ... 1879: ../../../../../../../../../work/selenium/selenium/javascript/node/selenium-webdriver/test/http/util_test.js 11ms (unchanged) 1880: ../../../../../../../../../work/selenium/selenium/javascript/node/selenium-webdriver/test/ie/options_test.js 7ms (unchanged) 1881: ../../../../../../../../../work/selenium/selenium/javascript/node/selenium-webdriver/test/io/io_test.js 29ms (unchanged) 1882: ../../../../../../../../../work/selenium/selenium/javascript/node/selenium-webdriver/test/io/zip_test.js 17ms (unchanged) 1883: ../../../../../../../../../work/selenium/selenium/javascript/node/selenium-webdriver/test/lib/api_test.js 4ms (unchanged) 1884: ../../../../../../../../../work/selenium/selenium/javascript/node/selenium-webdriver/test/lib/by_test.js 12ms (unchanged) 1885: ../../../../../../../../../work/selenium/selenium/javascript/node/selenium-webdriver/test/lib/capabilities_test.js 14ms (unchanged) 1886: ../../../../../../../../../work/selenium/selenium/javascript/node/selenium-webdriver/test/lib/credentials_test.js 12ms (unchanged) 1887: ../../../../../../../../../work/selenium/selenium/javascript/node/selenium-webdriver/test/lib/error_test.js 16ms (unchanged) ... 2224: @@ -77,7 +77,8 @@ module Selenium 2225: guards = described_class.new(example) 2226: guards.add_condition(:foo, false) 2227: - expect(guards.disposition).to eq [:pending, 'Test guarded; Guarded by {:foo=>false, :reason=>"No reason given"};'] 2228: + expect(guards.disposition).to eq [:pending, 2229: + 'Test guarded; Guarded by {:foo=>false, :reason=>"No reason given"};'] 2230: end 2231: it 'is skipped without provided reason', exclusive: {foo: true} do |example| 2232: ##[error]Process completed with exit code 1. ``` |
this should be good now. Looks much better, thanks
User description
I was going to just commit the Ruby pieces directly, but it doesn't really make sense to do that until we know how we are running the tests with Bazel.
So, I'm happy with the Ruby code, but I need feedback on the bazel toggle and how we want to start running both modes in our test suite
Description
--bidi
Motivation and Context
This matters as we start to implement Selenium features with both WebDriver classic and WebDriver bidi. We want to make sure that both modes continue to work for people.
PR Type
Enhancement, Tests
Description
reset_driver!
calls.reset_driver!
method.Changes walkthrough π
36 files
action_builder_spec.rb
Add BiDi exclusive condition to ActionBuilder tests
rb/spec/integration/selenium/webdriver/action_builder_spec.rb - Added exclusive condition for BiDi in `ActionBuilder` tests.
browsing_context_spec.rb
Add BiDi exclusive condition and modify after hook in BrowsingContext
tests
rb/spec/integration/selenium/webdriver/bidi/browsing_context_spec.rb
BrowsingContext
tests.log_inspector_spec.rb
Add BiDi exclusive condition and refactor LogInspector tests
rb/spec/integration/selenium/webdriver/bidi/log_inspector_spec.rb
LogInspector
tests.reset_driver!
calls.script_spec.rb
Add BiDi exclusive condition and modify Script tests
rb/spec/integration/selenium/webdriver/bidi/script_spec.rb
Script
tests.bidi_spec.rb
Add BiDi exclusive condition and modify BiDi tests
rb/spec/integration/selenium/webdriver/bidi_spec.rb
BiDi
tests.driver_spec.rb
Add BiDi exclusive condition to Chrome Driver tests
rb/spec/integration/selenium/webdriver/chrome/driver_spec.rb - Added exclusive condition for BiDi in `Chrome::Driver` tests.
options_spec.rb
Add BiDi exclusive condition to Chrome Options tests
rb/spec/integration/selenium/webdriver/chrome/options_spec.rb - Added exclusive condition for BiDi in `Chrome::Options` tests.
profile_spec.rb
Add BiDi exclusive condition to Chrome Profile tests
rb/spec/integration/selenium/webdriver/chrome/profile_spec.rb - Added exclusive condition for BiDi in `Chrome::Profile` tests.
service_spec.rb
Add BiDi exclusive condition to Chrome Service tests
rb/spec/integration/selenium/webdriver/chrome/service_spec.rb - Added exclusive condition for BiDi in `Chrome::Service` tests.
devtools_spec.rb
Add BiDi exclusive condition and modify DevTools tests
rb/spec/integration/selenium/webdriver/devtools_spec.rb
DevTools
tests.driver_spec.rb
Add BiDi exclusive condition to Driver tests
rb/spec/integration/selenium/webdriver/driver_spec.rb - Added exclusive condition for BiDi in `Driver` tests.
driver_spec.rb
Add BiDi exclusive condition to Edge Driver tests
rb/spec/integration/selenium/webdriver/edge/driver_spec.rb - Added exclusive condition for BiDi in `Edge::Driver` tests.
options_spec.rb
Add BiDi exclusive condition to Edge Options tests
rb/spec/integration/selenium/webdriver/edge/options_spec.rb - Added exclusive condition for BiDi in `Edge::Options` tests.
profile_spec.rb
Add BiDi exclusive condition to Edge Profile tests
rb/spec/integration/selenium/webdriver/edge/profile_spec.rb - Added exclusive condition for BiDi in `Edge::Profile` tests.
service_spec.rb
Add BiDi exclusive condition to Edge Service tests
rb/spec/integration/selenium/webdriver/edge/service_spec.rb - Added exclusive condition for BiDi in `Edge::Service` tests.
element_spec.rb
Add BiDi exclusive condition to Element tests
rb/spec/integration/selenium/webdriver/element_spec.rb - Added exclusive condition for BiDi in `Element` tests.
error_spec.rb
Add BiDi exclusive condition to Error tests
rb/spec/integration/selenium/webdriver/error_spec.rb - Added exclusive condition for BiDi in `Error` tests.
driver_spec.rb
Add BiDi exclusive condition to Firefox Driver tests
rb/spec/integration/selenium/webdriver/firefox/driver_spec.rb - Added exclusive condition for BiDi in `Firefox::Driver` tests.
profile_spec.rb
Add BiDi exclusive condition to Firefox Profile tests
rb/spec/integration/selenium/webdriver/firefox/profile_spec.rb - Added exclusive condition for BiDi in `Firefox::Profile` tests.
service_spec.rb
Add BiDi exclusive condition to Firefox Service tests
rb/spec/integration/selenium/webdriver/firefox/service_spec.rb - Added exclusive condition for BiDi in `Firefox::Service` tests.
guard_spec.rb
Add BiDi exclusive condition to Guards tests
rb/spec/integration/selenium/webdriver/guard_spec.rb - Added exclusive condition for BiDi in `Guards` tests.
listener_spec.rb
Add BiDi exclusive condition to Listener tests
rb/spec/integration/selenium/webdriver/listener_spec.rb - Added exclusive condition for BiDi in `Listener` tests.
manager_spec.rb
Add BiDi exclusive condition to Manager tests
rb/spec/integration/selenium/webdriver/manager_spec.rb - Added exclusive condition for BiDi in `Manager` tests.
navigation_spec.rb
Add BiDi exclusive condition to Navigation tests
rb/spec/integration/selenium/webdriver/navigation_spec.rb - Added exclusive condition for BiDi in `Navigation` tests.
driver_spec.rb
Add BiDi exclusive condition to Remote Driver tests
rb/spec/integration/selenium/webdriver/remote/driver_spec.rb - Added exclusive condition for BiDi in `Remote::Driver` tests.
element_spec.rb
Add BiDi exclusive condition to Remote Element tests
rb/spec/integration/selenium/webdriver/remote/element_spec.rb - Added exclusive condition for BiDi in `Remote::Element` tests.
driver_spec.rb
Add BiDi exclusive condition to Safari Driver tests
rb/spec/integration/selenium/webdriver/safari/driver_spec.rb - Added exclusive condition for BiDi in `Safari::Driver` tests.
select_spec.rb
Add BiDi exclusive condition to Select tests
rb/spec/integration/selenium/webdriver/select_spec.rb - Added exclusive condition for BiDi in `Select` tests.
shadow_root_spec.rb
Add BiDi exclusive condition to ShadowRoot tests
rb/spec/integration/selenium/webdriver/shadow_root_spec.rb - Added exclusive condition for BiDi in `ShadowRoot` tests.
storage_spec.rb
Add BiDi exclusive condition to HasWebStorage tests
rb/spec/integration/selenium/webdriver/storage_spec.rb - Added exclusive condition for BiDi in `HasWebStorage` tests.
takes_screenshot_spec.rb
Add BiDi exclusive condition and refactor TakesScreenshot tests
rb/spec/integration/selenium/webdriver/takes_screenshot_spec.rb
TakesScreenshot
tests.target_locator_spec.rb
Add BiDi exclusive condition and modify after hook in TargetLocator
tests
rb/spec/integration/selenium/webdriver/target_locator_spec.rb
TargetLocator
tests.timeout_spec.rb
Add BiDi exclusive condition to Timeouts tests
rb/spec/integration/selenium/webdriver/timeout_spec.rb - Added exclusive condition for BiDi in `Timeouts` tests.
virtual_authenticator_spec.rb
Add BiDi exclusive condition to VirtualAuthenticator tests
rb/spec/integration/selenium/webdriver/virtual_authenticator_spec.rb
VirtualAuthenticator
tests.window_spec.rb
Add BiDi exclusive condition to Window tests
rb/spec/integration/selenium/webdriver/window_spec.rb - Added exclusive condition for BiDi in `Window` tests.
zipper_spec.rb
Add BiDi exclusive condition and refactor Zipper tests
rb/spec/integration/selenium/webdriver/zipper_spec.rb
Zipper
tests.2 files
spec_helper.rb
Add BiDi condition to guards and modify after hook
rb/spec/integration/selenium/webdriver/spec_helper.rb
test_environment.rb
Add BiDi condition to reset_driver and build_options
rb/spec/integration/selenium/webdriver/spec_support/test_environment.rb
reset_driver!
method.build_options
to setweb_socket_url
based on environmentvariable.
3 files
tests.bzl
Add BiDi condition to Bazel test configurations
rb/spec/tests.bzl - Added BiDi condition to Bazel test configurations.
.bazelrc
Add BiDi flag alias to Bazel configuration
.bazelrc - Added BiDi flag alias to Bazel configuration.
BUILD.bazel
Add BiDi flag and configuration settings to Bazel build
common/BUILD.bazel - Added BiDi flag and configuration settings.