-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Parallel Workflow Update Tests [experiment] #8811
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
base: main
Are you sure you want to change the base?
Conversation
84b32e2 to
b2bf3e7
Compare
51af423 to
a371795
Compare
3dc8bc5 to
f4be4bc
Compare
25056f8 to
e5bb0d4
Compare
| TEMPORAL_TEST_OTEL_OUTPUT: ${{ github.workspace }}/.testoutput | ||
| TEMPORAL_OTEL_DEBUG: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed OTEL tracing for now as its design caused increased memory usage (details: the OTEL exporter is scoped to the lifetime of the cluster instead of a namespace which would be more efficient here).
| "busy_timeout": "30000", | ||
| "journal_mode": "wal", | ||
| "synchronous": "normal", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recommended settings I found online.
f8c3c8c to
647f077
Compare
| testSQLiteSchemaDir = "schema/sqlite/v3" // specify if mode is not "memory" | ||
| ) | ||
|
|
||
| // GetTestClusterOption returns test options for the given store type and driver. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A little refactoring to make the persistence setup slightly more ergonomic.
| "go.temporal.io/server/tests/testcore" | ||
| ) | ||
|
|
||
| type testEnv interface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Superseded by testcore.Env.
|
|
||
| runID := mustStartWorkflow(s, tv) | ||
| func TestWorkflowUpdateSuite(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes here were kept to a minimum; helpers and tests were migrated to regular functions. The nested nature of testify suites was preserved, too.
What changed?
Migrated
TestWorkflowUpdateSuiteaway from testify'sSuite; enabling parallel test execution.How it works
testcore.NewEnv(t)to obtain a newTestEnvTestEnvsetst.Parallel()(intentionally not giving a way to opt out!)TestEnvobtains a test cluster fromclusterPool(or blocks if all are in-use right now)TEMPORAL_TEST_SHARED_CLUSTERScontrols size of the poolTEMPORAL_TEST_DEDICATED_CLUSTERScontrols number of dedicated clusterstestify suites
Existing test suites are limited by the same dedicated cluster pool to prevent creating too many clusters.
Database connections
SQLite setup for TestEnv-based func tests (ie only TestWorkflowUpdateSuite so far) has been changed to a file-based approach since that supports much better concurrency due to its WAL that an in-memory SQLite database does not support.
Connection limits for other databases were also raised due to connection errors.
Planned follow-ups
time.Sleeps.Why?
Local speedup: benchmarks show a ~50% speed increase (36.1s → 16.6s) for
TestWorkflowUpdateSuite.Namespace isolation: every test runs in its own namespace. This greatly reduces the risk of (accidental) collisions and also reduces the need to craft unique identifiers such as for task queues and workflow IDs.
Deprecate testify suites: Long-term strategy to remove use of testify suites in functional tests (one reason being their inability to run tests within a suite in parallel).
How did you test it?
Potential Issues
InjectHookor dynamic config overrides. With some more effort the number of these can be reduced.