fix: replace FirefoxBinary with Selenium 4 equivalents (fixes #222)

[FernandoCelmer]

Summary

Fixes #222 — tbselenium was completely unusable with any Selenium version because:

• selenium>=4 is declared as a requirement, so Selenium 3 is rejected by package managers
• But the code imported FirefoxBinary from selenium.webdriver.firefox.firefox_binary, which was removed in Selenium 4

This PR eliminates the FirefoxBinary dependency and migrates all affected code to Selenium 4 APIs.

Changes

• tbselenium/tbbinary.py: Remove the TBBinary(FirefoxBinary) subclass. Replace it with a standalone TBBinary class that retains the kill() process-management method without any dependency on the removed Selenium 3 module.
• tbselenium/tbdriver.py:
  • Replace self.options.binary = ... (Selenium 3, accepted a FirefoxBinary object) with self.options.binary_location = ... (Selenium 4 API, accepts a plain string path).
  • Replace deprecated Service(executable_path=..., log_path=...) kwargs with Service(executable=..., log_output=...) as required by Selenium 4.10+.
  • Update the get_tb_binary() docstring to drop the stale FirefoxBinary reference.
• tbselenium/test/test_browser.py: Update binary-path assertions to use options.binary_location (plain string) instead of the removed FirefoxBinary.which() helper.

Test plan

• python -c "from tbselenium.tbdriver import TorBrowserDriver" no longer raises ModuleNotFoundError
• python -c "from tbselenium.tbbinary import TBBinary" imports cleanly
• Existing test suite passes against a Selenium 4.x install
• test_correct_firefox_binary and test_should_load_tbb_firefox_libs pass with the updated binary-path assertions

🤖 Generated with Claude Code

[FernandoCelmer]

🔍 Code Review

Code issues found: 3

See inline comments below.

[FernandoCelmer]

[Suggestion]

Problem — import subprocess is added at the top of the file but is never used anywhere in the module. The new TBBinary class has no launch_browser() or similar method that calls subprocess.Popen. This dead import will trigger flake8 F401 lint errors and misleads readers into thinking TBBinary is responsible for spawning the browser process.

Failure scenario — Any CI pipeline running flake8 will flag this and fail. A developer reading the file will naturally look for where subprocess is used and waste time concluding it was an incomplete implementation.

Fix — Remove import subprocess. If a launch_browser() method is added in a follow-up, the import can be restored then.

[FernandoCelmer]

[Blocking]

Problem — kill() will always be a no-op because self.process is initialized to None in __init__ and is never assigned anywhere in the codebase. The original FirefoxBinary base class populated self.process via its own launch_browser() mechanism, which Selenium 3 invoked internally when starting the browser. That hook no longer exists in Selenium 4, and the new standalone TBBinary class has no equivalent — no launch_browser(), no start(), nothing that sets self.process to a live subprocess.Popen handle.

The presence of import subprocess at the top of this file suggests a launch_browser() method was intended but never implemented, leaving kill() as permanently unreachable logic.

Failure scenario — A user calls driver.get_tb_binary().kill() expecting it to terminate a stuck browser process. Instead, the guard if self.process and self.process.poll() is None evaluates to False immediately (since self.process is None) and the method returns silently. The browser is never killed.

Fix — Either implement a launch_browser() / start() method using subprocess.Popen that sets self.process, or explicitly document that the caller is responsible for assigning self.process before calling kill(). If there is no valid use case for this class in the current codebase, consider removing TBBinary and get_tb_binary() entirely to avoid a public API that silently does nothing.

[FernandoCelmer]

[Suggestion]

Problem — self.log_file = log_file is stored in __init__ but is never read by any method in the class. The original FirefoxBinary used the log_file handle to redirect browser stdout/stderr to a file when launching the process. Since TBBinary has no process-launching code, the file handle is silently discarded.

Failure scenario — A caller uses driver.get_tb_binary(logfile='/var/log/tb.log') expecting to capture browser output for debugging. No output is written to the file, no error is raised, and the open file handle is leaked (never closed).

Fix — If log redirection is out of scope for this PR, remove the log_file parameter from __init__ and update get_tb_binary() in tbdriver.py accordingly. Alternatively, add a comment like # log_file is not yet used; process launch not implemented so callers are not misled by the parameter's presence.