Skip to content
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

Facilitate tests debugging in Antithesis #6164

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mprimi
Copy link
Contributor

@mprimi mprimi commented Nov 22, 2024

  1. Introduce dependency on antithesis-sdk-go (default NOOP variant)
  2. Introduce assertions utilities suitable for use in tests
  3. Instrument frequently used test utilities with the new assertions

N.B. unless the enable_antithesis_sdk tag is specified at build time, both the SDK and the assertions wrappers are NOOP, and should have no effect.

This will also facilitate other Antithesis one off experiments since developers no longer have to manually add the SDK to their branch.

Signed-off-by: Your Name [email protected]

@mprimi mprimi requested a review from a team as a code owner November 22, 2024 21:19
@mprimi mprimi force-pushed the marco/run-tests-in-antithesis branch from 2d872c9 to 9d0327f Compare November 25, 2024 18:37
Copy link
Member

@neilalexander neilalexander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit confused about internal/antithesis/fallback_sdk.go here. Antithesis already produce default-no-op builds of their SDK, which contain all of the functionality, but only when built with the enable_antithesis_sdk tag. Wouldn't that be what we want?

@mprimi
Copy link
Contributor Author

mprimi commented Nov 26, 2024

Antithesis already produce default-no-op builds of their SDK

I'll be honest, i didn't notice this release (it's not documented and wasn't brought up in discussion).
I also assumed (wrongly?) you'd not be comfortable bringing in the full SDK.

But if you are, then I agree using the official client is a great option.

I just launched a side-by-side comparison for the test flakes from last week, we should see the official client find the same violated assertions.

Assuming the two are equivalent in terms of results for that experiment, there's still a few minor differences and tradeoffs worth considering.

This fallback implementation gives us more control:

  • Add additional behavior (e.g. stack trace on assert false is useful, even outside Antithesis), and more
  • Control grouping and de-duplication by test (i.e. 2 distinct tests failing on the same require_* or waitFor* will produce 2 example traces. vs. the official client will produce a single trace/example per assertion violation)
  • Control over panic or continue execution on assert violation
  • No surprise breakage when upgrading dependencies

Of course there are upsides of using the official library:

  • Less code to own/maintain, simpler upgrades
  • Devs no longer need to go get antithesis in their branch when running one-off experiments

One more important distinction:

The fallback impl. is specifically for tests, it only exposes Assert(condition) and AssertUnreachable. Both take a *testing.T argument which means we won't end up using it by mistake in server code.

The official client is generic and has a much richer API suitable for the entire project, not just tests.
It exposes things like Reachable, Sometimes, which do not work properly inside a test context**. Devs may use these API count on the results without realizing they do not actually work.

So the richer API is a double-edged sword.

** Antithesis instrumentor skips test code => Assertions in tests code are not catalogued => Antithesis does not know of these assertion ahead of time => Antithesis cannot know it failed to reach a Reachable statement => There will be no failed Reachable property in the report. This can be misleading, as a failed property ins not reported.

@mprimi mprimi force-pushed the marco/run-tests-in-antithesis branch from 9d0327f to bc2bf67 Compare December 6, 2024 17:14
@mprimi mprimi changed the title Add module to facilitate tests debugging in Antithesis Facilitate tests debugging in Antithesis Dec 6, 2024
@mprimi mprimi force-pushed the marco/run-tests-in-antithesis branch from bc2bf67 to 686e951 Compare December 6, 2024 17:16
@mprimi mprimi requested a review from neilalexander December 6, 2024 17:18
@mprimi mprimi force-pushed the marco/run-tests-in-antithesis branch 5 times, most recently from 67e0e09 to 3977b5f Compare December 11, 2024 12:49
@mprimi
Copy link
Contributor Author

mprimi commented Dec 11, 2024

@neilalexander Ready for merge

Copy link
Member

@neilalexander neilalexander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise LGTM, thanks!

server/jetstream_helpers_test.go Outdated Show resolved Hide resolved
@mprimi mprimi force-pushed the marco/run-tests-in-antithesis branch from 3977b5f to 5855303 Compare December 12, 2024 12:55
Copy link
Member

@neilalexander neilalexander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mprimi mprimi force-pushed the marco/run-tests-in-antithesis branch 2 times, most recently from 0eac63c to b1bf222 Compare December 18, 2024 07:23
go.mod Outdated Show resolved Hide resolved
Overview:

1. Introduce dependency on antithesis-sdk-go (default NOOP variant)
2. Introduce assertions utilities suitable for use in tests
3. Instrument frequently used test utilities with the new assertions

N.B. unless the `enable_antithesis_sdk` tag is specified at build time, both the SDK and the assertions wrappers are NOOP, and should have no effect.

This will also facilitate other Antithesis one off experiments since developers no longer have to manually add the SDK to their branch.
@mprimi mprimi force-pushed the marco/run-tests-in-antithesis branch from b1bf222 to 8879c01 Compare January 2, 2025 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants