Refactor Test Environment and Worker initialization to CadenceTestRule #923
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
natemort
requested review from
Groxx,
abhishekj720,
shijiesheng,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
natemort
commented
Oct 22, 2024
| @@ -2464,15 +2288,6 @@ public void mySignal(String value) { | |||
|
|
|||
| @Test | |||
| public void testSignal() { | |||
| // Test getTrace through replay by a local worker. | |||
There was a problem hiding this comment.
This block is a no-op, it creates a new Worker and WorkerFactory but doesn't start it.
natemort
force-pushed
the
workflowtest
branch
from
88cfa0a
to
af67e19
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted filessee 17 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Pull Request Test Coverage Report for Build 2539Details
💛 - Coveralls |
shijiesheng
reviewed
Oct 24, 2024
| } else { | ||
| wc = testEnvironment.newWorkflowClient(clientOptions); | ||
| } | ||
| WorkflowClient wc = cadenceTestRule.createWorkflowClient(clientOptions); |
There was a problem hiding this comment.
nit: maybe add client options to builder?
There was a problem hiding this comment.
Added, good call. This test still needs to create a custom client to test out its interceptor but that's a very useful thing to expose.
natemort
force-pushed
the
workflowtest
branch
4 times, most recently
from
f988ba5
to
7a23c04
Compare
shijiesheng
approved these changes
Oct 24, 2024
…flowTest. WorkflowTest is currently 6,000 lines long and has nearly every test related to end to end client behavior. It provides the rather neat behavior that it supports running against both an instance of Cadence running in Docker and against the test version. It's additionally parameterized to run the entire test suite with or without sticky execution enabled. Due to the complexity in handling both environments, adding yet another test to WorkflowTest has always been the easiest option for developers. To allow for tests to easily be split into other files, extract the core functionality to a Junit test rule that can easily be reused by additional tests. With the exception of testSignalCrossDomainExternalWorkflow and the replay tests that don't use the test environment, all tests have been left in WorkflowTest to be split out later.
natemort
force-pushed
the
workflowtest
branch
from
7a23c04
to
54262a5
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changed?
WorkflowTest is additionally parameterized by whether or not to use sticky execution. I think there are obviously cases where that makes sense but I don't think we need to do that in every test going forward.
Why?
How did you test it?
Potential risks
Release notes
Documentation Changes