Skip to content

[ci][py] Update runners and tests#14729

Merged
VietND96 merged 1 commit intotrunkfrom
runner-macos
Nov 8, 2024
Merged

[ci][py] Update runners and tests#14729
VietND96 merged 1 commit intotrunkfrom
runner-macos

Conversation

@VietND96
Copy link
Copy Markdown
Member

@VietND96 VietND96 commented Nov 8, 2024 •
edited by qodo-code-review Bot
Loading

User description

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

Motivation and Context

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.

PR Type

enhancement, configuration changes

Description

  • Updated GitHub Actions workflows to use ubuntu-latest instead of ubuntu-20.04.
  • Simplified the Bazel workflow by removing specific macOS version targeting.
  • Enhanced the CI Python workflow by adding a matrix strategy for browser testing, allowing for parallel execution of tests across different browsers.
  • Unified browser tests under a single job in the CI Python workflow, improving maintainability.

Changes walkthrough 📝

Relevant files
Configuration changes
bazel.yml
Simplify macOS runner configuration in Bazel workflow       

.github/workflows/bazel.yml

  • Simplified the runner configuration for macOS.
  • Removed specific macOS version targeting.
    		•  +1/-1     
    label-commenter.yml
    Update label commenter workflow to latest runner                 

    .github/workflows/label-commenter.yml

    • Updated runner version to ubuntu-latest.
    		 +1/-1     
    Enhancement
    ci-python.yml
    Update CI Python workflow with matrix strategy and latest runner

    .github/workflows/ci-python.yml

  • Updated runner version to ubuntu-latest.
  • Added matrix strategy for browser testing.
  • Unified browser tests under a single job.
    		•  +26/-13 

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    @VietND96
    [ci][py] Update runners and tests
    6fdad31 
    Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>
    @qodo-code-review
    Copy link
    Copy Markdown
    Contributor

    qodo-code-review Bot commented Nov 8, 2024

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 Security concerns

    Sensitive information exposure:
    The workflow files contain references to secrets (GITHUB_TOKEN, SEL_M2_USER, SEL_M2_PASS). While these are properly used with GitHub's secret management, ensure that these secrets are properly configured in the repository settings and have appropriate access scopes.

    ⚡ Recommended focus areas for review

    Configuration Validation
    The matrix strategy for browser testing only includes Firefox for remote tests, while browser tests include Safari and Chrome. Verify if this limited browser coverage for remote tests is intentional.

    Performance Concern
    The flaky test attempts are set to 3 for all browser tests. Consider if this retry count is optimal for all browser types, as some might need different retry strategies.

    @qodo-code-review
    Copy link
    Copy Markdown
    Contributor

    qodo-code-review Bot commented Nov 8, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Add timeout constraints to prevent test executions from running indefinitely

    Add timeout limits to the browser tests to prevent potential hanging or long-running
    tests from blocking the CI pipeline.

    .github/workflows/ci-python.yml [112]

    -run: bazel test --flaky_test_attempts 3 //py:test-${{ matrix.browser }}
+run: bazel test --flaky_test_attempts 3 --test_timeout=900 //py:test-${{ matrix.browser }}
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding test timeouts is a critical best practice for CI pipelines to prevent hanging tests from blocking the entire workflow. This is especially important for browser tests which can be flaky.

    		8
    Enhancement
    Expand browser test coverage by including additional platform configurations in the test matrix

    Consider adding Chrome browser tests for Ubuntu platform as well, since it's a
    common testing environment and could catch platform-specific issues.

    .github/workflows/ci-python.yml [99-106]

     strategy:
   fail-fast: false
   matrix:
     include:
       - browser: safari
         os: macos
       - browser: chrome
         os: macos
+      - browser: chrome
+        os: ubuntu
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Adding Chrome tests on Ubuntu is valuable as it's a common CI environment and could catch platform-specific issues. The suggestion is well-aligned with the PR's goal of improving test coverage and matrix strategy.

    		7
    Possible issue
    Add input validation to prevent potential workflow failures due to invalid parameters

    Consider adding error handling for the case when inputs.os is empty or invalid, as
    format() function might fail in such cases.

    .github/workflows/bazel.yml [73]

    -runs-on: ${{ format('{0}-latest', inputs.os) }}
+runs-on: ${{ inputs.os != '' && format('{0}-latest', inputs.os) || 'ubuntu-latest' }}
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion adds important error handling for the workflow's input parameter, preventing potential failures when inputs.os is empty or invalid. This improves the workflow's robustness.

    		7

    💡 Need additional feedback ? start a PR chat

    @VietND96 VietND96 merged commit 7257cf3 into trunk Nov 8, 2024
    @VietND96 VietND96 deleted the runner-macos branch November 8, 2024 12:56
    @dependabot dependabot Bot mentioned this pull request Sep 18, 2025
    Merged
    @dependabot dependabot Bot mentioned this pull request Oct 1, 2025
    Closed
    @dependabot dependabot Bot mentioned this pull request Oct 24, 2025
    Closed
    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

    Review effort [1-5]: 2

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    1 participant

    @VietND96