Skip to content

Restore race-checking tests #1376

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Nov 1, 2024
Merged

Restore race-checking tests #1376

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 15 additions & 4 deletions internal/context_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,11 +70,11 @@ func TestContextChildParentCancelRace(t *testing.T) {
}
env.RegisterWorkflow(wf)
env.ExecuteWorkflow(wf)
assert.True(t, env.IsWorkflowCompleted())
assert.NoError(t, env.GetWorkflowError())
}

func TestContextConcurrentCancelRace(t *testing.T) {
t.Skip("This test is racy and not reliable. It is disabled until we can make it reliable.")
/*
A race condition existed due to concurrently ending goroutines on shutdown (i.e. closing their chan without waiting
on them to finish shutdown), which executed... quite a lot of non-concurrency-safe code in a concurrent way. All
Expand All @@ -83,8 +83,13 @@ func TestContextConcurrentCancelRace(t *testing.T) {
Context cancellation was one identified by a customer, and it's fairly easy to test.
In principle this must be safe to do - contexts are supposed to be concurrency-safe. Even if ours are not actually
safe (for valid reasons), our execution model needs to ensure they *act* like it's safe.

This intentionally avoids `newTestWorkflowEnv` because the dangling goroutines
may shut down after the test finishes -> produce a log, which fails on t.Log races.
Ideally the test suite would stop goroutines too, but that isn't reliable currently either.
This can be changed if we switch to the server's race-safe zaptest wrapper.
*/
env := newTestWorkflowEnv(t)
env := (&WorkflowTestSuite{}).NewTestWorkflowEnvironment()
wf := func(ctx Context) error {
ctx, cancel := WithCancel(ctx)
racyCancel := func(ctx Context) {
Expand All @@ -101,15 +106,20 @@ func TestContextConcurrentCancelRace(t *testing.T) {
}
env.RegisterWorkflow(wf)
env.ExecuteWorkflow(wf)
assert.True(t, env.IsWorkflowCompleted())
assert.NoError(t, env.GetWorkflowError())
}

func TestContextAddChildCancelParentRace(t *testing.T) {
t.Skip("This test is racy and not reliable. It is disabled until we can make it reliable.")
/*
It's apparently also possible to race on adding children while propagating the cancel to children.

This intentionally avoids `newTestWorkflowEnv` because the dangling goroutines
may shut down after the test finishes -> produce a log, which fails on t.Log races.
Ideally the test suite would stop goroutines too, but that isn't reliable currently either.
This can be changed if we switch to the server's race-safe zaptest wrapper.
*/
env := newTestWorkflowEnv(t)
env := (&WorkflowTestSuite{}).NewTestWorkflowEnvironment()
wf := func(ctx Context) error {
ctx, cancel := WithCancel(ctx)
racyCancel := func(ctx Context) {
Expand All @@ -131,6 +141,7 @@ func TestContextAddChildCancelParentRace(t *testing.T) {
}
env.RegisterWorkflow(wf)
env.ExecuteWorkflow(wf)
assert.True(t, env.IsWorkflowCompleted())
assert.NoError(t, env.GetWorkflowError())
}

Expand Down
Loading