-
Notifications
You must be signed in to change notification settings - Fork 770
[antithesis] Enable reuse of banff e2e test for antithesis testing #3554
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
Conversation
Switched to draft in light of wanting to rebase on #3557 |
cf8a4d4
to
292016a
Compare
Moving to draft and adding a bunch of TODOs prompted by a manually-triggered antithesis run. The error handling of the e2e test is not sufficiently robust, so there are a lot of unhelpful errors that need to be addressed before this PR will be mergeable. |
6e0b41c
to
981663d
Compare
This PR has become stale because it has been open for 30 days with no activity. Adding the |
This PR has become stale because it has been open for 30 days with no activity. Adding the |
Approving my own PR is a treat (because my personal account proposed it), but I'll hold off on merging until someone else approves. |
// execTestWithRecovery ensures assertion-related panics encountered during test execution are recovered | ||
// and that deferred cleanups are always executed before returning. | ||
func execTestWithRecovery(ctx context.Context, log logging.Logger, test Test, wallet *Wallet) { | ||
tc := tests.NewTestContext(log) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Creating a new TestContext here ensures that recovery executes only the cleanups registered to this context.
@@ -85,10 +85,7 @@ func (l LoadGenerator) Run( | |||
default: | |||
} | |||
|
|||
ctx, cancel := context.WithTimeout(ctx, testTimeout) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved to execTestWithRecovery to ensure that cancelation is localized to test execution rather than only being performed on goroutine exit.
Co-authored-by: Copilot <[email protected]> Signed-off-by: maru <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: since the test will now gracefully recover in the case of a context cancellation, we should modify the following:
avalanchego/tests/load2/generator.go
Lines 71 to 76 in 8a1bd2a
childCtx := ctx | |
if loadTimeout != 0 { | |
ctx, cancel := context.WithTimeout(ctx, loadTimeout) | |
childCtx = ctx | |
defer cancel() | |
} |
to just:
if loadTimeout != 0 {
childCtx, cancel := context.WithTimeout(ctx, loadTimeout)
ctx = childCtx
defer cancel()
}
This way, Run()
only deals with just one context which is easier to read + having two separate contexts isn't necessary as a result of this PR.
Done |
I'd like to rename SimpleTestContext to RecoverableTestContext, but would prefer to do that in a follow-on PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple minor comments / questions. Feel free to merge after resolving.
Why this should be merged
Having to duplicate existing test coverage to benefit from execution with antithesis is not ideal. Better to be able to trivially refactor compatible e2e coverage for reuse. This PR refactors the banff e2e test for reuse with antithesis to demonstrate that this is possible.
How this works
APITestFunction
type that a compatible e2e test can implement to allow for execution with both ginkgo and antithesisExecuteAPITest
helper to simplify execution of anAPITestFunction
with ginkgoAPITestFunction
executed byExecuteAPITest
How this was tested
CI
Need to be documented in RELEASES.md?
N/A