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

[feat][test]: add env vars to generate config for Relay in node-base #2254

Merged
merged 1 commit into from May 13, 2024

Conversation

VietND96
Copy link
Member

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

[feat][test]: add env vars to generate config for Relay in node-base

Motivation and Context


Node configuration relay commands

Relaying commands to a service endpoint that supports WebDriver.
It is useful to connect an external service that supports WebDriver to Selenium Grid. An example of such service could be a cloud provider or an Appium server.
In this way, Grid can enable more coverage to platforms and versions not present locally.

The following is an en example of connecting an Appium server to Grid.

docker-compose-v3-relay.yml

If you want to relay commands only, selenium/node-base is suitable and lightweight for this purpose.
In case you want to configure node with both browsers and relay commands, selenium/node-chrome or selenium/node-docker (with TOML configuration options) can be used.

To run a sample test with the relayed node, you can clone the project and try below command:

make test_node_relay

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, tests


Description

  • Introduced environment variables and conditional logic for node relay testing in Python tests.
  • Updated GitHub Actions workflows by adding node relay tests and removing scheduled cron jobs.
  • Added new Makefile targets and Docker Compose configurations to support node relay testing.
  • Enhanced node configuration scripts to include relay options and updated documentation accordingly.
  • Created a new Dockerfile for an Android testing environment.

Changes walkthrough 📝

Relevant files
Enhancement
4 files
__init__.py
Add Environment Variables and Conditional Delay for Node Relay

tests/SeleniumTests/init.py

  • Added environment variables for node relay and Android platform API.
  • Introduced a conditional delay when TEST_NODE_RELAY is not 'false'.
  • +13/-0   
    Makefile
    Introduce Make Targets for Node Relay Testing                       

    Makefile

    • Added new make targets for testing node relay configurations.
    +25/-0   
    generate_config
    Enhance Node Configuration Script with Relay Options         

    NodeBase/generate_config

  • Added relay configuration blocks to the node configuration script.
  • Adjusted node stereotype and driver configuration settings.
  • +49/-12 
    Dockerfile.android
    Create Dockerfile for Android Testing Environment               

    tests/Dockerfile.android

  • Added a new Dockerfile for building an Android-based testing
    environment.
  • +13/-0   
    Configuration changes
    5 files
    docker-test.yml
    Update GitHub Actions for Docker Tests with Node Relay     

    .github/workflows/docker-test.yml

  • Removed scheduled cron jobs.
  • Added a new test strategy for node relay with corresponding setup
    steps.
  • +8/-2     
    helm-chart-test.yml
    Remove Scheduled Cron Jobs from Helm Chart Tests                 

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

    • Removed scheduled cron jobs.
    +0/-2     
    nightly.yaml
    Update Nightly Workflow to Conditionally Include Tests     

    .github/workflows/nightly.yaml

  • Added conditional execution for docker and helm chart tests based on
    event triggers.
  • +9/-0     
    docker-compose-v3-relay.yml
    Add Docker Compose Configuration for Node Relay                   

    docker-compose-v3-relay.yml

  • Created a new Docker Compose file for setting up environments with
    node relay.
  • +66/-0   
    docker-compose-v3-test-node-relay.yml
    Setup Docker Compose for Node Relay Testing                           

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

    • Introduced a Docker Compose configuration for node relay testing.
    +94/-0   
    Documentation
    1 files
    README.md
    Document Node Configuration Relay Commands                             

    README.md

    • Added documentation for node configuration relay commands.
    +19/-0   

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

    Copy link

    PR Description updated to latest commit (95e3b54)

    Copy link

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    4, because the PR involves multiple complex changes across various files including Dockerfiles, Makefile, GitHub Actions workflows, and Python test scripts. The introduction of new environment variables and conditional logic increases the complexity, requiring careful review to ensure compatibility and correctness.

    🧪 Relevant tests

    Yes

    ⚡ Possible issues

    Hardcoded Values: The use of hardcoded values (e.g., URLs, platform names) in the Dockerfiles and test scripts might limit flexibility and reusability.

    Conditional Logic in Tests: The introduction of conditional logic based on environment variables in test scripts could lead to scenarios where certain code paths are not tested under different configurations.

    Delay in Tests: The hardcoded sleep of 120 seconds in the Python test script could introduce unnecessary delays in test execution, potentially impacting the overall test suite performance.

    🔒 Security concerns

    No

    Copy link

    PR Code Suggestions ✨

    CategorySuggestions                                                                                                                                                       
    Enhancement
    Convert string comparison to boolean for consistency and reliability.

    Convert the string comparison for TEST_NODE_RELAY to a boolean check to ensure consistency
    with other environment variable checks in the code.

    tests/SeleniumTests/init.py [30]

    -if TEST_NODE_RELAY != 'false':
    +if TEST_NODE_RELAY.lower() == 'true':
     
    Make the sleep delay configurable via an environment variable for better flexibility.

    Replace the hard-coded sleep time with a configurable environment variable to allow more
    flexible test setups.

    tests/SeleniumTests/init.py [31]

    -time.sleep(120)
    +time.sleep(int(os.environ.get('RELAY_ACTIVATION_DELAY', 120)))
     
    Bug
    Improve the reliability of platform string comparison by making it case-insensitive.

    Use a more specific condition to check for the Android platform to avoid potential issues
    with case sensitivity or unexpected platform strings.

    tests/SeleniumTests/init.py [134]

    -if TEST_NODE_RELAY == 'Android':
    +if TEST_NODE_RELAY.lower() == 'android':
     
    Possible issue
    Add error handling for a potentially unset environment variable to prevent runtime errors.

    Add error handling for the environment variable TEST_ANDROID_PLATFORM_API to ensure it is
    not None before setting capabilities, which could cause runtime errors.

    tests/SeleniumTests/init.py [136]

    -options.set_capability('appium:platformVersion', TEST_ANDROID_PLATFORM_API)
    +if TEST_ANDROID_PLATFORM_API:
    +    options.set_capability('appium:platformVersion', TEST_ANDROID_PLATFORM_API)
    +else:
    +    raise ValueError("TEST_ANDROID_PLATFORM_API environment variable is not set")
     
    Add validation for setting the 'platformName' capability to prevent setting invalid values.

    Ensure that the options.set_capability for 'platformName' is set to a valid value when
    TEST_NODE_RELAY is not 'Android'.

    tests/SeleniumTests/init.py [141]

    -options.set_capability('platformName', 'Linux')
    +if TEST_NODE_RELAY.lower() == 'linux':
    +    options.set_capability('platformName', 'Linux')
    +else:
    +    raise ValueError("Unsupported TEST_NODE_RELAY value")
     

    Copy link
    Member

    @diemol diemol left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Thank you for doing this, @VietND96.

    Offering a way to configure Relay is a good idea, but the PR adds too much complexity to the project. Let me explain:

    • We already have too many environment variables and need to document them all at some point. We recommend to use config.toml files to configure the Grid, so I prefer to offer a way to the user to mount it and use it for Relay. It is easier to read a file instead of several environment variables.
    • I know Relay is a common way to integrate Appium. However, the tests in this PR are hard to maintain, and the Android image is know to work slowly when nested virtualization is not present. If we want to test this, I prefer to start a Standalone container, and have it as the receiver of the forwarded commands through Relay.

    @VietND96
    Copy link
    Member Author

    VietND96 commented May 8, 2024

    @diemol, I see --config can pass multiple toml files. Can we separate another generate_relay_config and isolate related env vars there? Of course, still many env vars, but it is readable. Because there were few env vars with default values get/set. A toml config file can be defined and mounted to the container, however it needs to rewrite all without reusable existing. Separate out another toml file for relay I think it would help in case we want to mount specific relay configs within reuse existing default node configs.

    @diemol
    Copy link
    Member

    diemol commented May 9, 2024

    It is a good idea to have it as a different file. But I still dislike the environment variables approach; for example, setting the stereotypes via environment variables makes it very hard to read and maintain.

    @VietND96 VietND96 force-pushed the relay-service branch 6 times, most recently from 1300ae2 to ceb38b3 Compare May 10, 2024 09:18
    Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>
    @VietND96
    Copy link
    Member Author

    VietND96 commented May 10, 2024

    @diemol, few changes have been done

    • I don't add the env var for setting complex stereotypes to balance both approaches. Env vars are used for basic config to be generated. For complex stereotypes, let the user disable generate config mode and input a TOML config file.
    • Add a test with start a Standalone container, and have it as the receiver of the forwarded commands through Relay.
    • I would like to keep a test for the node relay to Appium (for reason like a reference test to verify & confirm something works quickly) . If the test is not stable or hard to maintain later, simplify to remove Android from the for loop in Makefile :)

    @VietND96 VietND96 requested a review from diemol May 10, 2024 11:37
    @VietND96 VietND96 merged commit 24527c6 into SeleniumHQ:trunk May 13, 2024
    15 checks passed
    @VietND96 VietND96 deleted the relay-service branch May 13, 2024 11:27
    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

    2 participants