diff --git a/rb/lib/selenium/webdriver/firefox/service.rb b/rb/lib/selenium/webdriver/firefox/service.rb index 1b54e084539eb..bd39a406eee22 100644 --- a/rb/lib/selenium/webdriver/firefox/service.rb +++ b/rb/lib/selenium/webdriver/firefox/service.rb @@ -33,25 +33,35 @@ def initialize(args: nil, **) args << '0' end - if ENV.key?('SE_DEBUG') - remove_log_args(args) - args << '-v' - end + configure_debug_args(args) if ENV.key?('SE_DEBUG') + + super + end + + def launch + configure_debug_args(args) if ENV.key?('SE_DEBUG') super end private - def remove_log_args(args) - if (index = args.index('--log')) - args.delete_at(index) # delete '--log' - args.delete_at(index) if args[index] && !args[index].start_with?('-') # delete value if present - warn_driver_log_override - elsif (index = args.index { |arg| arg.start_with?('--log=') }) - args.delete_at(index) - warn_driver_log_override + def configure_debug_args(args) + # An explicit geckodriver log setting is more specific than Selenium's + # generic SE_DEBUG fallback, so preserve it and skip adding `-v`. + if args.any? { |arg| arg.start_with?('--log') } + warn_explicit_log_preference unless @log_preference_warned + @log_preference_warned = true + args.reject! { |arg| /\A-v+\z/.match?(arg) } + return end + + args << '-v' unless args.any? { |arg| /\A-v+\z/.match?(arg) } + end + + def warn_explicit_log_preference + WebDriver.logger.warn('SE_DEBUG is set; preserving user-specified geckodriver --log setting instead of ' \ + 'adding -v', id: :se_debug) end end # Service end # Firefox diff --git a/rb/sig/lib/selenium/webdriver/firefox/service.rbs b/rb/sig/lib/selenium/webdriver/firefox/service.rbs index 609d70c6bc46c..9d3a7071d5927 100644 --- a/rb/sig/lib/selenium/webdriver/firefox/service.rbs +++ b/rb/sig/lib/selenium/webdriver/firefox/service.rbs @@ -13,7 +13,8 @@ module Selenium private - def remove_log_args: (Array[String] args) -> void + def configure_debug_args: (Array[String] args) -> void + def warn_explicit_log_preference: () -> void end end end diff --git a/rb/spec/integration/selenium/webdriver/bidi/script_spec.rb b/rb/spec/integration/selenium/webdriver/bidi/script_spec.rb index 578659befa1f5..eca2b848a428b 100644 --- a/rb/spec/integration/selenium/webdriver/bidi/script_spec.rb +++ b/rb/spec/integration/selenium/webdriver/bidi/script_spec.rb @@ -73,11 +73,10 @@ def a_stack_frame(**options) ) # Stack traces on console messages are optional. expect(log_entry.stack_trace).to be_nil.or match( - # Some browsers include stack traces from parts of the runtime, so we - # just check the first frames that come from user code. + # Some browsers include stack traces from parts of the runtime, and + # Firefox beta may only report the first user-code frame here. 'callFrames' => start_with( - a_stack_frame('functionName' => 'helloWorld'), - a_stack_frame('functionName' => 'onclick') + a_stack_frame('functionName' => 'helloWorld') ) ) end diff --git a/rb/spec/integration/selenium/webdriver/remote/element_spec.rb b/rb/spec/integration/selenium/webdriver/remote/element_spec.rb index 8944f413131aa..3316a16da2bc3 100644 --- a/rb/spec/integration/selenium/webdriver/remote/element_spec.rb +++ b/rb/spec/integration/selenium/webdriver/remote/element_spec.rb @@ -22,6 +22,16 @@ module Selenium module WebDriver describe Element, exclusive: {bidi: false, reason: 'Not yet implemented with BiDi'} do + def uploaded_body_text + driver.switch_to.frame('upload_target') + wait.until do + text = driver.find_element(tag_name: 'body').text + text unless text.empty? + rescue Error::StaleElementReferenceError + nil + end + end + before do driver.file_detector = lambda(&:first) end @@ -39,11 +49,7 @@ module WebDriver driver.find_element(id: 'go').click wait.until { driver.find_element(id: 'upload_label').displayed? } - driver.switch_to.frame('upload_target') - wait.until { !driver.find_element(xpath: '//body').text.empty? } - - body = driver.find_element(xpath: '//body') - expect(body.text.scan('This is a dummy test file').count).to eq(1) + expect(uploaded_body_text.scan('This is a dummy test file').count).to eq(1) end end @@ -60,11 +66,7 @@ module WebDriver driver.find_element(id: 'go').click wait.until { driver.find_element(id: 'upload_label').displayed? } - driver.switch_to.frame('upload_target') - wait.until { !driver.find_element(xpath: '//body').text.empty? } - - body = driver.find_element(xpath: '//body') - expect(body.text.scan('This is a dummy test file').count).to eq(2) + expect(uploaded_body_text.scan('This is a dummy test file').count).to eq(2) end end end diff --git a/rb/spec/tests.bzl b/rb/spec/tests.bzl index e875b9585f0a9..8544947c5e791 100644 --- a/rb/spec/tests.bzl +++ b/rb/spec/tests.bzl @@ -234,12 +234,13 @@ def rb_integration_test(name, srcs, deps = [], data = [], browsers = BROWSERS.ke target_compatible_with = BROWSERS[browser]["target_compatible_with"], ) -def rb_unit_test(name, srcs, deps, data = []): +def rb_unit_test(name, srcs, deps, data = [], flaky = False): rb_test( name = name, size = "small", srcs = srcs, args = ["rb/spec/"], + flaky = flaky, main = "@bundle//bin:rspec", data = data, tags = ["no-sandbox"], # TODO: Do we need this? diff --git a/rb/spec/unit/selenium/webdriver/BUILD.bazel b/rb/spec/unit/selenium/webdriver/BUILD.bazel index 2a76a3198091f..121a71ec8db4d 100644 --- a/rb/spec/unit/selenium/webdriver/BUILD.bazel +++ b/rb/spec/unit/selenium/webdriver/BUILD.bazel @@ -24,6 +24,13 @@ rb_unit_test( ], ) +rb_unit_test( + name = "zipper", + srcs = ["zipper_spec.rb"], + flaky = True, + deps = ["//rb/lib/selenium/webdriver:common"], +) + [ rb_unit_test( name = file[:-8], @@ -32,6 +39,9 @@ rb_unit_test( ) for file in glob( ["*_spec.rb"], - exclude = ["search_context_spec.rb"], + exclude = [ + "search_context_spec.rb", + "zipper_spec.rb", + ], ) ] diff --git a/rb/spec/unit/selenium/webdriver/firefox/service_spec.rb b/rb/spec/unit/selenium/webdriver/firefox/service_spec.rb index 66cfd3aece88d..345678370943b 100644 --- a/rb/spec/unit/selenium/webdriver/firefox/service_spec.rb +++ b/rb/spec/unit/selenium/webdriver/firefox/service_spec.rb @@ -26,9 +26,18 @@ module Firefox describe '#new' do let(:service_path) { "/path/to/#{Service::EXECUTABLE}" } + around do |example| + original_debug = ENV.fetch('SE_DEBUG', nil) + ENV.delete('SE_DEBUG') + example.run + ensure + original_debug ? ENV['SE_DEBUG'] = original_debug : ENV.delete('SE_DEBUG') + end + before do allow(Platform).to receive(:assert_executable) allow(WebDriver.logger).to receive(:debug?).and_return(false) + allow(WebDriver.logger).to receive(:warn) end it 'uses default port and nil path' do @@ -103,10 +112,11 @@ module Firefox context 'when SE_DEBUG is set' do around do |example| + original_debug = ENV.fetch('SE_DEBUG', nil) ENV['SE_DEBUG'] = '1' example.run ensure - ENV.delete('SE_DEBUG') + original_debug ? ENV['SE_DEBUG'] = original_debug : ENV.delete('SE_DEBUG') end it 'adds -v flag' do @@ -115,27 +125,51 @@ module Firefox expect(service.extra_args).to include('-v') end - it 'removes conflicting --log args with value' do + it 'preserves conflicting --log args with value and warns' do service = described_class.new(args: ['--log', 'info']) - expect(service.extra_args).to include('-v') - expect(service.extra_args).not_to include('--log') - expect(service.extra_args).not_to include('info') + expect(service.extra_args).not_to include('-v') + expect(service.extra_args).to include('--log') + expect(service.extra_args).to include('info') + expect(WebDriver.logger).to have_received(:warn).with( + 'SE_DEBUG is set; preserving user-specified geckodriver --log setting instead of adding -v', + id: :se_debug + ) end - it 'removes conflicting --log= args' do + it 'preserves conflicting --log= args and warns' do service = described_class.new(args: ['--log=info']) - expect(service.extra_args).to include('-v') - expect(service.extra_args).not_to include('--log=info') + expect(service.extra_args).not_to include('-v') + expect(service.extra_args).to include('--log=info') + expect(WebDriver.logger).to have_received(:warn).with( + 'SE_DEBUG is set; preserving user-specified geckodriver --log setting instead of adding -v', + id: :se_debug + ) end it 'does not remove next arg if --log has no value' do service = described_class.new(args: ['--log', '--other-flag']) - expect(service.extra_args).to include('-v') + expect(service.extra_args).not_to include('-v') + expect(service.extra_args).to include('--log') expect(service.extra_args).to include('--other-flag') end + + it 'preserves conflicting --log args added after initialization' do + service = described_class.new(path: service_path) + manager = instance_double(ServiceManager, start: nil) + service.args.push('--log', 'trace') + + allow(ServiceManager).to receive(:new).with(service).and_return(manager) + + service.launch + + expect(service.extra_args).not_to include('-v') + expect(service.extra_args).to include('--log') + expect(service.extra_args).to include('trace') + expect(WebDriver.logger).to have_received(:warn).once + end end end diff --git a/rb/spec/unit/selenium/webdriver/support/BUILD.bazel b/rb/spec/unit/selenium/webdriver/support/BUILD.bazel index e1d58c79a21cd..770888eaca946 100644 --- a/rb/spec/unit/selenium/webdriver/support/BUILD.bazel +++ b/rb/spec/unit/selenium/webdriver/support/BUILD.bazel @@ -2,6 +2,16 @@ load("//rb/spec:tests.bzl", "rb_unit_test") package(default_visibility = ["//rb:__subpackages__"]) +rb_unit_test( + name = "select", + srcs = ["select_spec.rb"], + flaky = True, + deps = [ + "//rb/lib/selenium/webdriver:common", + "//rb/lib/selenium/webdriver:remote", + ], +) + [ rb_unit_test( name = file[:-8], @@ -11,5 +21,8 @@ package(default_visibility = ["//rb:__subpackages__"]) "//rb/lib/selenium/webdriver:remote", ], ) - for file in glob(["*_spec.rb"]) + for file in glob( + ["*_spec.rb"], + exclude = ["select_spec.rb"], + ) ]