Skip to content

Conversation

@cgoldberg
Copy link
Member

@cgoldberg cgoldberg commented Nov 13, 2025

User description

Description

This PR fixes some Python test failures. Several tests were failing with PermissionError. The reason for this is that we run tests in parallel using the pytest-xdist plugin. The logging tests all use a common fixture (log_path). This fixture generates a log file name and deletes the file when each test completes. The problem is that when running tests in parallel, multiple tests were using the same log file name and attempting to delete the file while other tests were still using it. I updated the fixture to use a UUID in the file name instead of a timestamp to make sure log file names are unique.

I also added a few things to .gitignore and fixed the import order in .conftest.py.

Motivation and Context

Tests were failing when run in parallel.

Types of changes

  • Fixed test failures

Checklist

  • I have read the contributing document.
  • I have used hugo to render the site/docs locally and I am sure it works.

PR Type

Bug fix, Tests


Description

  • Fixed parallel test failures by using UUID for log file names

  • Replaced timestamp-based naming with UUID to ensure unique log files

  • Corrected import order in conftest.py for PEP 8 compliance

  • Added color output flag to pytest command in tox.ini


Diagram Walkthrough

flowchart LR
  A["Timestamp-based log names"] -->|"Replace with UUID"| B["Unique log file names"]
  B -->|"Prevents conflicts"| C["Parallel tests succeed"]
  D["Import order issues"] -->|"Reorganize imports"| E["PEP 8 compliance"]
Loading

File Walkthrough

Relevant files
Bug fix
conftest.py
Use UUID for unique log file naming                                           

examples/python/tests/conftest.py

  • Replaced datetime-based log file naming with UUID-based naming to
    ensure uniqueness
  • Added import uuid statement
  • Reorganized imports to follow PEP 8 standards (stdlib, third-party,
    local)
  • Changed log_path fixture to use f-string with uuid.uuid4() instead of
    timestamp
+5/-5     
Enhancement
tox.ini
Add color output to pytest command                                             

examples/python/tox.ini

  • Added {tty:--color=yes} flag to pytest command for colored output
  • Improves test output readability when running in terminal
+1/-1     

@netlify
Copy link

netlify bot commented Nov 13, 2025

👷 Deploy request for selenium-dev pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 8452856

@qodo-merge-pro
Copy link
Contributor

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
Test logging scope: The changes adjust a test fixture for unique log filenames but do not add or remove any
audit logging of critical actions, so impact on audit trails cannot be determined from the
diff.

Referred Code
def log_path():
    log_path = f'log_file_{uuid.uuid4()}.log'

    yield log_path

    logger = logging.getLogger('selenium')

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Fixture cleanup errors: The fixture yields a path but does not handle possible errors during teardown (e.g., file
still in use) which was the source issue in parallel runs, though this may be acceptable
for tests.

Referred Code
def log_path():
    log_path = f'log_file_{uuid.uuid4()}.log'

    yield log_path

    logger = logging.getLogger('selenium')

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status:
No user output: The diff pertains to test fixtures and does not introduce user-facing error messages; no
clear violation or confirmation of secure error handling is present.

Referred Code
def log():
    logging.basicConfig(level=logging.WARN)
    logging.getLogger('selenium').setLevel(logging.DEBUG)


@pytest.fixture(scope='function')
def log_path():
    log_path = f'log_file_{uuid.uuid4()}.log'

    yield log_path

    logger = logging.getLogger('selenium')

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status:
Log content unknown: The change only randomizes log file names and sets logger levels; it does not reveal what
is logged, so exposure of sensitive data cannot be assessed from this diff.

Referred Code
def log():
    logging.basicConfig(level=logging.WARN)
    logging.getLogger('selenium').setLevel(logging.DEBUG)


@pytest.fixture(scope='function')
def log_path():
    log_path = f'log_file_{uuid.uuid4()}.log'

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
No external input: The new code introduces a UUID-based filename and modifies pytest invocation flags without
handling external inputs; security of data handling cannot be fully evaluated from this
diff.

Referred Code
def log_path():
    log_path = f'log_file_{uuid.uuid4()}.log'

Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-merge-pro
Copy link
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Isolate test logging in parallel runs

Refactor the log_path fixture to only manage the log file path and its deletion.
Remove the unsafe logger handler cleanup logic to prevent race conditions in
parallel tests.

examples/python/tests/conftest.py [98-109]

 @pytest.fixture(scope='function')
 def log_path():
     log_path = f'log_file_{uuid.uuid4()}.log'
 
     yield log_path
 
-    logger = logging.getLogger('selenium')
-    for handler in logger.handlers:
-        logger.removeHandler(handler)
-        handler.close()
+    # The fixture that adds the handler should be responsible for removing it.
+    # This fixture should only clean up the file it is responsible for.
+    try:
+        os.remove(log_path)
+    except FileNotFoundError:
+        pass
 
-    os.remove(log_path)
-
  • Apply / Chat
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a critical race condition in the logger handler teardown that will cause failures during parallel test execution, a feature being enabled in this PR via tox.ini.

High
  • More

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant