Conversation
… Actions workflow.
Summary of ChangesHello @chatton, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new end-to-end test for the 'spamoor' transaction generation tool, ensuring its proper integration and functionality within the EVM ecosystem. To support this new test and provide more flexibility for future E2E scenarios, the common EVM test setup function has been enhanced to expose the underlying Reth full node instance. This change required minor adjustments across several existing test files to accommodate the updated function signature. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
|
Claude finished @chatton's task in 2m 9s —— View job Code Review
Good addition overall — the test is well-structured, has meaningful assertions (addressing Gemini's TODO note), and uses proper cleanup via
|
| Severity | Issue |
|---|---|
| Minor | context.Background() on line 44 should use ctx from t.Context() |
| Minor | sumGauge is dead code — use or remove |
| Minor | time.Sleep on lines 115/123 not context-aware |
| Minor | requireHTTP should accept a context |
| Nit | Update doc comment for setupCommonEVMTest return values |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new end-to-end smoke test for 'Spamoor' and refactors the common EVM test setup function setupCommonEVMTest to return the reth.Node instance. The changes also include updating go.mod with new dependencies for the smoke test.
My review has identified a few areas for improvement:
- Inconsistent Formatting: Several files have indentation changed from tabs to spaces, which is inconsistent with the project's style. Running
gofmtshould resolve this. - Build Configuration: The
go.modfile includes a localreplacedirective which will break builds for others and in CI. This should be removed. - Test Completeness: The new
TestSpamoorSmokeis a good addition but currently lacks assertions to verify its behavior, as noted by aTODO. - Test Context: The new test uses
context.Background()wheret.Context()would be more appropriate for better test lifecycle management.
Overall, the changes are a good step towards adding more comprehensive testing, but the identified issues should be addressed before merging.
|
|
||
| // Use common setup (no full node needed initially) | ||
| jwtSecret, _, genesisHash, endpoints := setupCommonEVMTest(t, sut, false) | ||
| jwtSecret, _, genesisHash, endpoints, _ := setupCommonEVMTest(t, sut, false) |
There was a problem hiding this comment.
The indentation on this line has been changed from a tab to spaces, which is inconsistent with the project's coding style. Please use tabs for indentation or run gofmt to correct it.
| jwtSecret, _, genesisHash, endpoints, _ := setupCommonEVMTest(t, sut, false) | |
| jwtSecret, _, genesisHash, endpoints, _ := setupCommonEVMTest(t, sut, false) |
|
|
||
| // Use common setup with full node support | ||
| jwtSecret, fullNodeJwtSecret, genesisHash, endpoints := setupCommonEVMTest(t, sut, true) | ||
| jwtSecret, fullNodeJwtSecret, genesisHash, endpoints, _ := setupCommonEVMTest(t, sut, true) |
There was a problem hiding this comment.
The indentation on this line has been changed from a tab to spaces, which is inconsistent with the project's coding style. Please use tabs for indentation or run gofmt to correct it.
| jwtSecret, fullNodeJwtSecret, genesisHash, endpoints, _ := setupCommonEVMTest(t, sut, true) | |
| jwtSecret, fullNodeJwtSecret, genesisHash, endpoints, _ := setupCommonEVMTest(t, sut, true) |
test/e2e/evm_spamoor_smoke_test.go
Outdated
| time.Sleep(2 * time.Second) | ||
| printCollectedTraceReport(t, collector) | ||
|
|
||
| // TODO: test should pass / fail based on results |
There was a problem hiding this comment.
This TODO indicates the test is incomplete. A smoke test should have some basic assertions to be effective. Consider adding assertions to verify the outcome, for example, checking that some transactions were sent and that there were no failures.
require.Greater(t, sent, float64(0), "at least one transaction should have been sent")
require.Zero(t, fail, "no transactions should have failed")
test/e2e/failover_e2e_test.go
Outdated
|
|
||
| // Get JWT secrets and setup common components first | ||
| jwtSecret, fullNodeJwtSecret, genesisHash, testEndpoints := setupCommonEVMTest(t, sut, true) | ||
| jwtSecret, fullNodeJwtSecret, genesisHash, testEndpoints, _ := setupCommonEVMTest(t, sut, true) |
There was a problem hiding this comment.
The indentation on this line has been changed from a tab to spaces, which is inconsistent with the project's coding style. Please use tabs for indentation or run gofmt to correct it.
| jwtSecret, fullNodeJwtSecret, genesisHash, testEndpoints, _ := setupCommonEVMTest(t, sut, true) | |
| jwtSecret, fullNodeJwtSecret, genesisHash, testEndpoints, _ := setupCommonEVMTest(t, sut, true) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3091 +/- ##
==========================================
- Coverage 61.01% 61.00% -0.02%
==========================================
Files 113 113
Lines 11544 11544
==========================================
- Hits 7044 7042 -2
- Misses 3701 3703 +2
Partials 799 799
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
chatton
changed the title
| // | ||
| // Returns: jwtSecret, fullNodeJwtSecret (empty if needsFullNode=false), genesisHash | ||
| func setupCommonEVMTest(t testing.TB, sut *SystemUnderTest, needsFullNode bool, _ ...string) (string, string, string, *TestEndpoints) { | ||
| func setupCommonEVMTest(t testing.TB, sut *SystemUnderTest, needsFullNode bool) (string, string, string, *TestEndpoints, *reth.Node) { |
There was a problem hiding this comment.
I needed to return the reth node in order to get access to the DockerClient / Network so the spamoor instance can be deployed in the same network.
I think we can refactor this function in a follow up as it is quite bloated at this stage.
I also removed the unused string varargs
3 participants
Overview
This PR adds an E2E test that deploys Spamoor, and kicks off a few spammers using the API.
The test itself is quite basic, but can be used as a template for other tests to deploy more elaborate scenarios.