Allow setting activity timeouts in testsuite #1187
Open
+64
−2
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
|
|
ohad83
force-pushed
the
feature/testsuite-activity-timeout
branch
from
8d1302f
to
c27397f
Compare
Add 2 method to TestActivityEnvironment: - SetActivityStartToCloseTimeout - SetActivityScheduleToCloseTimeout This will allow changing the default 10 minute timeouts in the current test environments.
ohad83
force-pushed
the
feature/testsuite-activity-timeout
branch
from
c27397f
to
0d10acf
Compare
cretz
reviewed
Aug 7, 2023
Comment on lines
+265
to
+273
| // SetActivityStartToCloseTimeout sets the start to close timeouts of activities in the test environment. | ||
| func (e *TestActivityEnvironment) SetActivityStartToCloseTimeout(timeout time.Duration) { | ||
| e.impl.testWorkflowEnvironmentShared.activityStartToCloseTimeout = timeout | ||
| } | ||
|
|
||
| // SetActivityStartToCloseTimeout sets the schedule to close timeouts of activities in the test environment | ||
| func (e *TestActivityEnvironment) SetActivityScheduleToCloseTimeout(timeout time.Duration) { | ||
| e.impl.testWorkflowEnvironmentShared.activityScheduleToCloseTimeout = timeout | ||
| } |
There was a problem hiding this comment.
Hrmm, longer than 10-minutes using this framework is a bit rough. You might consider a proper integration test with a real server instead.
Regardless, I wonder if we should document on these what the default is and document any relation to SetTestTimeout here and on that method.
There was a problem hiding this comment.
Yeah we should include a comment here on what the default values are
| return nil | ||
| } | ||
|
|
||
| noTimeoutActivity := func(ctx context.Context) error { |
There was a problem hiding this comment.
The low timing resolution/granularity of this test will lead to flakiness on an overloaded CI worker. I think you can just remove the no-timeout assertion part.
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 was changed
Added 2 method to TestActivityEnvironment:
This will allow changing the default 10 minute timeouts in the current test environments.
Why?
There are cases where we would like to test long running activities, or test activity timeouts.
The current test suite did not support that, as it had a hardcoded 10 minute timeout.
Checklist
Closes Allow setting activity timeouts in TestActivityEnvironment #1167
How was this tested:
Set the StartToClose timeout to a specific duration, and test that:
Same test for ScheduleToClose, I did not see a difference between them in case of the testsuite.
Comment added to the new method, I don't believe further updates are needed.