-
Notifications
You must be signed in to change notification settings - Fork 818
[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 |
| // 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.
| 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]>
RodrigoVillar
left a comment
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. |
StephenButtolph
left a comment
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
APITestFunctiontype that a compatible e2e test can implement to allow for execution with both ginkgo and antithesisExecuteAPITesthelper to simplify execution of anAPITestFunctionwith ginkgoAPITestFunctionexecuted byExecuteAPITestHow this was tested
CI
Need to be documented in RELEASES.md?
N/A