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: block_modes can receive multiple modes #1571

Merged
merged 2 commits into from
Jul 30, 2024
Merged

test: block_modes can receive multiple modes #1571

merged 2 commits into from
Jul 30, 2024

Conversation

dinhani-cw
Copy link
Contributor

@dinhani-cw dinhani-cw commented Jul 30, 2024

PR Type

Enhancement


Description

  • Enhanced the E2E testing script to support multiple block modes by renaming block-mode to block_modes.
  • Added logic to split the block_modes variable and iterate over each mode.
  • Updated the test execution commands to handle multiple block modes, allowing for more flexible testing configurations.

Changes walkthrough 📝

Relevant files
Enhancement
justfile
Support multiple block modes in E2E tests                               

justfile

  • Renamed block-mode to block_modes to support multiple modes.
  • Added logic to split and iterate over multiple block modes.
  • Updated test execution commands to handle multiple block modes.
  • +11/-6   

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

    @dinhani-cw dinhani-cw enabled auto-merge (squash) July 30, 2024 19:21
    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Code Smell
    The loop iterating over block_modes_split should include logging for each test execution to ensure proper monitoring and debugging. Additionally, consider using traced_sleep if any sleep operations are added in the future.

    Copy link

    github-actions bot commented Jul 30, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Use IFS for splitting the block_modes string to handle special characters safely

    To avoid potential issues with special characters in the block_modes variable,
    consider using IFS (Internal Field Separator) instead of sed for splitting the
    string.

    justfile [148-149]

    -block_modes_split=$(echo {{block_modes}} | sed "s/,/ /g")
    -for block_mode in $block_modes_split
    +IFS=',' read -r -a block_modes_split <<< "{{block_modes}}"
    +for block_mode in "${block_modes_split[@]}"
     
    Suggestion importance[1-10]: 9

    Why: Using IFS for splitting the string is a best practice in bash scripting as it handles special characters more safely and reliably than using sed. This improves the robustness of the script.

    9
    Possible issue
    Add a check to ensure block_modes is not empty before processing

    Add a check to ensure that the block_modes variable is not empty before proceeding
    with the loop to avoid unnecessary execution.

    justfile [148-149]

     block_modes_split=$(echo {{block_modes}} | sed "s/,/ /g")
    -for block_mode in $block_modes_split
    +if [ -n "$block_modes_split" ]; then
    +    for block_mode in $block_modes_split
     
    Suggestion importance[1-10]: 8

    Why: Adding a check to ensure block_modes is not empty before entering the loop prevents unnecessary execution and potential errors, improving the script's reliability.

    8
    Maintainability
    Extract the repeated npx hardhat test command into a separate function for better readability

    To improve readability and maintainability, consider extracting the repeated npx
    hardhat test command into a separate function.

    justfile [152-155]

    -if [ -z "{{test}}" ]; then
    -    BLOCK_MODE=$block_mode npx hardhat test test/$block_mode/*.test.ts --network {{network}}
    -else
    -    BLOCK_MODE=$block_mode npx hardhat test test/$block_mode/*.test.ts --network {{network}} --grep "{{test}}"
    -fi
    +run_tests() {
    +    if [ -z "{{test}}" ]; then
    +        BLOCK_MODE=$1 npx hardhat test test/$1/*.test.ts --network {{network}}
    +    else
    +        BLOCK_MODE=$1 npx hardhat test test/$1/*.test.ts --network {{network}} --grep "{{test}}"
    +    fi
    +}
    +...
    +for block_mode in $block_modes_split; do
    +    just _log "Executing: $block_mode"
    +    run_tests $block_mode
    +done
     
    Suggestion importance[1-10]: 7

    Why: Extracting the repeated command into a function improves code readability and maintainability by reducing redundancy and making the script easier to understand and modify.

    7

    @dinhani-cw dinhani-cw disabled auto-merge July 30, 2024 19:25
    @dinhani-cw dinhani-cw merged commit 3cdfd39 into main Jul 30, 2024
    33 checks passed
    @dinhani-cw dinhani-cw deleted the test-justfile branch July 30, 2024 19:26
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    None yet
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant