Skip to content
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.

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

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

Merged
merged 1 commit into from
Nov 8, 2024
Merged

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

merged 1 commit into from
Nov 8, 2024

Conversation

VietND96
Copy link
Member

@VietND96 VietND96 commented Nov 8, 2024

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

    Signed-off-by: Viet Nguyen Duc <[email protected]>
    Copy link
    Contributor

    qodo-merge-pro 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.

    Copy link
    Contributor

    qodo-merge-pro 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
    15 checks passed
    @VietND96 VietND96 deleted the runner-macos branch November 8, 2024 12:56
    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