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

ci: add support for isolated balance tracker repository in e2e scripts #1874

Merged
merged 2 commits into from
Nov 14, 2024

Conversation

mayconamaroCW
Copy link
Contributor

@mayconamaroCW mayconamaroCW commented Nov 14, 2024

PR Type

Enhancement


Description

This PR adds support for an isolated Balance Tracker repository in various e2e scripts:

  • Clone script: Added cloning of brlc-balance-tracker and improved error handling for npm install
  • Compile script: Added fallback compilation for BalanceTracker from different repositories
  • Coverage script: Included brlc-balance-tracker in coverage calculation
  • Flatten script: Updated to support BalanceTracker in new repository
  • Remove script: Added removal of brlc-balance-tracker repository
  • Test script: Adapted to support BalanceTracker in new repository

These changes ensure smooth transition and compatibility with the isolated Balance Tracker repository while maintaining support for the existing structure.


Changes walkthrough 📝

Relevant files
Enhancement
contracts-clone.sh
Improve clone script robustness and add balance tracker support

e2e/cloudwalk-contracts/contracts-clone.sh

  • Added error handling for npm install failure
  • Added cloning of brlc-balance-tracker repository
  • +6/-1     
    contracts-compile.sh
    Add flexible BalanceTracker compilation                                   

    e2e/cloudwalk-contracts/contracts-compile.sh

  • Added fallback compilation for BalanceTracker from different
    repositories
  • +3/-1     
    contracts-coverage.sh
    Include balance tracker in coverage calculation                   

    e2e/cloudwalk-contracts/contracts-coverage.sh

  • Added coverage calculation for brlc-balance-tracker with fallback
  • +1/-0     
    contracts-flatten.sh
    Adapt flattening for isolated BalanceTracker                         

    e2e/cloudwalk-contracts/contracts-flatten.sh

  • Updated flattening process to support BalanceTracker in new repository

  • +1/-1     
    contracts-remove.sh
    Include balance tracker in removal process                             

    e2e/cloudwalk-contracts/contracts-remove.sh

    • Added removal of brlc-balance-tracker repository
    +1/-0     
    contracts-test.sh
    Adapt testing for isolated BalanceTracker                               

    e2e/cloudwalk-contracts/contracts-test.sh

  • Updated testing process to support BalanceTracker in new repository
  • +3/-1     

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

    @mayconamaroCW mayconamaroCW requested a review from a team November 14, 2024 18:46
    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Error Handling
    The new error handling for npm install removes the target directory on failure. This might lead to unexpected behavior if the directory contains important files not managed by npm.

    Silent Failure
    The clone operation for brlc-balance-tracker silently continues if the clone fails. This might lead to issues in subsequent steps that depend on this repository.

    Coverage Gap
    The coverage calculation for brlc-balance-tracker is skipped if the repository is not isolated. This might lead to incomplete coverage reports.

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Improve error handling and logging for npm install failures to aid in debugging

    Consider using a more robust error handling mechanism for the npm install command.
    The current approach of removing the directory and returning might not provide
    enough context about the failure.

    e2e/cloudwalk-contracts/contracts-clone.sh [40-44]

     if ! npm --prefix "$target" --silent install; then
    -    log "Dependencies install failed. Removing folder and exiting."
    -    rm -rf "$target"
    +    log "Dependencies install failed for $repo. Error code: $?"
    +    log "Keeping folder for debugging. Please check $target for issues."
         return 1
     fi
    Suggestion importance[1-10]: 7

    Why: The suggestion improves error handling by providing more context about the failure, which can significantly aid in debugging. It also suggests keeping the folder for inspection instead of removing it, which is a valuable change for troubleshooting.

    7
    Enhance logging to clearly indicate which repository was used for compiling BalanceTracker

    Consider adding a log message to indicate which repository was successfully used for
    compiling BalanceTracker, as it improves traceability and debugging.

    e2e/cloudwalk-contracts/contracts-compile.sh [48]

    -compile_contract brlc-balance-tracker BalanceTracker || compile_contract brlc-yield-streamer BalanceTracker
    +if compile_contract brlc-balance-tracker BalanceTracker; then
    +    log "Compiled BalanceTracker from brlc-balance-tracker"
    +elif compile_contract brlc-yield-streamer BalanceTracker; then
    +    log "Compiled BalanceTracker from brlc-yield-streamer"
    +else
    +    log "Failed to compile BalanceTracker from either repository"
    +    exit 1
    +fi
    Suggestion importance[1-10]: 6

    Why: This suggestion improves traceability by clearly indicating which repository was used for compiling BalanceTracker. It also adds proper error handling, which enhances the script's robustness.

    6
    Improve logging to clearly indicate which repository was used for testing BalanceTracker

    Consider adding a log message to indicate which repository was successfully used for
    testing BalanceTracker, as it improves traceability and debugging.

    e2e/cloudwalk-contracts/contracts-test.sh [155]

    -test brlc-balance-tracker BalanceTracker "$@" || test brlc-yield-streamer BalanceTracker "$@"
    +if test brlc-balance-tracker BalanceTracker "$@"; then
    +    log "Tested BalanceTracker from brlc-balance-tracker"
    +elif test brlc-yield-streamer BalanceTracker "$@"; then
    +    log "Tested BalanceTracker from brlc-yield-streamer"
    +else
    +    log "Failed to test BalanceTracker from either repository"
    +    exit 1
    +fi
    Suggestion importance[1-10]: 6

    Why: This suggestion enhances traceability and debugging by clearly indicating which repository was used for testing BalanceTracker. It also adds proper error handling, improving the script's robustness.

    6
    Best practice
    Standardize logging method for consistency across the script

    Instead of using echo for skipping Balance Tracker coverage, consider using the log
    function for consistency with other logging in the script.

    e2e/cloudwalk-contracts/contracts-coverage.sh [55]

    -coverage brlc-balance-tracker || echo "Balance Tracker is not isolated yet. Skipping..."
    +coverage brlc-balance-tracker || log "Balance Tracker is not isolated yet. Skipping..."
    Suggestion importance[1-10]: 4

    Why: While this suggestion improves consistency in logging, it's a minor change that doesn't significantly impact functionality. It's a good practice but has a relatively low impact on the overall script.

    4

    @mayconamaroCW mayconamaroCW merged commit 146330d into main Nov 14, 2024
    31 checks passed
    @mayconamaroCW mayconamaroCW deleted the ci/balance-tracker-rename branch November 14, 2024 20:37
    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