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

fix(e2e): update performance test parameters and bump go-tss #2563

Merged
merged 2 commits into from
Jul 31, 2024

Conversation

gartnera
Copy link
Member

@gartnera gartnera commented Jul 25, 2024

Timeout increases are needed because some withdrawals are taking longer than the default 5 minute cctx timeout:

perf_eth_w | index 47: withdraw cctx success in 5m22.401391814s
perf_eth_w | index 48: withdraw cctx success in 5m20.393530031s

Closes #2562

Summary by CodeRabbit

  • New Features

    • Enhanced performance testing with updated timeout defaults for Ethereum withdrawal tests.
    • Improved timeout management in the test runner for better synchronization.
  • Chores

    • Updated dependencies in the project to ensure improved performance and stability.

@gartnera gartnera added the no-changelog Skip changelog CI check label Jul 25, 2024
Copy link
Contributor

coderabbitai bot commented Jul 25, 2024

Walkthrough

Walkthrough

The recent changes enhance the performance testing framework by implementing default timeout values for Ethereum withdrawal operations, updating dependency versions for stability, and refining timeout management in the test runner. These modifications aim to address persistent performance test failures and improve overall robustness and reliability in transaction handling.

Changes

Files Change Summary
Makefile, cmd/zetae2e/local/performance.go, cmd/zetae2e/local/test_runner.go Updated timeout settings and command execution for performance tests to enhance reliability and control flow.
go.mod Updated dependency versions for go-tss and transitioned from go-libp2p to zeta-chain/go-libp2p for improved functionality.

Assessment against linked issues

Objective Addressed Explanation
Performance Test Failures (###2562)
Timeout Issues (###2562)
Withdrawal Hang (###2562) It's unclear if the changes fully resolve the hanging issue.
Recent Changes Impact (###2562)
Keysign Failures (###2562) No specific adjustments were made to address keysigning issues.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@gartnera gartnera added the PERFORMANCE_TESTS Run make start-e2e-performance-test label Jul 29, 2024
Copy link

codecov bot commented Jul 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.57%. Comparing base (ca7f98d) to head (9080b61).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #2563   +/-   ##
========================================
  Coverage    71.57%   71.57%           
========================================
  Files          327      327           
  Lines        17798    17798           
========================================
  Hits         12739    12739           
  Misses        4490     4490           
  Partials       569      569           

@gartnera gartnera changed the title fix(e2e): increase performance test timeouts fix(e2e): update performance tests parameters and bump go-tss Jul 29, 2024
@gartnera gartnera force-pushed the fix-performance-tests branch from 06b5b04 to 9080b61 Compare July 29, 2024 19:58
@gartnera gartnera changed the title fix(e2e): update performance tests parameters and bump go-tss fix(e2e): update performance test parameters and bump go-tss Jul 29, 2024
@gartnera gartnera marked this pull request as ready for review July 30, 2024 15:50
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Outside diff range, codebase verification and nitpick comments (1)
go.mod (1)

364-364: Issue Found: Dependency Replacement Not Applied

The replacement of github.com/libp2p/go-libp2p with github.com/zeta-chain/go-libp2p has not been applied correctly. The original dependency is still present in the following files:

  • zetaclient/tss/tss_signer.go
  • cmd/zetaclientd/p2p_diagnostics.go

Please ensure that all instances of github.com/libp2p/go-libp2p are replaced with github.com/zeta-chain/go-libp2p to align with the intended changes.

Analysis chain

LGTM! Verify the impact of the go-libp2p replacement.

The replacement of github.com/libp2p/go-libp2p with github.com/zeta-chain/go-libp2p is approved.

However, ensure that the new dependency does not introduce any breaking changes or regressions in the codebase.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the impact of the `go-libp2p` replacement.

# Test: Search for the usage of `go-libp2p` in the codebase. Expect: No breaking changes or regressions.
rg --type go 'go-libp2p'

Length of output: 753


Script:

#!/bin/bash
# Description: Verify the replacement of `go-libp2p` with `zeta-chain/go-libp2p`.

# Test: Search for the usage of `zeta-chain/go-libp2p` in the codebase to ensure proper replacement.
rg --type go 'github.com/zeta-chain/go-libp2p'

Length of output: 46


Script:

#!/bin/bash
# Description: Verify if the original `go-libp2p` dependency is still present in the codebase.

# Test: Search for the usage of `github.com/libp2p/go-libp2p` in the codebase to confirm its presence.
rg --type go 'github.com/libp2p/go-libp2p'

Length of output: 771

Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between ca7f98d and 9080b61.

Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
Files selected for processing (4)
  • Makefile (1 hunks)
  • cmd/zetae2e/local/performance.go (1 hunks)
  • cmd/zetae2e/local/test_runner.go (1 hunks)
  • go.mod (1 hunks)
Additional context used
Path-based instructions (2)
cmd/zetae2e/local/test_runner.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

cmd/zetae2e/local/performance.go (1)

Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.

Additional comments not posted (4)
cmd/zetae2e/local/test_runner.go (1)

33-35: LGTM! Ensure consistent usage of timeout settings.

The added lines to copy timeout values from the deployer runner improve consistency in timeout handling.

However, ensure that these timeout settings are correctly used across the codebase.

Verification successful

LGTM! Ensure consistent usage of timeout settings.

The added lines to copy timeout values from the deployer runner improve consistency in timeout handling.

Verified consistent usage of CctxTimeout and ReceiptTimeout across the codebase.

The timeout settings are correctly used in various files for context management and transaction receipt handling, which aligns with their intended purpose.

  • e2e/runner/setup_zeta.go
  • e2e/runner/setup_evm.go
  • e2e/runner/evm.go
  • e2e/runner/zeta.go
  • e2e/e2etests/test_message_passing_external_chains_revert_fail.go
  • e2e/e2etests/test_bitcoin_withdraw_invalid_address.go
  • e2e/e2etests/test_zeta_withdraw.go
  • e2e/e2etests/test_zeta_deposit_new_address.go
  • e2e/e2etests/test_update_bytecode_zrc20.go
  • e2e/e2etests/test_pause_zrc20.go
  • e2e/e2etests/test_stress_btc_withdraw.go
  • e2e/e2etests/test_stress_btc_deposit.go
  • e2e/e2etests/test_solana_deposit.go
  • e2e/e2etests/test_rate_limiter.go
  • e2e/e2etests/test_migrate_tss.go
  • e2e/e2etests/test_message_passing_zevm_to_evm_revert_fail.go
  • e2e/e2etests/test_message_passing_zevm_to_evm_revert.go
  • e2e/e2etests/test_message_passing_zevm_to_evm.go
  • e2e/e2etests/test_message_passing_external_chains.go
  • e2e/e2etests/test_message_passing_evm_to_zevm.go
  • e2e/e2etests/test_eth_withdraw_restricted_address.go
  • e2e/e2etests/test_eth_withdraw.go
  • e2e/e2etests/test_eth_deposit_refund.go
  • e2e/e2etests/test_stress_eth_withdraw.go
  • e2e/e2etests/test_eth_deposit_call.go
  • e2e/e2etests/test_eth_deposit.go
  • e2e/e2etests/test_erc20_withdraw.go
  • e2e/e2etests/test_erc20_multiple_withdraws.go
  • e2e/e2etests/test_update_bytecode_connector.go
  • e2e/e2etests/test_eth_deposit_liquidity_cap.go
  • e2e/e2etests/test_zrc20_swap.go
  • e2e/e2etests/test_erc20_deposit_refund.go
  • e2e/e2etests/test_deploy_contract.go
  • e2e/e2etests/test_crosschain_swap.go
  • e2e/e2etests/test_context_upgrade.go
  • e2e/e2etests/test_bitcoin_deposit.go
  • e2e/e2etests/helpers.go
  • e2e/e2etests/test_message_passing_evm_to_zevm_revert_fail.go
  • e2e/e2etests/test_message_passing_evm_to_zevm_revert.go
  • e2e/e2etests/test_migrate_chain_support.go
  • e2e/e2etests/test_erc20_multiple_deposits.go
  • e2e/e2etests/test_erc20_deposit.go
  • e2e/e2etests/test_stress_eth_deposit.go
  • e2e/e2etests/test_donation.go
  • cmd/zetae2e/stress.go
  • cmd/zetae2e/run.go
  • cmd/zetae2e/local/test_runner.go
  • cmd/zetae2e/local/performance.go
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the usage of CctxTimeout and ReceiptTimeout in the codebase.

# Test: Search for the usage of timeout settings. Expect: Consistent usage of CctxTimeout and ReceiptTimeout.
rg --type go -A 5 'CctxTimeout|ReceiptTimeout'

Length of output: 76570

cmd/zetae2e/local/performance.go (1)

78-83: LGTM! Ensure default timeout values are appropriate.

The added lines to set default timeout values improve the robustness of the function.

However, ensure that these default timeout values are appropriate for all scenarios.

Makefile (1)

252-252: LGTM! Ensure correct usage of --profile stress option.

The added line to include the --profile stress option enhances the testing approach.

However, ensure that the --profile stress option is correctly used in the Docker Compose configuration.

Verification successful

Verified: The --profile stress option is correctly used in the Docker Compose configuration.

The stress profile is defined in the contrib/localnet/docker-compose.yml file, confirming the correct usage of the --profile stress option.

  • contrib/localnet/docker-compose.yml
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the usage of `--profile stress` option in the Docker Compose configuration.

# Test: Search for the usage of `--profile stress` option. Expect: Correct usage in the Docker Compose configuration.
rg --type yaml -A 5 '--profile stress'

Length of output: 459


Script:

#!/bin/bash
# Description: Verify the usage of the `stress` profile in Docker Compose configuration files.

# Test: Search for the `profiles` key in YAML files and check if `stress` is listed as a profile.
rg 'profiles:' -A 5 --type yaml

Length of output: 4393

go.mod (1)

367-367: LGTM! Verify the impact of the go-tss update.

The update to github.com/zeta-chain/go-tss from v0.0.0-20240718213003-31767e0c90ba to v0.0.0-20240729195411-9f5ae8189449 is approved.

However, ensure that the new version does not introduce any breaking changes or regressions in the codebase.

Verification successful

The update to go-tss does not introduce breaking changes or regressions.

The changes between the specified versions are related to concurrency and scale testing, as well as CI linting improvements. These updates do not affect the core functionality of the go-tss library.

  • Commit 9f5ae81: "Add concurrency and scale testing"
  • Commit 76ca65a: "ci: add linting"
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the impact of the `go-tss` update.

# Test: Search for the usage of `go-tss` in the codebase. Expect: No breaking changes or regressions.
rg --type go 'go-tss'

Length of output: 1146


Script:

#!/bin/bash
# Description: Verify the impact of the `go-tss` update by inspecting the usage in the identified files.

# List of files using `go-tss`
files=(
  "zetaclient/zetacore/tx_test.go"
  "zetaclient/zetacore/client_vote.go"
  "zetaclient/tss/tss_signer.go"
  "zetaclient/tss/concurrent_keysigns_tracker.go"
  "zetaclient/testutils/mocks/zetacore_client.go"
  "x/observer/types/utils_test.go"
  "x/observer/types/utils.go"
  "zetaclient/chains/interfaces/interfaces.go"
  "x/observer/client/cli/tx_vote_blame.go"
  "cmd/zetaclientd/keygen_tss.go"
)

# Inspect each file for usage of `go-tss`
for file in "${files[@]}"; do
  echo "Inspecting $file"
  rg 'gitlab.com/thorchain/tss/go-tss' "$file"
done

Length of output: 2980


Script:

#!/bin/bash
# Description: Fetch the changelog or commit history of the `go-tss` library to identify any breaking changes or significant updates.

# Fetch the commit history for the `go-tss` library from the old version to the new version.
git log 31767e0c90ba..9f5ae8189449 --oneline --no-merges

Length of output: 265


Script:

#!/bin/bash
# Description: Clone the `go-tss` repository and fetch the commit history between the specified versions.

# Clone the `go-tss` repository
git clone https://github.com/zeta-chain/go-tss.git

# Navigate into the repository directory
cd go-tss

# Fetch the commit history between the specified versions
git log 31767e0c90ba..9f5ae8189449 --oneline --no-merges

Length of output: 288

@lumtis lumtis added this pull request to the merge queue Jul 31, 2024
Merged via the queue into develop with commit 65c60d9 Jul 31, 2024
34 checks passed
@lumtis lumtis deleted the fix-performance-tests branch July 31, 2024 07:07
gartnera added a commit that referenced this pull request Jul 31, 2024
* fix(e2e): increase performance test timeouts

* bump go-tss
gartnera added a commit that referenced this pull request Jul 31, 2024
* fix(ci): use static preparams for zetaclient by default (#2587)

* refactor: move localnet monitoring to profile (#2574)

Co-authored-by: Lucas Bertrand <[email protected]>

* fix(e2e): update performance test parameters and bump go-tss (#2563)

* fix(e2e): increase performance test timeouts

* bump go-tss

* fix(ci): remove job concurrency (#2595)

* ci: run on PRs to all base branches

---------

Co-authored-by: Lucas Bertrand <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking:cli no-changelog Skip changelog CI check PERFORMANCE_TESTS Run make start-e2e-performance-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

e2e: performance tests failing
3 participants