panic test hooks if not enabled #8726
Conversation
| // Set is only to be used by test code together with the test_dep build tag. | ||
| func Set[T any](th TestHooks, key Key, val T) func() { | ||
| softassert.Fail(th.logger, "testhooks.Set called without test_dep build tag") | ||
| return func() {} |
There was a problem hiding this comment.
The main change: instead of a compile time error, a runtime error for the very few tests that rely on testhooks is emitted.
Risk and impact of accidentally using it in production code are very low (ie extra logging, which should be caught in testing). But could be mitigated via linter, if needed.
There was a problem hiding this comment.
Thoughts from a few days later; could also consider using panic actually to make the error clearer when a user forgets to set test_dep when testing.
I wouldn't expect this function to be used in production code. Again, linter could be leveraged to make sure.
| // GetCtx gets the value of a test hook from the registry embedded in the | ||
| // context chain. | ||
| // | ||
| // TestHooks should be used sparingly, see comment on TestHooks. | ||
| func GetCtx[T any](ctx context.Context, key Key) (T, bool) { | ||
| hooks := ctx.Value(contextKey{}) | ||
| if hooks, ok := hooks.(TestHooks); ok { | ||
| return Get[T](hooks, key) | ||
| } | ||
| var zero T | ||
| return zero, false | ||
| } | ||
|
|
There was a problem hiding this comment.
It's been a while and these haven't seen any use. Safe to remove IMO.
stephanos
force-pushed
the
softassert-testhooks
branch
4 times, most recently
from
167a1cc to
c4bdc6a
Compare
stephanos
force-pushed
the
softassert-testhooks
branch
from
c4bdc6a to
81df793
Compare
stephanos
force-pushed
the
softassert-testhooks
branch
2 times, most recently
from
51b2d72 to
1ee00ab
Compare
stephanos
force-pushed
the
softassert-testhooks
branch
from
1ee00ab to
7274c8a
Compare
|
|
||
| var Module = fx.Options( | ||
| fx.Provide(func() (_ TestHooks) { return }), | ||
| fx.Provide(NewTestHooks), |
There was a problem hiding this comment.
what was wrong with the interface/concrete switch? an interface takes 16 bytes, a concrete struct{} takes 0. yeah, it may not be noticeable but why change it?
There was a problem hiding this comment.
I was playing around with some options; the size wasn't top of mind for me here. I'll revert that, no problem.
| // concerns into production code. In general you should prefer other ways of writing tests | ||
| // wherever possible, and only use TestHooks sparingly, as a last resort. | ||
| type TestHooks struct { | ||
| data *testHooksData |
There was a problem hiding this comment.
could probably just be data *sync.Map?
2 participants
What changed?
Changed use of
testhooks'sSetto be runtime error instead of compile time error.It still ensures that the default impl (used in
go buildartifacts withouttest_deptag) is the noop impl.Why?
When running a functional test via plain
go testit will fail, as thetest_depbuild flag is required. It is required to prevent accidentally using the default/noop implementation of thetesthookspackage which allows injecting custom behavior into production code from tests.But requiring
test_depto run any test adds friction: CLI invocations need to specify it manually, editor setups need to be adjusted, AI agents need to be instructed. That's a high price to pay for a test helper that is used fairly sparsely.How did you test it?
Functional test without testhook:
Functional test with testhook and build tag:
Functional test with testhook but no build tag: