overload log_path to accept tuple (path, mode)

[vringar]

Description

This change allows users to specify how they want to open their log file with a fallback to a+ if nothing else is specified

Motivation and Context

In OpenWPM we run multiple Firefox instances in parallel. To integrate their output with our logging infrastructure we want them to write their logs into pipes, which we then read on the other side, enrich it with metadata and log it ourselves.

This feature is relevant to anyone who wants to interleave their own code's log lines with Selenium's or the browser's.

Fixes #10703

Types of changes

• [X] Bug fix (non-breaking change which fixes an issue)
• [X] New feature (non-breaking change which adds functionality)
• [ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• [X] I have read the contributing document.
• [X] My change requires a change to the documentation.
• [X] I have updated the documentation accordingly.
• [X] I have added tests to cover my changes.
• [ ] All new and existing tests passed.

[CLAassistant]

All committers have signed the CLA.

[vringar]

This branch is still lacking test cases, which I'll be adding once there is interest to merge. If https://github.com/SeleniumHQ/selenium/issues/10703 gets closed as invalid/not relevant I don't want to have invested too much effort.

[vringar]

I think this PR is ready for another round of review. The test I've added is pretty basic but idk if you wanted more @symonk.

[symonk]

Apologies for the delays! I've approved the PR for running actions workflows. Thanks a bunch for the pull request and taking the effort to contribute, it is greatly appreciated! Few comments around tests

[codecov-commenter]

Codecov Report

Merging #10704 (945c4d8) into trunk (ae46fd8) will decrease coverage by 0.02%. The diff coverage is 50.00%.

@@            Coverage Diff             @@
##            trunk   #10704      +/-   ##
==========================================
- Coverage   50.45%   50.42%   -0.03%     
==========================================
  Files          84       84              
  Lines        5480     5483       +3     
  Branches      278      281       +3     
==========================================
  Hits         2765     2765              
  Misses       2437     2437              
- Partials      278      281       +3     

Impacted Files	Coverage Δ
py/selenium/webdriver/firefox/service.py	63.63% <50.00%> (-10.05%)	:arrow_down:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update ae46fd8...945c4d8. Read the comment docs.

[vringar]

No worries about the delays! Unfortunately your permission to run the tests only seems to apply to a single commit, so I've activated the Python Actions for my fork, to make sure the tests are passing.

[vringar]

Okay, the chrome tests are failing (in the acitons of my fork) but as I didn't modify any code on that path, I don't think that should block merging here.

[vringar]

Just bumping to make sure it's not lost.

[vringar]

Hey, is there something I can do to move this forward?

[vringar]

@symonk Sorry to ping you on this but can you run the tests again please

[titusfortner]

It doesn't make sense to me for a parameter named log_path to take a dictionary that takes 3 optional arguments. Does Python Selenium manage logging this way elsewhere in the code or is it only geckodriver logs? Would it make sense to create a new parameter/class? (that's what Java would do, I'm not actually sure what the Python idiomatic approach would be)

I'm also thinking along the lines of making all logging more easy and have one class to manage all the sources and outputs of logs. #10944

[vringar]

Does Python Selenium manage logging this way elsewhere in the code or is it only geckodriver logs?

I found the webkitgtk/service.py and wpewebkit/service.py have a similar pattern, but they use binary logs and always use wb open mode, so they are already different from the Firefox service. Chromium browsers just pass the log location as an Command line argument.

Would it make sense to create a new parameter/class? (that's what Java would do, I'm not actually sure what the Python idiomatic approach would be)

AFAIK, having a parameter accept multiple types is the usual way to do overloading, as there is no overload resolution in Python and always only a single constructor. I agree the name is unfortunate, but changing it would be backwards incompatible.

It doesn't make sense to me for a parameter named log_path to take a dictionary that takes 3 optional arguments.

As a technicality, the dict can have up to 8 values since that is how many the open function accepts. I could also break out the encoding and mode as separate optional params, which is what I initially did until @symonk guided me to the overload approach.