You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Replaces instances of platform.system() with sys.platform and appropriate conversions.
🔧 Implementation Notes
💡 Additional Considerations
How well will it work on other OSes except the top 3 (Linux, MacOS, Windows)?
🔄 Types of changes
Bug fix (backwards compatible)
PR Type
Bug fix
Description
Replace platform.system() with sys.platform across Python codebase
Add proper platform detection logic for Windows, Darwin, and Linux
Remove type ignore comments for Windows-specific subprocess attributes
Update test utilities to use consistent platform detection
Diagram Walkthrough
flowchart LR
A["platform.system()"] --> B["sys.platform"]
B --> C["Windows: win32"]
B --> D["macOS: darwin"]
B --> E["Linux: linux*"]
B --> F["Other: title case"]
Loading
File Walkthrough
Relevant files
Bug fix
conftest.py
Update platform detection in test configuration
py/conftest.py
Replace platform.system() import with sys import
Add platform detection logic in exe_platform property
Switching to sys.platform checks changes default for close_fds and STARTUPINFO usage. Verify subprocess behavior on Windows remains correct and that hiding console windows still works.
def_start_process(self, path: str) ->None:
"""Creates a subprocess by executing the command provided. :param cmd: full command to execute """cmd= [path]
cmd.extend(self.command_line_args())
close_file_descriptors=self.popen_kw.pop("close_fds", sys.platform!="win32")
try:
start_info=Noneifsys.platform=="win32":
start_info=subprocess.STARTUPINFO()
start_info.dwFlags=subprocess.CREATE_NEW_CONSOLE|subprocess.STARTF_USESHOWWINDOWstart_info.wShowWindow=subprocess.SW_HIDE
Class-level system now uses raw sys.platform (e.g., 'win32', 'linux', 'darwin' mapped to 'mac'). Confirm downstream consumers expect these exact values and not previous lowercased platform.system() outputs (e.g., 'windows', 'linux').
system=sys.platformifsystem=="darwin":
system="mac"# Class variables for headers
exe_platform now returns 'Windows'/'Darwin'/'Linux' else title-cased sys.platform. Ensure callers previously expecting platform.system() exact values still behave, and that linux prefix handling matches prior behavior.
Replacing platform.system() with sys.platform changes OS string semantics (e.g., "Windows" vs "win32", "FreeBSD" vs "freebsd13") and the PR now returns mixed forms across modules (exe_platform vs user agent vs FirefoxBinary.platform). This risks breaking equality checks and any downstream logic keyed on previous values, especially on Windows and non-main OSes. Introduce a single normalization helper (canonicalizing to "Windows"/"Linux"/"mac") and use it consistently everywhere to avoid subtle regressions.
# In py/conftest.pyclassDriver:
@propertydefexe_platform(self):
ifsys.platform=="win32":
return"Windows"elifsys.platform=="darwin":
return"Darwin"# ...# In py/selenium/webdriver/remote/remote_connection.pyclassRemoteConnection:
system=sys.platformifsystem=="darwin":
system="mac"user_agent=f"selenium/{__version__} (python {system})"# In py/selenium/webdriver/firefox/firefox_binary.pyclassFirefoxBinary:
def__init__(...):
self.platform=sys.platform
After:
# In a new shared utility file, e.g., selenium/webdriver/common/platform.pyimportsysdefget_canonical_platform() ->str:
ifsys.platform=="win32":
return"windows"ifsys.platform=="darwin":
return"mac"ifsys.platform.startswith("linux"):
return"linux"returnsys.platform# In py/conftest.py# ...classDriver:
@propertydefexe_platform(self):
# Logic to convert canonical name to required format# e.g. get_canonical_platform().title()# In py/selenium/webdriver/remote/remote_connection.py & firefox_binary.py# ...self.platform=get_canonical_platform()
Suggestion importance[1-10]: 8
__
Why: The suggestion correctly identifies that replacing platform.system() with sys.platform has been done inconsistently across modules, creating a risk of platform-specific bugs.
Medium
General
Use explicit fallback value
The fallback case sys.platform.title() may not provide consistent platform names for edge cases. Consider using a more explicit mapping or returning a standardized default value to ensure predictable behavior across all platforms.
Why: The suggestion correctly points out that sys.platform.title() can lead to unpredictable platform names, and replacing it with a constant like "Unknown" improves the robustness and predictability of the exe_platform property for unhandled operating systems.
exe_platform previously returned values like 'Windows'/'Linux'/'Darwin' via platform.system(); now it derives from sys.platform and title-cases unknowns. Downstream callers may rely on exact strings. Verify all consumers accept 'Darwin' and that non-top-3 platforms behave correctly.
self.platform changed from lowercased system name to raw sys.platform (e.g., 'win32', 'linux', 'darwin'). Confirm no code/tests elsewhere expect lowercase plain OS names and that behavior remains consistent.
STARTUPINFO flags now set without type-ignores and gated by sys.platform == 'win32'. Validate behavior on Cygwin/MSYS and ensure new console/window hiding still works; confirm close_fds default change matches previous logic.
Switching from platform.system().lower() to sys.platform without normalization breaks OS checks (e.g., FirefoxBinary now sees "win32" instead of "windows"), likely disabling Windows-specific logic. Add a single normalization helper (mapping win32/cygwin/msys→windows, darwin→darwin/mac as needed, linux*→linux) and use it consistently across the codebase to avoid regressions on Windows and less common platforms.
# In conftest.pyclassDriver:
@propertydefexe_platform(self):
ifsys.platform=="win32":
return"Windows"elifsys.platform=="darwin":
return"Darwin"# ... and so on# In selenium/webdriver/firefox/firefox_binary.pyclassFirefoxBinary:
def__init__(self, ...):
self.platform=sys.platform# This will be "win32" on Windowsdef_get_firefox_start_cmd(self):
...
elifself.platform=="windows": # This check now fails on Windows# ... windows-specific logic
After:
# In a new utility file, e.g., selenium/webdriver/common/platform.pydefget_platform_name():
ifsys.platform.startswith("win"):
return"windows"ifsys.platform=="darwin":
return"darwin"# ... and so on# In conftest.pyfromselenium.webdriver.common.platformimportget_platform_nameclassDriver:
@propertydefexe_platform(self):
returnget_platform_name().title()
# In selenium/webdriver/firefox/firefox_binary.pyfromselenium.webdriver.common.platformimportget_platform_nameclassFirefoxBinary:
def__init__(self, ...):
self.platform=get_platform_name() # This will be "windows"def_get_firefox_start_cmd(self):
...
elifself.platform=="windows": # This check now works# ... windows-specific logic
Suggestion importance[1-10]: 9
__
Why: The suggestion correctly identifies a critical regression in FirefoxBinary where Windows-specific logic is broken, and proposes a robust, centralized normalization approach to fix it and prevent future inconsistencies.
High
Possible issue
Normalize platform string values
Preserve the previous normalized values for the platform string to avoid breaking downstream logic that may check for "windows"/"linux"/"darwin". Map sys.platform to these canonical lowercase names. This prevents regressions where win32 would no longer match prior "windows" checks.
-self.platform = sys.platform+p = sys.platform+if p == "win32":+ self.platform = "windows"+elif p == "darwin":+ self.platform = "darwin"+elif p.startswith("linux"):+ self.platform = "linux"+else:+ self.platform = p
Apply / Chat
Suggestion importance[1-10]: 7
__
Why: The suggestion correctly identifies that changing platform.system().lower() to sys.platform alters the platform string for Windows from "windows" to "win32", which could break downstream logic. It proposes a fix to maintain backward compatibility.
Medium
General
Normalize Windows platform naming
Normalize Windows to "windows" to keep backward compatibility in identifiers like the user agent string. This avoids unintended changes from "windows" to "win32" that may affect consumer parsing or comparisons.
system = sys.platform
if system == "darwin":
system = "mac"
+elif system == "win32":+ system = "windows"+elif system.startswith("linux"):+ system = "linux"
Apply / Chat
Suggestion importance[1-10]: 6
__
Why: This suggestion correctly points out that the change from platform.system().lower() to sys.platform will alter the user-agent string on Windows. Normalizing "win32" back to "windows" is a good practice for maintaining backward compatibility.
Also, Cygwin is an annoying corner case (does anyone even use Cygwin anymore?). sys.platform returns cygwin even though the underlying platform is Windows... so I think we need to change windows detection to if sys.platform in ("win32", "cygwin"): ... unless we just don't care about Cygwin these days.
Looks like current code should work well for cygwin since /dev/null is supported in cygwin too. But I am not sure if STARTUPINFO, CREATE_NEW_CONSOLE and others are available in cygwin.
Looks like current code should work well for cygwin since /dev/null is supported in cygwin too. But I am not sure if STARTUPINFO, CREATE_NEW_CONSOLE and others are available in cygwin.
I just installed cygwin to test this. It supports /dev/null and does not support the Windows subprocess attributes (STARTUPINFO, CREATE_NEW_CONSOLE, STARTF_USESHOWWINDOW, SW_HIDE), so it should follow the non-Windows behavior. I think you have everything correct in this PR as-is.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
🔗 Related Issues
Fixes #16256
💥 What does this PR do?
Replaces instances of
platform.system()withsys.platformand appropriate conversions.🔧 Implementation Notes
💡 Additional Considerations
How well will it work on other OSes except the top 3 (Linux, MacOS, Windows)?
🔄 Types of changes
PR Type
Bug fix
Description
Replace
platform.system()withsys.platformacross Python codebaseAdd proper platform detection logic for Windows, Darwin, and Linux
Remove type ignore comments for Windows-specific subprocess attributes
Update test utilities to use consistent platform detection
Diagram Walkthrough
File Walkthrough
conftest.py
Update platform detection in test configurationpy/conftest.py
platform.system()import withsysimportexe_platformpropertyservice.py
Modernize platform detection in service modulepy/selenium/webdriver/common/service.py
platform.systemimport withsysimportfirefox_binary.py
Update Firefox binary platform detectionpy/selenium/webdriver/firefox/firefox_binary.py
platform.systemimport withsysimportsys.platformdirectlyremote_connection.py
Modernize platform detection in remote connectionpy/selenium/webdriver/remote/remote_connection.py
platformimport withsysimportsys.platformfirefox_sizing_tests.py
Update Linux detection in Firefox sizing testspy/test/selenium/webdriver/firefox/firefox_sizing_tests.py
platformimport withsysimportsys.platform.startswith("linux")chrome_options_tests.py
Update platform detection in Chrome options testspy/test/unit/selenium/webdriver/chrome/chrome_options_tests.py
platformimport withsysimport