Generic hooks for testing #6938
Conversation
common/errorinjector/noop_impl.go
Outdated
| @@ -0,0 +1,18 @@ | |||
| //go:build !errorinjector | |||
There was a problem hiding this comment.
I'd like to consider flipping this, actually (ie the default being the real impl).
A while ago I experimented with build tags and assertions; and I feel confident that we can add a step for the actual binary build that verifies there's no trace of ErrorInjector to be found, as it was optimized away. That way we don't need to tell every single developer to run their tests with this flag (in their IDE etc.).
There was a problem hiding this comment.
I like that, but I'm still a little concerned since we have so many different binary builds (docker images, goreleaser, internal ones), and then users that build their own binaries...
There was a problem hiding this comment.
FWIW, docker-builds seems to already invoke the server Makefile.
| } | ||
| return lb | ||
| } | ||
|
|
||
| func (lb *defaultLoadBalancer) PickWritePartition( | ||
| taskQueue *tqid.TaskQueue, | ||
| ) *tqid.NormalPartition { | ||
| if n := lb.forceWritePartition(); n >= 0 { | ||
| if n, ok := testhooks.Get[int](lb.testHooks, testhooks.MatchingLBForceWritePartition); ok { |
There was a problem hiding this comment.
did we discuss it?
I really dislike the idea of having this kind of dependency, and having this kind of code in the main code path.
I would suggest to extract functionality into something like "PartitionPicker", and provide different implementations in functional tests.
There was a problem hiding this comment.
We're discussing it now...
-
If you look at what it was doing before, it was abusing dynamic config to hook in here, so this is already a strict improvement (reduces runtime overhead, and makes it clearer that this is a hook for testing).
-
We only want this hook in some tests, only some of the time. So even in tests, most of the time we want the standard behavior. So we'd need a test LoadBalancer that can be set/unset to a mode with fixed behavior, otherwise falls back to the default. I think that's worse:
- First it's just a lot more code.
- Second, that means the mechanism to poke the test implementation is specific to each object, and tests will have to do their own cleanup. This generic mechanism is simpler for test writers, you just
s.InjectHookand it's automatically cleaned up.
-
How do we do that for the other two examples here, forcing async match, and injecting a racing call in the middle of an update-with-start sequence? The alternative implementation method doesn't work there, as far as I can see.
There was a problem hiding this comment.
Another good use will be the case when test needs to continue only after specific code line is reached somewhere deep inside server (like in almost all Update tests, I need to make sure that Update actually reached the server and added to the registry before moving forward). In this case, hook can unblock the channel which is awaited in test code.
| const ( | ||
| MatchingDisableSyncMatch = "matching.disableSyncMatch" | ||
| MatchingLBForceReadPartition = "matching.lbForceReadPartition" | ||
| MatchingLBForceWritePartition = "matching.lbForceWritePartition" | ||
|
|
||
| UpdateWithStartInBetweenLockAndStart = "history.updateWithStartInBetweenLockAndStart" |
There was a problem hiding this comment.
Could we use iota instead? Since the identifiers don't need to be stable; it would save us some typing and effort to keep things consistent.
Counter point could be that we lose the ability to print them. Which we don't do right now; but might in the future.
There was a problem hiding this comment.
Print them as int should be ok too. Anyway, adding Stringer implementation later is not a big deal.
| @@ -410,6 +414,7 @@ func (c *TemporalImpl) startFrontend() { | |||
| fx.Provide(func() persistenceClient.AbstractDataStoreFactory { return c.abstractDataStoreFactory }), | |||
| fx.Provide(func() visibility.VisibilityStoreFactory { return c.visibilityStoreFactory }), | |||
| fx.Provide(func() dynamicconfig.Client { return c.dcClient }), | |||
| fx.Decorate(func() testhooks.TestHooks { return c.testHooks }), | |||
There was a problem hiding this comment.
Side note: most of these fx.Provide should be fx.Decorate.
| } | ||
| return lb | ||
| } | ||
|
|
||
| func (lb *defaultLoadBalancer) PickWritePartition( | ||
| taskQueue *tqid.TaskQueue, | ||
| ) *tqid.NormalPartition { | ||
| if n := lb.forceWritePartition(); n >= 0 { | ||
| if n, ok := testhooks.Get[int](lb.testHooks, testhooks.MatchingLBForceWritePartition); ok { |
There was a problem hiding this comment.
Another good use will be the case when test needs to continue only after specific code line is reached somewhere deep inside server (like in almost all Update tests, I need to make sure that Update actually reached the server and added to the registry before moving forward). In this case, hook can unblock the channel which is awaited in test code.
There was a problem hiding this comment.
I would name this file key.go.
What changed?
Why?
To write integration/functional tests that require tweaking behavior of code under test, without affecting non-test builds.
Potential risks
Hooks are disabled by default, so there should be zero risk to production code, and zero overhead (assuming the Go compiler can do very basic inlining and dead code elimination).
The downside is that functional tests now have to be run with
-tags test_depeverywhere.