test(bridge withdrawer): remove unit tests and add blackbox tests #1232
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…idge-withdrawer-errorscleanup
…idge-withdrawer-errorscleanup
…idge-withdrawer-startup-fixes
…idge-withdrawer-startup-fixes
…idge-withdrawer-startup-fixes
…idge-withdrawer-startup-fixes
…idge-withdrawer-startup-fixes
…idge-withdrawer-startup-fixes
…idge-withdrawer-startup-fixes
…idge-withdrawer-startup-fixes
itamarreif
force-pushed
the
itamarreif/bridge-withdrawer/blackbox
branch
from
6b92b94 to
21b0ba5
Compare
joroshiba
reviewed
Jul 23, 2024
bharath-123
pushed a commit
that referenced
this pull request
Jul 25, 2024
## Summary Adds a check that waits for an empty mempool on startup and then uses the pending nonce instead of latest finalized nonce for transaction submission. ## Background Before these changes we used the latest nonce, which could lead to data races if there are batches awaiting inclusion in the mempool. See #1228 for more context. ## Changes - Add `ASTRIA_BRIDGE_WITHDRAWER_SEQUENCER_GRPC_ENDPOINT` config - Wait for mempool to be empty as part of startup - Submitter connects to the grpc service as part of its startup - Submitter uses pending nonces for batch submission instead of latest nonce ## Testing Removed most testing; reworked in #1232 ## Metrics - Punted: #1272 ## Breaking Changelist - Adds a required env var `ASTRIA_BRIDGE_WITHDRAWER_SEQUENCER_GRPC_ENDPOINT` ## Related Issues closes #1228 --------- Co-authored-by: Richard Janis Goldschmidt <github@aberrat.io>
SuperFluffy
added
testing
bridge-withdrawer
SuperFluffy
approved these changes
Jul 29, 2024
There was a problem hiding this comment.
Looks good, great to have these finally in! Thank you for the lift!
As discussed offline, instead of leaving review comments I have made a few changes to the PR, notably:
- added
ignoreattributes to the tests that require anvil - these now run if--include-ignoredis specified and the normal tests no longer fail. - clippy flagged the
ethereum::Watcher::startupmethod. I replaced it bystartup(self) -> FullyInitialized, which itself includes has a methodFullyInitialized::run. That makes the purpose ofstartupclearer. - I have removed a lot of tracing events from long lived tasks (they make sense when looking at the command line, but will never be observed in grafana)
- I have removed events that notify that a specific line of code is running. This information is redundant on a telemetry platform. If desired, https://docs.rs/tracing-subscriber/0.3.18/tracing_subscriber/fmt/struct.SubscriberBuilder.html#method.with_span_events can be used.
- Instead of the events in 4., I have instrumented the functions.
steezeburger
added a commit
that referenced
this pull request
Jul 29, 2024
* main: test(bridge withdrawer): remove unit tests and add blackbox tests (#1232)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
changes made in #1215 required adding a mock gRPC server. Moving to black box tests was the next step planned so this will replace the invalidated unit tests with valid black box tests.
Background
Brief background on why these changes were made, ie "why?"
Changes
submitter/tests.rsTesting
Related Issues
Link any issues that are related, prefer full github links.
closes #1227