[py]: replace `platform.system()` with `sys.platform` #16259

navin772 · 2025-08-26T06:27:59Z

User description

🔗 Related Issues

Fixes #16256

💥 What does this PR do?

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"]

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 Update Linux detection in server fixture	+10/-3
service.py `Modernize platform detection in service module` py/selenium/webdriver/common/service.py Replace `platform.system` import with `sys` import Update Windows detection from string comparison to platform check Remove type ignore comments for subprocess attributes	+6/-6
firefox_binary.py `Update Firefox binary platform detection` py/selenium/webdriver/firefox/firefox_binary.py Replace `platform.system` import with `sys` import Update platform assignment to use `sys.platform` directly	+2/-2
remote_connection.py `Modernize platform detection in remote connection` py/selenium/webdriver/remote/remote_connection.py Replace `platform` import with `sys` import Update system detection to use `sys.platform`	+2/-2

Tests

firefox_sizing_tests.py `Update Linux detection in Firefox sizing tests` py/test/selenium/webdriver/firefox/firefox_sizing_tests.py Replace `platform` import with `sys` import Update Wayland detection to use `sys.platform.startswith("linux")`	+2/-2
chrome_options_tests.py `Update platform detection in Chrome options tests` py/test/unit/selenium/webdriver/chrome/chrome_options_tests.py Replace `platform` import with `sys` import Update Windows detection in test utility function	+2/-2

qodo-merge-pro · 2025-08-26T06:28:44Z

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 1234 - Partially compliant Compliant requirements: None Non-compliant requirements: Firefox on click() should trigger JavaScript in link href (regression from 2.47.1 to 2.48.x) Reproduce and validate behavior difference on Firefox 42 Ensure compatibility and restore expected alert behavior Requires further human verification: End-to-end validation in Firefox that click() triggers href JavaScript 5678 - Partially compliant Compliant requirements: None Non-compliant requirements: Investigate "ConnectFailure (Connection refused)" when instantiating multiple ChromeDriver instances Validate behavior on Ubuntu 16.04.4 with Chrome 65 / ChromeDriver 2.35 / Selenium 3.9.0 Provide fix or guidance to avoid connection failures on subsequent instances Requires further human verification: Reproduction on the specified environment and versions
⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review Windows Behavior 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 = None if sys.platform == "win32": start_info = subprocess.STARTUPINFO() start_info.dwFlags = subprocess.CREATE_NEW_CONSOLE \| subprocess.STARTF_USESHOWWINDOW start_info.wShowWindow = subprocess.SW_HIDE Platform String 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.platform if system == "darwin": system = "mac" # Class variables for headers Platform Mapping 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. if sys.platform == "win32": return "Windows" elif sys.platform == "darwin": return "Darwin" elif sys.platform.startswith("linux"): return "Linux" else: return sys.platform.title()

qodo-merge-pro · 2025-08-26T06:31:20Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	Standardize platform normalization globally 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. Examples: py/conftest.py [184-192] def exe_platform(self): if sys.platform == "win32": return "Windows" elif sys.platform == "darwin": return "Darwin" elif sys.platform.startswith("linux"): return "Linux" else: return sys.platform.title() py/selenium/webdriver/remote/remote_connection.py [159-161] system = sys.platform if system == "darwin": system = "mac" Solution Walkthrough: Before: # In py/conftest.py class Driver: @property def exe_platform(self): if sys.platform == "win32": return "Windows" elif sys.platform == "darwin": return "Darwin" # ... # In py/selenium/webdriver/remote/remote_connection.py class RemoteConnection: system = sys.platform if system == "darwin": system = "mac" user_agent = f"selenium/{__version__} (python {system})" # In py/selenium/webdriver/firefox/firefox_binary.py class FirefoxBinary: def __init__(...): self.platform = sys.platform After: # In a new shared utility file, e.g., selenium/webdriver/common/platform.py import sys def get_canonical_platform() -> str: if sys.platform == "win32": return "windows" if sys.platform == "darwin": return "mac" if sys.platform.startswith("linux"): return "linux" return sys.platform # In py/conftest.py # ... class Driver: @property def exe_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. py/conftest.py [183-192] @property def exe_platform(self): if sys.platform == "win32": return "Windows" elif sys.platform == "darwin": return "Darwin" elif sys.platform.startswith("linux"): return "Linux" else: - return sys.platform.title() + return "Unknown" Apply / Chat Suggestion importance[1-10]: 6 __ 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.	Low
Update

qodo-merge-pro · 2025-08-26T07:34:56Z

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 5678 - Partially compliant Compliant requirements: None Non-compliant requirements: Investigate/connectivity error "ConnectFailure (Connection refused)" when instantiating multiple ChromeDriver instances on Ubuntu/Linux. Determine if issue is OS- or driver-specific (Chrome/ChromeDriver). Provide a fix or workaround to prevent repeated console errors on subsequent driver instantiations. Requires further human verification: N/A 1234 - Partially compliant Compliant requirements: None Non-compliant requirements: Firefox: Ensure click() triggers javascript in link's href (regression between 2.47.1 and 2.48.x). Provide reproducible fix and tests validating alert behavior. Requires further human verification: N/A
⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review Behavior Change 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. def exe_platform(self): if sys.platform == "win32": return "Windows" elif sys.platform == "darwin": return "Darwin" elif sys.platform.startswith("linux"): return "Linux" else: return sys.platform.title() API Contract 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. self.platform = sys.platform if not self._start_cmd: Windows Startup 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. close_file_descriptors = self.popen_kw.pop("close_fds", sys.platform != "win32") try: start_info = None if sys.platform == "win32": start_info = subprocess.STARTUPINFO() start_info.dwFlags = subprocess.CREATE_NEW_CONSOLE \| subprocess.STARTF_USESHOWWINDOW start_info.wShowWindow = subprocess.SW_HIDE

qodo-merge-pro · 2025-08-26T07:37:18Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	Normalize platform name usage 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. Examples: py/selenium/webdriver/firefox/firefox_binary.py [48] self.platform = sys.platform py/conftest.py [185-192] if sys.platform == "win32": return "Windows" elif sys.platform == "darwin": return "Darwin" elif sys.platform.startswith("linux"): return "Linux" else: return sys.platform.title() Solution Walkthrough: Before: # In conftest.py class Driver: @property def exe_platform(self): if sys.platform == "win32": return "Windows" elif sys.platform == "darwin": return "Darwin" # ... and so on # In selenium/webdriver/firefox/firefox_binary.py class FirefoxBinary: def __init__(self, ...): self.platform = sys.platform # This will be "win32" on Windows def _get_firefox_start_cmd(self): ... elif self.platform == "windows": # This check now fails on Windows # ... windows-specific logic After: # In a new utility file, e.g., selenium/webdriver/common/platform.py def get_platform_name(): if sys.platform.startswith("win"): return "windows" if sys.platform == "darwin": return "darwin" # ... and so on # In conftest.py from selenium.webdriver.common.platform import get_platform_name class Driver: @property def exe_platform(self): return get_platform_name().title() # In selenium/webdriver/firefox/firefox_binary.py from selenium.webdriver.common.platform import get_platform_name class FirefoxBinary: def __init__(self, ...): self.platform = get_platform_name() # This will be "windows" def _get_firefox_start_cmd(self): ... elif self.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. py/selenium/webdriver/firefox/firefox_binary.py [48] -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. py/selenium/webdriver/remote/remote_connection.py [159-161] 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.	Low
More

cgoldberg · 2025-08-26T12:51:56Z

According to the docs: https://docs.python.org/3/library/sys.html#sys.platform
Since Python 3.3, it will always return "linux" , so I think it's safe to do sys.platform == "linux" instead of sys.platform.startswith("linux").

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.

navin772 · 2025-08-26T13:10:45Z

Since Python 3.3, it will always return "linux" , so I think it's safe to do sys.platform == "linux" instead of sys.platform.startswith("linux").

Sure, I will update it.

unless we just don't care about Cygwin these days

I would say a lot of people still use it, so its good to support it.

navin772 · 2025-08-26T13:46:40Z

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.

shbenzer · 2025-08-26T15:09:06Z

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.

We can only do so much for OS outside the big 3

cgoldberg · 2025-08-28T19:26:08Z

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.

navin772 · 2025-08-29T06:29:49Z

Thanks a lot @cgoldberg for verifying.

navin772 added 2 commits August 26, 2025 11:53

replace platform.system() with sys.platform

d703303

remove type ignore

2356a26

selenium-ci added the C-py Python Bindings label Aug 26, 2025

navin772 marked this pull request as draft August 26, 2025 06:28

qodo-merge-pro bot added the Review effort 2/5 label Aug 26, 2025

navin772 marked this pull request as ready for review August 26, 2025 07:34

navin772 added 2 commits August 26, 2025 22:39

remove startswith

73d50a9

Merge branch 'trunk' into switch-to-sys-platform

2da2618

navin772 requested review from cgoldberg and shbenzer August 28, 2025 07:14

cgoldberg approved these changes Aug 28, 2025

View reviewed changes

Merge branch 'trunk' into switch-to-sys-platform

8ff0a70

navin772 merged commit c585e6a into SeleniumHQ:trunk Aug 29, 2025
16 checks passed

navin772 deleted the switch-to-sys-platform branch August 29, 2025 07:26

Uh oh!

[py]: replace platform.system() with sys.platform #16259

[py]: replace platform.system() with sys.platform #16259

Uh oh!

Conversation

navin772 commented Aug 26, 2025 • edited by qodo-merge-pro bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User description

🔗 Related Issues

💥 What does this PR do?

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

PR Type

Description

Diagram Walkthrough

File Walkthrough

Uh oh!

qodo-merge-pro bot commented Aug 26, 2025

PR Reviewer Guide 🔍

Uh oh!

qodo-merge-pro bot commented Aug 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Examples:

Solution Walkthrough:

Before:

After:

Uh oh!

qodo-merge-pro bot commented Aug 26, 2025

PR Reviewer Guide 🔍

Uh oh!

qodo-merge-pro bot commented Aug 26, 2025

PR Code Suggestions ✨

Examples:

Solution Walkthrough:

Before:

After:

Uh oh!

cgoldberg commented Aug 26, 2025

Uh oh!

navin772 commented Aug 26, 2025

Uh oh!

navin772 commented Aug 26, 2025

Uh oh!

shbenzer commented Aug 26, 2025

Uh oh!

cgoldberg commented Aug 28, 2025

Uh oh!

navin772 commented Aug 29, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

[py]: replace `platform.system()` with `sys.platform` #16259

[py]: replace `platform.system()` with `sys.platform` #16259

navin772 commented Aug 26, 2025 •

edited by qodo-merge-pro bot

Loading

qodo-merge-pro bot commented Aug 26, 2025 •

edited

Loading