Skip to content

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

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
Open
wants to merge 3 commits into
base: trunk
Choose a base branch
from
Open

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

wants to merge 3 commits into from
+24 −17

Conversation

navin772
Copy link
Member

@navin772 navin772 commented Aug 26, 2025 •
edited by qodo-merge-pro bot
Loading

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

@selenium-ci selenium-ci added the C-py Python Bindings label Aug 26, 2025
@navin772 navin772 marked this pull request as draft August 26, 2025 06:28
@qodo-merge-pro Qodo Merge Pro
Copy link
Contributor

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 Qodo Merge Pro
Copy link
Contributor

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

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    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

@navin772 navin772 marked this pull request as ready for review August 26, 2025 07:34
@qodo-merge-pro Qodo Merge Pro
Copy link
Contributor

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 Qodo Merge Pro
Copy link
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    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
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
C-py Python Bindings Review effort 2/5
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[🐛 Bug]: [py] Platform detection code doesn't work well with type checker
4 participants
@navin772 @cgoldberg @shbenzer @selenium-ci