Conversation
stephanos
force-pushed
the
injecthook-ns
branch
from
9820f1b to
cb371ab
Compare
|
|
||
| // simulate a race condition | ||
| s.InjectHook(testhooks.UpdateWithStartInBetweenLockAndStart, func() { | ||
| s.InjectHook(testhooks.NewHook(testhooks.UpdateWithStartInBetweenLockAndStart, func() { |
There was a problem hiding this comment.
Using testhooks.NewHook since InjectHook is method and Go doesn't allow type parameters there. This way it's a bit more verbose but preserves type checking.
There was a problem hiding this comment.
oh, hmm. they are relaxing this constraint, but I guess it'll be in go 1.27
There was a problem hiding this comment.
oh that'd be really neat
stephanos
force-pushed
the
injecthook-ns
branch
12 times, most recently
from
6dd53b6 to
1e27fcf
Compare
stephanos
force-pushed
the
injecthook-ns
branch
from
1e27fcf to
7b6db0b
Compare
stephanos
force-pushed
the
injecthook-ns
branch
8 times, most recently
from
aa65611 to
0d2edcd
Compare
| if th.data == nil { | ||
| if th.lock == nil { | ||
| // This means TestHooks wasn't created via NewTestHooks. Ignore. | ||
| return zero, false |
There was a problem hiding this comment.
This happened in service/worker/workerdeployment/workflow_test.go.
stephanos
force-pushed
the
injecthook-ns
branch
from
0d2edcd to
4b60f9b
Compare
| // Test hook keys with their return type and scope. | ||
| // Try to avoid global scope as it requires a dedicated test cluster. | ||
| var ( | ||
| MatchingDisableSyncMatch = newKey[bool, namespace.ID]() |
There was a problem hiding this comment.
It's unfortunate to do a bunch of stuff at static init time just for tests (of course much better than doing it at runtime). What if we did something like define newKey differently in test/prod and have the prod version just return a struct{}{}?
common/testing/testhooks/key.go
Outdated
| return h.key.scope | ||
| } | ||
|
|
||
| func (h hook[T, S]) Apply(th TestHooks, scope any) func() { |
There was a problem hiding this comment.
why the new interface here? we can't roll this check into Set?
| type TestHooks struct { | ||
| data *sync.Map | ||
| lock *sync.Mutex | ||
| hooks map[keyID]map[any]any // id -> scope -> value |
There was a problem hiding this comment.
did you consider a single level map with a struct key? (then it could still be a sync.Map which might simplify the code?)
| scopes := th.hooks[key.id] | ||
| if scopes == nil { | ||
| scopes = make(map[any]any) | ||
| th.hooks[key.id] = scopes | ||
| } |
There was a problem hiding this comment.
| scopes := th.hooks[key.id] | |
| if scopes == nil { | |
| scopes = make(map[any]any) | |
| th.hooks[key.id] = scopes | |
| } | |
| scopes := util.GetOrSetMap(th.hooks, key.id) |
|
|
||
| // simulate a race condition | ||
| s.InjectHook(testhooks.UpdateWithStartInBetweenLockAndStart, func() { | ||
| s.InjectHook(testhooks.NewHook(testhooks.UpdateWithStartInBetweenLockAndStart, func() { |
There was a problem hiding this comment.
oh, hmm. they are relaxing this constraint, but I guess it'll be in go 1.27
# Conflicts: # tests/testcore/test_env.go
|
👍 Thank you for the feedback; it's shorter and cleaner now I think. |
| } | ||
| } | ||
|
|
||
| type Key[T any, S any] struct { |
There was a problem hiding this comment.
can you move this into test_impl/noop_impl and have it be defined as struct{} in noop_impl?
yeah, it works fine. I had to move Hook+NewHook also, and defined it as struct{} also. and defined Hook.Scope/Apply to do the same panic as Set()
common/testing/testhooks/hooks.go
Outdated
|
|
||
| type Key[T any, S any] struct { | ||
| id keyID | ||
| scope Scope |
There was a problem hiding this comment.
I got a little confused at first between the difference between Key and hookKey because they both have a "scope" but the meaning of the word is different. maybe:
| scope Scope | |
| scopeType ScopeType |
What changed?
Make test hooks scopable to namespace.
Why?
Right now using
InjectHookrequires a dedicated cluster, but most hooks can be scoped to a namespace and allow using a shared cluster. This reduces resource consumption and increases test concurrency.How did you test it?