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: add test for node docker (dynamic grid) #2177

Merged
merged 4 commits into from Mar 29, 2024
Merged

Conversation

VietND96
Copy link
Member

@VietND96 VietND96 commented Mar 29, 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

test: add test for node docker (dynamic grid)

Motivation and Context

Test in part of CI to guard dynamic grid works against regression changes

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.

Type

enhancement, tests


Description

  • Updated Docker Python package version to 7.0.0 in tests/bootstrap.sh.
  • Installed Docker Compose v2.26.0 for AMD64/x86_64 in tests/charts/make/chart_setup_env.sh.
  • Updated GitHub Actions workflow name to "Test Docker Selenium" and added test_node_docker to the test strategy in .github/workflows/test-video.yml.
  • Updated Selenium version to 4.19.1 and added test_node_docker target in Makefile.
  • Added a new TOML configuration file for Docker tests in tests/config.toml.
  • Introduced a new Docker Compose file for testing node docker in tests/docker-compose-v3-test-node-docker.yaml.

Changes walkthrough

Relevant files
Enhancement
bootstrap.sh
Update Docker Python Package Version                                         

tests/bootstrap.sh

  • Updated docker Python package version from 6.1.3 to 7.0.0.
+1/-1     
chart_setup_env.sh
Install Docker Compose for AMD64/x86_64                                   

tests/charts/make/chart_setup_env.sh

  • Added installation steps for Docker Compose version v2.26.0 for
    AMD64/x86_64 architectures.
  • +7/-0     
    test-video.yml
    Update GitHub Actions Workflow for Docker Selenium Tests 

    .github/workflows/test-video.yml

  • Changed workflow name to "Test Docker Selenium".
  • Added test_node_docker to the test strategy matrix.
  • +2/-2     
    Makefile
    Update Selenium Version and Add Test Node Docker Target   

    Makefile

  • Updated BASE and VERSION variables to 4.19.1 from 4.19.0.
  • Added test_node_docker target for testing node docker.
  • +21/-2   
    Tests
    config.toml
    Add TOML Configuration for Docker Tests                                   

    tests/config.toml

    • Added a new TOML configuration file for Docker tests.
    +11/-0   
    docker-compose-v3-test-node-docker.yaml
    Add Docker Compose for Node Docker Testing                             

    tests/docker-compose-v3-test-node-docker.yaml

    • Introduced a new Docker Compose file for testing node docker.
    +39/-0   

    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 <nguyenducviet4496@gmail.com>
    Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>
    Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>
    Copy link

    PR Description updated to latest commit (ac68923)

    Copy link

    PR Review

    ⏱️ Estimated effort to review [1-5]

    2, because the PR primarily focuses on adding tests and updating dependencies, which are usually straightforward to review. However, it introduces changes across multiple files and configurations that need to be verified for compatibility and correctness.

    🧪 Relevant tests

    Yes

    🔍 Possible issues

    Dependency Version Mismatch: The PR updates the Docker dependency to version 7.0.0 and the Selenium base version to 4.19.1. It's important to ensure that these new versions are compatible with the rest of the project's dependencies and the intended use cases.

    Hardcoded Docker Compose Version: The PR hardcodes the Docker Compose version to v2.26.0. This could lead to future compatibility issues or prevent the use of new Docker Compose features. Consider using a configurable approach or documenting the reason for this specific version choice.

    Test Coverage: While the PR adds a new test (test_node_docker), it's crucial to ensure that this test, along with the existing ones, covers all the critical paths and potential edge cases introduced by the changes.

    🔒 Security concerns

    No


    ✨ Review tool usage guide:

    Overview:
    The review tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be added by configuring the tool.

    The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.

    • When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:
    /review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...
    
    [pr_reviewer]
    some_config1=...
    some_config2=...
    

    See the review usage page for a comprehensive guide on using this tool.

    Copy link

    PR Code Suggestions

    CategorySuggestions                                                                                                                                                       
    Possible issue
    Correct the version specification typo for the docker package.

    It appears there's a typo in the version specification for the docker package, using three
    equal signs instead of two. This could lead to unexpected behavior or installation issues.
    Correct the version specification to use only two equal signs.

    tests/bootstrap.sh [11]

    -docker===7.0.0 \
    +docker==7.0.0 \
     
    Enhancement
    Ensure docker-compose is successfully downloaded before proceeding with permissions change and move.

    Consider verifying the successful download of docker-compose before attempting to change
    its permissions and move it. This can prevent partial or failed downloads from causing
    issues.

    tests/charts/make/chart_setup_env.sh [33-35]

    -curl -fsSL -o ./docker-compose "https://github.com/docker/compose/releases/download/${DOCKER_COMPOSE_VERSION}/docker-compose-linux-x86_64"
    -chmod +x ./docker-compose
    +curl -fsSL -o ./docker-compose "https://github.com/docker/compose/releases/download/${DOCKER_COMPOSE_VERSION}/docker-compose-linux-x86_64" && \
    +chmod +x ./docker-compose && \
     sudo mv ./docker-compose /usr/libexec/docker/cli-plugins
     
    Rename the workflow to reflect its broader test coverage.

    The workflow name 'Test Docker Selenium' might be too specific if the workflow is intended
    to cover multiple test strategies. Consider renaming it to something more general that
    reflects all the types of tests being run.

    .github/workflows/test-video.yml [1]

    -name: Test Docker Selenium
    +name: Selenium Test Suite
     
    Maintainability
    Simplify and improve the efficiency of the test_node_docker target using Makefile variables and pattern rules.

    The test_node_docker target uses a for-loop with a subshell to set environment variables,
    which is less efficient and harder to maintain. Consider using a Makefile variable for the
    node types and a pattern rule to simplify and improve the efficiency of this target.

    Makefile [430-446]

    -for node in StandaloneChrome StandaloneFirefox StandaloneEdge ; do \
    -    ...
    -done
    +NODE_TYPES := StandaloneChrome StandaloneFirefox StandaloneEdge
    +$(NODE_TYPES):
    +	@echo "Running tests for $@"
    +	...
    +test_node_docker: $(NODE_TYPES)
     
    Security
    Avoid direct Docker socket mounting in docker-compose for security reasons.

    The docker-compose file specifies a direct volume mount for /var/run/docker.sock, which
    can pose security risks by allowing containers to control the Docker daemon. Consider
    using a more secure approach, such as Docker-in-Docker (DinD) or a dedicated tool like
    docker-compose for managing containers within containers.

    tests/docker-compose-v3-test-node-docker.yaml [8]

    -- /var/run/docker.sock:/var/run/docker.sock
    +# Consider a more secure approach than direct Docker socket mounting
     

    ✨ Improve tool usage guide:

    Overview:
    The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.

    • When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:
    /improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...
    
    [pr_code_suggestions]
    some_config1=...
    some_config2=...
    

    See the improve usage page for a comprehensive guide on using this tool.

    @VietND96 VietND96 merged commit cc2b0a4 into SeleniumHQ:trunk Mar 29, 2024
    5 of 11 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.

    None yet

    1 participant