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] Add simple Playwright parallel tests connect to autoscaling Grid in K8s #2409

Merged
merged 1 commit into from
Sep 24, 2024

Conversation

VietND96
Copy link
Member

@VietND96 VietND96 commented Sep 24, 2024

User description

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

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

Description

Motivation and Context

Add simple Playwright tests to guard against regression changes could break the experimental Playwright can connect to Selenium Grid - https://playwright.dev/java/docs/selenium-grid

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

Tests, Enhancement


Description

  • Added Playwright test setup and configuration for Selenium Grid.
  • Integrated Playwright tests into existing chart testing scripts.
  • Updated CI/CD configurations to include Playwright tests.
  • Added new test cases for Playwright covering multiple scenarios.
  • Updated Helm and Docker versions in CI configurations.

Changes walkthrough 📝

Relevant files
Tests
bootstrap.sh
Add Playwright test setup script for Selenium Grid             

tests/CDPTests/bootstrap.sh

  • Added a script to set up Playwright tests.
  • Configures environment variables for Selenium Grid.
  • Waits for the Selenium Grid to be ready before running tests.
  • +26/-0   
    chart_test.sh
    Integrate Playwright tests into chart testing                       

    tests/charts/make/chart_test.sh

  • Added condition to run CDPTests for Playwright.
  • Executes Playwright tests for Chrome and Microsoft Edge.
  • +3/-0     
    Tests.ts
    Add Playwright test cases for Selenium Grid                           

    tests/CDPTests/tests/Tests.ts

  • Added Playwright test cases for various scenarios.
  • Includes tests for page navigation, frames, dropdowns, and file
    download.
  • +50/-0   
    Configuration changes
    config.yml
    Add Playwright Kubernetes test job and update versions     

    .circleci/config.yml

  • Added a new Kubernetes test job for Playwright.
  • Updated Helm and Docker versions for various jobs.
  • +29/-10 
    helm-chart-test.yml
    Add Playwright test strategy to GitHub Actions                     

    .github/workflows/helm-chart-test.yml

  • Added Playwright test strategy to GitHub Actions.
  • Updated Helm and Docker versions for test jobs.
  • +16/-10 
    Makefile
    Add Makefile target for Playwright connect grid test         

    Makefile

  • Added target for Playwright connect grid test.
  • Configures secure ingress and basic auth for tests.
  • +5/-5     
    playwright.config.ts
    Add Playwright configuration for test settings                     

    tests/CDPTests/playwright.config.ts

  • Added Playwright configuration file.
  • Configures test settings and reporters.
  • +22/-0   
    Dependencies
    package.json
    Add package.json for CDPTests with dependencies                   

    tests/CDPTests/package.json

  • Added package.json for CDPTests with Playwright dependencies.
  • Includes dotenv and express packages.
  • +21/-0   

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

    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 PR contains tests
    🔒 Security concerns

    Sensitive information exposure:
    The script in 'tests/CDPTests/bootstrap.sh' is using base64 encoding for basic authentication (lines 14-15). This method is not secure as base64 is easily decodable. It's recommended to use more secure methods for handling credentials, such as environment variables or secure secret management systems.

    ⚡ Key issues to review

    Security Concern
    The script is using base64 encoding for basic authentication, which is not secure. Consider using a more secure method for handling credentials.

    Potential Flakiness
    The use of fixed sleep times (sleep(2)) in tests may lead to flaky tests. Consider using more robust waiting mechanisms.

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Use double square brackets for bash conditional expressions

    Use double square brackets for the if condition to improve robustness and avoid
    potential issues with empty variables.

    tests/CDPTests/bootstrap.sh [13]

    -if [ -n ${SELENIUM_GRID_USERNAME} ] && [ -n ${SELENIUM_GRID_PASSWORD} ]; then
    +if [[ -n ${SELENIUM_GRID_USERNAME} ]] && [[ -n ${SELENIUM_GRID_PASSWORD} ]]; then
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Using double square brackets in bash scripts is a best practice that improves robustness and avoids potential issues with empty variables, making the script more reliable.

    9
    Enhancement
    Add error handling for file download test

    Add error handling for the file download test to ensure the download was successful.

    tests/CDPTests/tests/Tests.ts [47-49]

     const fileName = download.suggestedFilename();
     expect(fileName).toBe('some-file.txt');
    +const path = await download.path();
    +expect(path).toBeTruthy();
     await sleep(2);
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding error handling ensures that the file download test verifies the download path, increasing test reliability and robustness.

    8
    Use environment variables for configurable options

    Consider using environment variables for configurable options like the number of
    workers to make the configuration more flexible.

    tests/CDPTests/playwright.config.ts [19]

    -workers: 5
    +workers: process.env.PLAYWRIGHT_WORKERS ? parseInt(process.env.PLAYWRIGHT_WORKERS) : 5
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Using environment variables for configuration options like the number of workers increases flexibility and adaptability of the test configuration, allowing for easier adjustments without code changes.

    8
    Maintainability
    Use consistent async/await pattern in test cases

    Use async/await consistently throughout the test cases instead of mixing with
    Promise.all().

    tests/CDPTests/tests/Tests.ts [42-45]

    -const [download] = await Promise.all([
    -    page.waitForEvent('download'),
    -    fileLink.click()
    -]);
    +await fileLink.click();
    +const download = await page.waitForEvent('download');
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion improves code consistency and readability by using async/await consistently, but the original code with Promise.all() is also valid and efficient.

    7

    💡 Need additional feedback ? start a PR chat

    … in K8s
    
    Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>
    @VietND96 VietND96 merged commit e61f20c into SeleniumHQ:trunk Sep 24, 2024
    27 of 34 checks passed
    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