[rb] Modern Firefox does not like both the -v and --log flags at the same time

[shs96c]

No description provided.

[qodo-code-review]

Review Summary by Qodo

Fix Firefox geckodriver -v and --log flag conflict in SE_DEBUG mode

🐞 Bug fix

Walkthroughs

Description

• Fix Firefox geckodriver conflict between -v and --log flags
• Preserve explicit logging arguments when SE_DEBUG is set
• Apply debug configuration during both initialization and launch
• Update test expectations and add launch-time configuration test

File Changes

1. rb/lib/selenium/webdriver/firefox/service.rb 🐞 Bug fix +13/-12

Invert debug flag logic to preserve explicit logging

• Renamed remove_log_args to configure_debug_args with inverted logic
• Changed behavior to preserve --log arguments instead of removing them
• Removes -v flags when explicit logging is present, adds -v only when no logging is configured
• Added launch method to reapply debug configuration at launch time

rb/lib/selenium/webdriver/firefox/service.rb

---

2. rb/spec/unit/selenium/webdriver/firefox/service_spec.rb 🧪 Tests +32/-8

Update tests for inverted debug flag behavior

• Added around hook to isolate SE_DEBUG environment variable from ambient state
• Updated test expectations to reflect new behavior (preserve --log, remove -v)
• Fixed existing SE_DEBUG context to properly restore original environment state
• Added new test for launch-time configuration with dynamically added logging arguments

rb/spec/unit/selenium/webdriver/firefox/service_spec.rb

---

3. rb/sig/lib/selenium/webdriver/firefox/service.rbs 📝 Documentation +1/-1

Sync type signature with method rename

• Updated method signature from remove_log_args to configure_debug_args

rb/sig/lib/selenium/webdriver/firefox/service.rbs

---

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (1)

1. instance_double(ServiceManager) added 📘 Rule violation ⚙ Maintainability

Description

The new test introduces an RSpec mock (instance_double and stubbing ServiceManager.new), which
can diverge from the real ServiceManager contract and reduce test fidelity. This conflicts with
the guideline to prefer unit/small tests without mocks where feasible.

Code

rb/spec/unit/selenium/webdriver/firefox/service_spec.rb[R150-156]

+            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)
+

Evidence

PR Compliance ID 6 discourages introducing mocks in tests; the added spec stubs ServiceManager.new
and uses instance_double(ServiceManager, ...) to avoid exercising real behavior.

AGENTS.md
rb/spec/unit/selenium/webdriver/firefox/service_spec.rb[150-156]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A new unit test stubs `ServiceManager.new` and uses `instance_double(ServiceManager, ...)`, introducing a mock that may not match the real `ServiceManager` behavior/contract.

## Issue Context
The test’s intent is to verify `SE_DEBUG` argument adjustment when `--log` is added after initialization; this can likely be validated without mocking process-launch components.

## Fix Focus Areas
- rb/spec/unit/selenium/webdriver/firefox/service_spec.rb[150-162]
- rb/lib/selenium/webdriver/firefox/service.rb[41-56]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Misleading spec descriptions 🐞 Bug ⚙ Maintainability

Description

Two examples in the Firefox service spec still say they “remove conflicting --log” even though the
expectations now assert --log is preserved and -v is removed. This mismatch makes the tests
misleading and increases the chance of future incorrect edits.

Code

rb/spec/unit/selenium/webdriver/firefox/service_spec.rb[R127-140]

            it 'removes conflicting --log args with value' 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')
            end

-            it 'removes conflicting --log= args' do
+            it 'preserves conflicting --log= args' 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')
            end

Evidence

The implementation now preserves --log* and removes -v when SE_DEBUG is set, but one spec
title still states it “removes” --log while the assertions confirm the opposite behavior.

rb/spec/unit/selenium/webdriver/firefox/service_spec.rb[127-140]
rb/lib/selenium/webdriver/firefox/service.rb[49-56]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The Firefox service unit spec example descriptions are out of sync with the new behavior: they still claim `--log` is removed even though expectations verify `--log` is preserved and `-v` is removed.

### Issue Context
The implementation (`configure_debug_args`) now avoids the `-v` + `--log*` combination by dropping `-v` whenever `--log*` is present.

### Fix Focus Areas
- rb/spec/unit/selenium/webdriver/firefox/service_spec.rb[127-140]

### Suggested change
Rename the example(s) to reflect the current behavior (e.g., change “removes conflicting --log …” to “preserves --log … and removes -v”).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[aguspe]

so SE_DEBUG does not overrides the user's --log and the warning's gone, is that what we want? and why?

[shs96c]

Good point. I've amended the PR.