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

[test]: fix node relay tests #2269

Merged
merged 1 commit into from
May 22, 2024
Merged

[test]: fix node relay tests #2269

merged 1 commit into from
May 22, 2024

Conversation

VietND96
Copy link
Member

@VietND96 VietND96 commented May 22, 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

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 conditional setup for containerd image store in GitHub Actions workflow to skip for test_node_relay strategy.
  • Updated OpenTelemetry and gRPC versions in the Dockerfile.
  • Enhanced Docker Compose commands in the Makefile by adding DOCKER_DEFAULT_PLATFORM and fixing browser name assignments.
  • Reorganized services and profiles in the Docker Compose file for node relay tests, including updated volume mounts and service dependencies.
  • Simplified relay configuration in relay_config.toml by using dynamic browser name variables.

Changes walkthrough 📝

Relevant files
Configuration changes
docker-test.yml
Conditional setup for containerd image store in GitHub Actions
workflow.

.github/workflows/docker-test.yml

  • Added condition to skip containerd image store setup for
    test_node_relay strategy.
  • +1/-0     
    Dependencies
    Dockerfile
    Update OpenTelemetry and gRPC versions in Dockerfile.       

    Base/Dockerfile

  • Updated OPENTELEMETRY_VERSION to 1.38.0.
  • Updated GRPC_VERSION to 1.64.0.
  • +2/-2     
    Enhancement
    Makefile
    Enhance Docker Compose commands and environment setup in Makefile.

    Makefile

  • Added DOCKER_DEFAULT_PLATFORM to Docker Compose commands.
  • Enhanced test_node_relay to include video directory setup and
    environment variable export.
  • Fixed browser name environment variable assignments.
  • +17/-13 
    docker-compose-v3-test-node-relay.yml
    Reorganize services and profiles in Docker Compose for node relay
    tests.

    tests/docker-compose-v3-test-node-relay.yml

  • Reorganized services and profiles.
  • Updated volume mount path for relay configuration.
  • Changed service names and dependencies.
  • +30/-40 
    relay_config.toml
    Simplify relay configuration with dynamic browser name.   

    tests/relay_config.toml

  • Updated relay URL to use standalone service.
  • Simplified browser configurations using BROWSER_NAME variable.
  • +2/-4     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Signed-off-by: Viet Nguyen Duc <[email protected]>
    @VietND96 VietND96 merged commit 8fa9113 into SeleniumHQ:trunk May 22, 2024
    14 of 15 checks passed
    Copy link

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    3, because the PR includes changes across multiple files and configurations, impacting different aspects of the system such as Docker configurations, Makefile commands, and Docker Compose setups. The changes are moderately complex, involving environment variables, conditional logic, and file paths which require careful review to ensure they are correct and do not disrupt existing functionalities.

    🧪 Relevant tests

    No

    ⚡ Possible issues

    Possible Bug: In the Makefile, the line && BROWSER_NAMEchrome >> .env seems to be a typo or syntax error. It likely should be && echo BROWSER_NAME=chrome >> .env.

    Configuration Error: The removal of the container_name in various services in docker-compose-v3-test-node-relay.yml might affect scripts or systems that depend on specific container names for operations or monitoring.

    Inconsistency: The docker-compose-v3-test-node-relay.yml file has significant structural changes, including the removal of certain services and the restructuring of others. This could lead to unforeseen issues if not properly tested, especially in environments relying on the previous configuration.

    🔒 Security concerns

    No

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Correct the typo to ensure the environment variable is set correctly

    Fix the typo in line 649 by changing BROWSER_NAMEchrome to echo BROWSER_NAME=chrome.

    Makefile [649]

    -&& BROWSER_NAMEchrome >> .env ;
    +&& echo BROWSER_NAME=chrome >> .env ;
     
    Suggestion importance[1-10]: 10

    Why: This suggestion correctly identifies and fixes a typo that would prevent the environment variable from being set correctly, which could lead to functional errors in the script.

    10
    Best practice
    Enclose the condition in quotes to ensure proper YAML parsing

    The condition matrix.test-strategy != 'test_node_relay' should be enclosed in quotes to
    avoid potential issues with YAML parsing and ensure it is treated as a string.

    .github/workflows/docker-test.yml [69]

    -if: matrix.test-strategy != 'test_node_relay'
    +if: "matrix.test-strategy != 'test_node_relay'"
     
    Suggestion importance[1-10]: 8

    Why: Enclosing the condition in quotes is a best practice in YAML to prevent parsing issues, especially with complex expressions.

    8

    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