Expand e2e tests to cover the true end-to-end flow #267
Conversation
joshklop
force-pushed
the
joshklop.revamp-e2e
branch
from
b925fd0
to
8bf8ec7
Compare
|
Caution Review failedThe pull request is closed. WalkthroughThe changes in this pull request involve significant modifications to the end-to-end (e2e) testing framework and related components. Key alterations include the removal of specific steps from the GitHub Actions workflow, updates to the Makefile, and the introduction of new functionality in the e2e testing files. Several files related to the e2e stack and its configuration have been deleted, while new methods and structures have been added to enhance the MonomerClient and testing capabilities. Additionally, modifications to the Changes
Possibly related PRs
Suggested reviewers
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
joshklop
force-pushed
the
joshklop.revamp-e2e
branch
from
8bf8ec7
to
21ad610
Compare
joshklop
force-pushed
the
joshklop.revamp-e2e
branch
from
21ad610
to
e528073
Compare
|
We might need to commit a |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (17)
README.md (2)
15-15: Improve clarity by separating version and tool information.The current line combines Go version information with buf usage information, which could be clearer if separated.
-We use Go 1.22. We use [`buf`](https://buf.build/) to manage protobufs. +# Development Requirements + +- Go 1.22 or higher +- [`buf`](https://buf.build/) for protobuf management
15-15: Resolve inconsistent Go version specifications.The Go version requirement is mentioned twice with different specifications:
- Line 15: "Go 1.22"
- Line 19: "go 1.22 or higher"
This could lead to confusion about the minimum required version.
Consider standardizing to one clear specification, preferably in the prerequisites section only.
Also applies to: 19-19
cmd/monogen/testapp/setup-helper.sh (2)
9-11: Enhance version comparison robustnessThe current version comparison using string comparison (
>) might not handle all Go version formats correctly. Consider using a more robust version comparison approach.-if [[ $goVersion > go1.23.0 || $goVersion == go1.23.0 ]]; then +# Extract version numbers and compare numerically +version=$(echo $goVersion | sed 's/go//') +if [[ $(echo -e "$version\n1.23.0" | sort -V | tail -n1) == "$version" ]]; then
9-13: Enhance documentation for GOFLAGS modificationWhile the comment mentions the dependency issue, it would be helpful to add more context about:
- Why
-checklinkname=0is specifically needed- Potential implications of disabling this check
- Whether this is a temporary workaround or permanent solution
# Build the app. -# Because we transitively depend on github.com/fjl/memsize, we need to disable checklinkname in go1.23.0 and higher. +# Workaround for Go 1.23.0+ compatibility issue: +# - The github.com/fjl/memsize dependency requires disabling checklinkname +# - This flag disables link name verification during build +# - This is necessary due to changes in Go 1.23.0's linking behavior +# - TODO: Monitor for upstream fixes in the dependencye2e/clients.go (1)
61-63: LGTM! Consider adding documentation.The implementation correctly delegates to the underlying ethclient and follows the established pattern in the codebase. However, for consistency with other methods like
GetProof, consider adding documentation that describes the method's purpose and return values.+// ChainID returns the chain ID of the connected Ethereum network. func (m *MonomerClient) ChainID(ctx context.Context) (*big.Int, error) { return m.ethclient.ChainID(ctx) }integrations/integrations.go (2)
241-249: Consider documenting the rationale for removing readFromFileOrGetDefault.The TODO comment suggests removing this abstraction, but it would be helpful to document why it's considered "clunky" and what alternative approach you're considering. This will help future maintainers understand the motivation for the potential change.
Review the type conversion approach.
The conversion from
state.Dumptoopgenesis.ForgeDumpand back again seems circuitous. Consider if this could be simplified to avoid the double conversion.- got, err := opdevnet.DefaultL1Allocs() - if err != nil { - return nil, err - } - gotForge := opgenesis.ForgeDump(*got) - return &gotForge, nil + return opdevnet.DefaultL1Allocs()
253-253: Add a comment explaining the type conversion.The conversion between
ForgeDumpandstate.Dumptypes might not be immediately obvious to other developers. Consider adding a comment explaining why this conversion is necessary.+ // Convert ForgeDump back to state.Dump as required by BuildL1Config l1Allocs := state.Dump(*l1AllocsForge)e2e/e2e.go (4)
62-63: Mismatch between comment and code regarding environment variablesThe comment mentions adding the
"GOFLAGS='-gcflags=all=-N -l'"environment variable to disable optimizations and make debugging easier, but the code actually adds"e2eapp_HOME="+outDirto the environment variables. Please update the comment or the code to reflect the intended action.
41-44: Error message clarity inmonomerPathresolutionThe error message in case of failure to resolve
monomerPathcould be more descriptive. Consider providing additional context to help troubleshoot the issue.Apply this diff to improve the error message:
if err != nil { - return fmt.Errorf("absolute path to local monomer directory: %v", err) + return fmt.Errorf("failed to determine absolute path to local monomer directory: %w", err) }
68-69: Redundant//nolint:gosecdirectiveThe
//nolint:gosecdirective is used to suppress security warnings about potentially tainted command arguments. Ensure that suppressing this warning is necessary and justified. If the command arguments are constructed safely, consider removing the directive to maintain codebase cleanliness.
85-90: Proper error handling in deferred functionIn the deferred function
env.DeferErr("wait for app", func() error { ... }), any error returned fromappCmd.Wait()that is not a context cancellation is returned as-is. Consider wrapping the error with additional context to aid in debugging.Apply this diff to wrap the error:
if errors.Is(err, context.Canceled) { return nil } -return err +return fmt.Errorf("app command exited with error: %w", err)e2e/e2e_test.go (2)
58-60: Simplify directory creation logicThe error handling for creating the
artifactsdirectory can be simplified usingos.MkdirAll, which creates a directory along with any necessary parents and doesn't return an error if the directory already exists.Apply this diff to streamline the code:
-if err := os.Mkdir(artifactsDir, 0o755); !errors.Is(err, os.ErrExist) { - require.NoError(t, err) -} +require.NoError(t, os.MkdirAll(artifactsDir, 0o755))
142-149: Replacetime.Sleepwith context-based waitingUsing
time.Sleepfor waiting can be inefficient and unresponsive to context cancellations. It's better to implement a polling mechanism that waits for the required condition while respecting the context's cancellation.Consider implementing a wait function that checks for block confirmations with a timeout:
WaitL1: func(numBlocks int) error { ctx, cancel := context.WithTimeout(context.Background(), time.Duration(maxWaitTime)) defer cancel() for { // Replace with actual block height fetching logic currentBlockHeight := getCurrentL1BlockHeight() if currentBlockHeight >= targetBlockHeight { return nil } select { case <-ctx.Done(): return ctx.Err() default: time.Sleep(pollInterval) } } },e2e/stack_test.go (4)
138-142: Clean up commented code inDepositTransactionparametersThere is commented code left in the parameters of
DepositTransaction, which can affect readability. Consider removing or updating the commented code to improve clarity.Apply this diff to clean up:
- 100_000, // 0l2GasLimit, // l2GasLimit, + 100_000, // l2GasLimit,
286-286: Address the TODO comment regarding wait durationThe
TODOcomment questions why it's necessary to wait for two L1 blocks. Investigate and document the reason for this delay to improve code clarity and test reliability.Would you like assistance in determining the appropriate wait time and updating the code accordingly? I can help analyze the process and propose a solution.
320-330: Handle errors instead of panicking inconvertPrivKeyUsing
panicin theconvertPrivKeyfunction can cause the entire test suite to halt on failure, potentially making debugging difficult. Consider returning an error instead to allow for graceful error handling.Here is how you might modify the function:
-func convertPrivKey(ecdsaPrivKey *ecdsa.PrivateKey) *secp256k1.PrivateKey { +func convertPrivKey(ecdsaPrivKey *ecdsa.PrivateKey) (*secp256k1.PrivateKey, error) { privKeyBytes := ecdsaPrivKey.D.Bytes() var key secp256k1.ModNScalar - if len(privKeyBytes) > 32 || key.SetByteSlice(privKeyBytes) { - panic("overflow") + if len(privKeyBytes) > 32 || key.SetByteSlice(privKeyBytes) { + return nil, errors.New("overflow") } - if key.IsZero() { - panic("private keys must not be 0") + if key.IsZero() { + return nil, errors.New("private keys must not be 0") } - return secp256k1.NewPrivateKey(&key) + return secp256k1.NewPrivateKey(&key), nil }Then, update the calling code to handle the error appropriately.
366-368: Consider parameterizing fee amount and gas limitThe fee amount and gas limit are currently hard-coded in the transaction. Parameterizing these values or using constants can improve flexibility and maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
cmd/monogen/testapp.zipis excluded by!**/*.zipgo.workis excluded by!**/*.workgo.work.sumis excluded by!**/*.sum
📒 Files selected for processing (15)
.github/workflows/test.yml(0 hunks).gitignore(0 hunks)Makefile(1 hunks)README.md(1 hunks)cmd/monogen/testapp/setup-helper.sh(1 hunks)e2e/clients.go(1 hunks)e2e/e2e.go(1 hunks)e2e/e2e_test.go(1 hunks)e2e/gethdevnet.go(0 hunks)e2e/op_stack.go(0 hunks)e2e/optimism(0 hunks)e2e/selective_listener.go(0 hunks)e2e/stack.go(0 hunks)e2e/stack_test.go(14 hunks)integrations/integrations.go(2 hunks)
💤 Files with no reviewable changes (7)
- .github/workflows/test.yml
- .gitignore
- e2e/gethdevnet.go
- e2e/op_stack.go
- e2e/optimism
- e2e/selective_listener.go
- e2e/stack.go
🔇 Additional comments (9)
e2e/clients.go (1)
61-63: Verify test coverage and usage in E2E tests.
Given this is part of an E2E testing enhancement and some stack management files have been removed, let's verify the test coverage and usage of this new method.
✅ Verification successful
ChainID method implementation and usage verified successfully
The verification results show that:
- The
ChainIDmethod is properly used in E2E tests for both L1 and L2 chain identification - It's used in critical test scenarios:
- L1 chain validation and signer setup
- L2 chain validation for withdrawal transactions
- Transaction building with correct chain IDs
- The removal of previous ChainID usages from deleted files (shown in git history) has been properly replaced with this implementation in the new E2E testing structure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage and usage of ChainID method
# Test 1: Check for test coverage
echo "Checking for test coverage of ChainID method..."
rg -l "ChainID.*ctx.*context\.Context.*\*big\.Int.*error" e2e/*_test.go
# Test 2: Check for actual usage in tests
echo "Checking for usage of ChainID in tests..."
rg "ChainID\(" e2e/*_test.go -A 3
# Test 3: Check if this replaces functionality from removed files
echo "Checking if ChainID is used in place of removed functionality..."
rg "ChainID" e2e/e2e.go -A 3
Length of output: 1124
Script:
#!/bin/bash
# Let's verify the broader impact by checking:
# 1. Interface definition and implementation
# 2. Usage in the entire codebase
# 3. Changes in the PR that might affect this
# Check interface and implementation
echo "Checking interface definition..."
rg "interface.*\{.*ChainID.*\}" -A 5
# Check all ChainID usages in e2e directory
echo "Checking all ChainID usages in e2e..."
rg "ChainID" e2e/ -A 3
# Check if there are any removed files that contained ChainID
echo "Checking git history for removed files with ChainID..."
git log -p --diff-filter=D | rg "ChainID" -B 2 -A 2
Length of output: 3669
Makefile (2)
22-22: Consider documenting the test environment setup requirements.
With the removal of explicit configuration paths and setup commands:
- The PR comments mention potential CI issues with
go.work - The simplified command assumes a standardized environment
Please consider:
- Documenting the required test environment setup in the README
- Adding CI-specific setup instructions
- Addressing the
go.workrequirement mentioned in the PR comments
#!/bin/bash
# Description: Check for environment setup documentation
# Expected: Find setup instructions in README or documentation
# Check if README contains e2e test setup instructions
echo "Checking README for e2e test setup instructions:"
rg -i "e2e|test setup|environment" README.md
# Look for any existing go.work files
echo "Checking for go.work files:"
fd -g "go.work*"22-22: LGTM on the simplified e2e test command.
The simplified command makes the testing process more straightforward. However, let's verify that all necessary test configurations are properly handled within the test files themselves.
✅ Verification successful
Based on the provided context, I can now generate a final response. The e2e test files and their contents show that the test configuration is properly handled within the test files themselves, and the configuration paths are no longer needed as the tests are self-contained.
Simplified e2e test command is properly implemented
The removal of configuration paths from the Makefile is justified as:
- Test configuration is now handled within the e2e test files
- The e2e tests are comprehensive, covering ETH deposits/withdrawals, ERC-20 operations, and system stability
- Test setup is managed through the test code itself, making it more maintainable and self-contained
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for test configuration handling in e2e tests
# Expected: Find configuration setup within test files
# Look for configuration setup in e2e test files
echo "Checking for configuration setup in e2e tests:"
rg -l "func TestE2E|func Setup|func Init" ./e2e/
# Check for any remaining references to old configuration paths
echo "Checking for any missed configuration path references:"
rg -l "E2E_STATE_SETUP|E2E_CONFIG_SETUP"
Length of output: 475
Script:
#!/bin/bash
# Let's check the actual e2e test files and their structure
# Find all e2e test files
echo "E2E test files:"
fd -e go . e2e/
# Check the content of e2e test files for setup/configuration
echo -e "\nTest file contents:"
fd -e go . e2e/ -x cat {}
# Check if there are any environment variables used in e2e tests
echo -e "\nEnvironment variables in e2e tests:"
rg "os.Getenv|flag." e2e/
# Check Makefile history for context of the change
echo -e "\nMakefile changes context:"
git log -p -- Makefile | grep -A 5 -B 5 "e2e"
Length of output: 42352
integrations/integrations.go (1)
281-281: Verify pointer handling in BuildL1Config.
The change to pass l1Allocs by reference is good for performance, but we should ensure that BuildL1Config properly handles the pointer and doesn't modify the original data unexpectedly.
✅ Verification successful
Pointer usage in BuildL1Config is safe
The code inspection reveals that l1Allocs is only used as input to BuildL1DeveloperGenesis and the only modification to it occurs intentionally before the BuildL1Config call to set up the L1 user account balance. This is a safe and expected usage pattern for the pointer parameter.
- The modification in
integrations.gois intentional: setting up initial account balance BuildL1Configpasses the pointer toBuildL1DeveloperGenesiswhich uses it to construct the genesis state- No unexpected modifications to the original data are found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for BuildL1Config implementation to verify pointer handling
ast-grep --pattern 'func BuildL1Config($_, $_, $l1Allocs *state.Dump, $_, $_) {
$$$
}'
Length of output: 90
Script:
#!/bin/bash
# Let's try a broader search for BuildL1Config function
rg "func BuildL1Config" -A 10
# Also search for any usage of l1Allocs to understand the context
rg "l1Allocs" -A 3
Length of output: 3233
Script:
#!/bin/bash
# Let's check the BuildL1DeveloperGenesis function to see how it handles the pointer
rg "func BuildL1DeveloperGenesis" -A 10
# Also check for any modifications to l1Allocs in the codebase
rg "l1Allocs\." -A 3
Length of output: 308
e2e/e2e.go (3)
41-55: Handling of relative paths in command execution
Ensure that all relative paths used in command executions are accurately resolved, especially when the working directory may vary. Consider using absolute paths or setting the Dir field of exec.Cmd to avoid any potential issues with path resolution.
49-55: Potential issue with command argument construction
When constructing command arguments for monogenCmd, ensure that the arguments are correctly formatted and safe. Although you have a //nolint:gosec directive, it's important to validate that monomerPath, appDirPath, and other variables are sanitized and not influenced by external input.
60-66: Ensure proper execution of setup-helper.sh script
When executing setup-helper.sh, ensure that the script is executable and has the correct shebang line (#!) at the top. Alternatively, specify the shell interpreter explicitly to prevent execution errors.
Run the following script to check the script's executability and shebang line:
✅ Verification successful
setup-helper.sh script is properly configured and executable
The verification shows that:
- The script has proper execute permissions (
-rwxr-xr-x) - Contains the correct shebang line (
#!/usr/bin/env bash) - Is a valid shell script with proper error handling (
set -e)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that 'setup-helper.sh' is executable and has a shebang.
# Test: Check if 'setup-helper.sh' is executable.
# Expect: File has execute permissions.
if [ ! -x "$(filepath.Join(appDirPath, 'setup-helper.sh'))" ]; then
echo "'setup-helper.sh' is not executable."
fi
# Test: Check for shebang line.
# Expect: File starts with '#!'.
head -n 1 "$(filepath.Join(appDirPath, 'setup-helper.sh'))" | grep '^#!' || echo "Missing shebang in 'setup-helper.sh'."
Length of output: 214
Script:
#!/bin/bash
# Let's first find the setup-helper.sh file
fd "setup-helper.sh"
# Then check its content and permissions
for file in $(fd "setup-helper.sh"); do
echo "=== File: $file ==="
echo "Permissions:"
ls -l "$file"
echo -e "\nFirst line (shebang check):"
head -n 1 "$file"
echo -e "\nFull content:"
cat "$file"
done
Length of output: 2450
e2e/stack_test.go (2)
111-114: LGTM
The code correctly derives the Ethereum and Cosmos addresses from the user's public key.
332-394: LGTM
The buildTx function correctly constructs and signs the transaction using the provided parameters. Error handling is appropriate, and the implementation aligns with best practices.
joshklop
force-pushed
the
joshklop.revamp-e2e
branch
from
62dd2ba
to
b11e824
Compare
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Chores