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

lint: warn panics, expects and unwraps #1932

Merged
merged 4 commits into from
Dec 27, 2024

Conversation

carneiro-cw
Copy link
Contributor

@carneiro-cw carneiro-cw commented Dec 27, 2024

PR Type

Enhancement, Tests


Description

  • Enhanced linting rules:
    • Added warnings for expect and panic usage in Cargo.toml
    • Configured Clippy to allow expect and panic in tests
  • Improved test infrastructure:
    • Refactored e2e-admin-password test to use a loop, reducing code duplication
    • Split e2e-leader-follower-up into implementation and wrapper functions for better modularity
  • Optimized code coverage process:
    • Introduced _coverage-run-stratus-recipe for reusable coverage runs
    • Refactored coverage-related commands to reduce repetition and improve maintainability
  • Updated lint-check command to include new clippy flags, allowing for more flexible linting options

Changes walkthrough 📝

Relevant files
Configuration changes
.clippy.toml
Allow expect and panic in tests                                                   

.clippy.toml

  • Added allow-expect-in-tests = true
  • Added allow-panic-in-tests = true
  • +3/-1     
    Enhancement
    Cargo.toml
    Add warnings for expect and panic usage                                   

    Cargo.toml

    • Added expect_used = "warn"
    • Added panic = "warn"
    +2/-0     
    justfile
    Refactor test commands and improve code coverage                 

    justfile

  • Modified lint-check command to include new clippy flags
  • Refactored e2e-admin-password test to use a loop
  • Split e2e-leader-follower-up into implementation and wrapper functions
  • Refactored coverage-related commands for better reusability
  • +41/-64 

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

    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Potential Performance Impact

    The refactoring of the e2e-admin-password test into a loop might impact test execution time. Validate that this change doesn't significantly increase the overall test duration.

    for test in "enabled|test123" "disabled|"; do
        IFS="|" read -r type pass <<< "$test"
        just _log "Running admin password tests with password $type"
        ADMIN_PASSWORD=$pass just run -a 0.0.0.0:3000 > /dev/null &
        just _wait_for_stratus
        npx hardhat test test/admin/e2e-admin-password-$type.test.ts --network stratus
        killport 3000 -s sigterm
    done
    Logging Improvement

    The new _coverage-run-stratus-recipe function doesn't include any tracing or logging. Consider adding appropriate logging to track the execution of coverage runs.

    _coverage-run-stratus-recipe *recipe="":
        just {{recipe}}
        sleep 10
        -rm -r e2e_logs

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add error handling to prevent script continuation after critical failures

    In the stratus-test-coverage recipe, consider adding error handling for the cargo
    llvm-cov commands to ensure the script doesn't continue if these crucial steps fail.

    justfile [528-540]

    +set -e
     cargo llvm-cov clean --workspace
     just build
     source <(cargo llvm-cov show-env --export-prefix)
     export RUST_LOG=error
     just contracts-clone
     just contracts-flatten
     
     # cargo test
    -cargo llvm-cov --no-report
    +cargo llvm-cov --no-report || { echo "cargo llvm-cov failed"; exit 1; }
     sleep 10
    Suggestion importance[1-10]: 8

    Why: Adding error handling for critical commands like 'cargo llvm-cov' is important to prevent the script from continuing after a failure, which could lead to misleading test results or coverage reports.

    8
    General
    Improve process termination reliability in test cleanup

    Consider using a more robust method for process termination in the
    e2e-leader-follower-up recipe. The current killport command may not always
    successfully terminate the processes, especially if they're unresponsive.

    justfile [389-392]

     e2e-leader-follower-up test="brlc" release_flag="--release":
         just _e2e-leader-follower-up-impl {{test}} {{release_flag}}
    -    killport 3000 -s sigterm
    -    killport 3001 -s sigterm
    +    pkill -f "stratus.*:3000" || true
    +    pkill -f "stratus.*:3001" || true
    Suggestion importance[1-10]: 7

    Why: The suggestion improves the reliability of process termination, which is crucial for clean test execution and preventing interference between tests.

    7

    @carneiro-cw
    Copy link
    Contributor Author

    closes #1511

    @carneiro-cw carneiro-cw enabled auto-merge (squash) December 27, 2024 16:46
    @carneiro-cw carneiro-cw merged commit 57ff938 into main Dec 27, 2024
    36 checks passed
    @carneiro-cw carneiro-cw deleted the warn_panics_expects_and_unwraps branch December 27, 2024 16:51
    @mayconamaroCW mayconamaroCW linked an issue Dec 27, 2024 that may be closed by this pull request
    @gabriel-aranha-cw
    Copy link
    Contributor

    Final benchmark:
    Run ID: bench-2540146340

    Git Info:

    Configuration:

    • Target Account Strategy: Default

    RPS Stats: Max: 1259.00, Min: 781.00, Avg: 1146.96, StdDev: 40.36
    TPS Stats: Max: 1237.00, Min: 1038.00, Avg: 1112.40, StdDev: 48.47

    Plot: View Plot

    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.

    configure clippy to warn unwraps, expects and panics
    2 participants