Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use which() to get Firefox path on Mac OS #10818

Merged
merged 3 commits into from
Jun 29, 2022
Merged

Use which() to get Firefox path on Mac OS #10818

merged 3 commits into from
Jun 29, 2022

Conversation

bogdancondurache
Copy link
Contributor

Fixes #10686

Description

The Firefox binary under Mac OS was using a hardcoded path. This was
failing when trying to manipulate PATH env var to use Firefox from a
different location or if Firefox was installed in a different place.

Types of changes

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

Checklist

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

@CLAassistant
Copy link

CLAassistant commented Jun 27, 2022 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@bogdancondurache bogdancondurache mentioned this pull request Jun 27, 2022
Closed
symonk
symonk previously approved these changes Jun 27, 2022
View reviewed changes
@symonk symonk self-requested a review June 27, 2022 20:34
@symonk symonk dismissed their stale review June 27, 2022 20:36

potential issue spotted after

symonk
symonk requested changes Jun 27, 2022
View reviewed changes
Copy link
Member

@symonk symonk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

referencing a local variable that doesn't exist

Use which() to get Firefox path on Mac OS
890ef3f 
The Firefox binary under Mac OS was using a hardcoded path. This was
failing when trying to manipulate PATH env var to use firefox from a
different location or if Firefox was installed in a different place.

Fixes SeleniumHQ#10686
@bogdancondurache bogdancondurache force-pushed the fix/use-which-for-firefox-path-macos branch from 21b70d8 to 890ef3f Compare June 27, 2022 21:14
@bogdancondurache bogdancondurache requested a review from symonk June 27, 2022 21:16
@codecov-commenter
Copy link

codecov-commenter commented Jun 27, 2022 •
edited
Loading

Codecov Report

Merging #10818 (5e0b56b) into trunk (334a103) will decrease coverage by 0.00%.
The diff coverage is 25.00%.

❗ Current head 5e0b56b differs from pull request most recent head f0465d3. Consider uploading reports for the commit f0465d3 to get more accurate results

@@            Coverage Diff             @@
##            trunk   #10818      +/-   ##
==========================================
- Coverage   50.46%   50.45%   -0.01%     
==========================================
  Files          84       84              
  Lines        5477     5480       +3     
  Branches      278      278              
==========================================
+ Hits         2764     2765       +1     
- Misses       2435     2437       +2     
  Partials      278      278
Impacted Files Coverage Δ
py/selenium/webdriver/firefox/firefox_binary.py 32.59% <25.00%> (+0.01%) ⬆️

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 334a103...f0465d3. Read the comment docs.

@diemol diemol requested a review from symonk June 28, 2022 14:00
diemol
diemol approved these changes Jun 29, 2022
View reviewed changes
Copy link
Member

@diemol diemol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @bogdancondurache!

@diemol diemol merged commit de0a144 into SeleniumHQ:trunk Jun 29, 2022
@bogdancondurache bogdancondurache deleted the fix/use-which-for-firefox-path-macos branch July 18, 2022 08:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@diemol diemol diemol approved these changes

@symonk symonk symonk approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[🐛 Bug]: Contained Firefox can't be used on MacOS
5 participants
@bogdancondurache @CLAassistant @codecov-commenter @diemol @symonk