Conversation
stephanos
force-pushed
the
batch-rate-limit
branch
from
2b31be5 to
8c9470c
Compare
|
|
stephanos
force-pushed
the
batch-rate-limit
branch
3 times, most recently
from
35423f1 to
ffd2c01
Compare
| func (s *SharedServerSuite) TestWorkflow_Terminate_BatchWorkflowSuccess_Ratelimit() { | ||
| events, _ := s.testTerminateBatchWorkflow(2, 1, false) | ||
| delay := events[1].EventTime.AsTime().Sub(events[0].EventTime.AsTime()).Abs() | ||
| s.InDelta(delay, 1*time.Second, float64(100*time.Millisecond)) |
There was a problem hiding this comment.
In my tests it hasn't always been 1s or greater, so I added a delta here.
There was a problem hiding this comment.
This is the kind of test that grows flaky with Go runner slowness I've found. Expecting this margin of error is a bit rough. I think for this you may only need to test that it's set on the outbound request (e.g. w/ gRPC interceptor maybe), you don't need to test server behavior for something like this IMO (that's for the server to test).
| // Send batch terminate with a "y" for non-json or "--yes" for json | ||
| args := []string{ | ||
| "workflow", "terminate", | ||
| "--rps", fmt.Sprint(rps), |
There was a problem hiding this comment.
This introduces a delay in the test suite of up to 1s. Not sure how to avoid that while still testing the Server end-to-end.
There was a problem hiding this comment.
We may have to accept some things cannot reasonably tested in CLI without slowing all tests down.
cretz
left a comment
cretz
left a comment
There was a problem hiding this comment.
LGTM. One minor suggestion and concerns about using time-based expectations in tests
| * `--build-id` (string) - Only used if type is BuildId. Reset the first workflow task processed by this build id. Note that by default, this reset is allowed to be to a prior run in a chain of continue-as-new. | ||
| * `--query`, `-q` (string) - Start a batch reset to operate on Workflow Executions with given List Filter. | ||
| * `--yes`, `-y` (bool) - Confirm prompt to perform batch. Only allowed if query is present. | ||
| * `--rps` (float) - Limit batch's requests per second. Only allowed if query is present. |
There was a problem hiding this comment.
Can you update the top of this file to include this new data type in the bullet that lists the data types supported? It's our admittedly-informal "spec".
| // Send batch terminate with a "y" for non-json or "--yes" for json | ||
| args := []string{ | ||
| "workflow", "terminate", | ||
| "--rps", fmt.Sprint(rps), |
There was a problem hiding this comment.
We may have to accept some things cannot reasonably tested in CLI without slowing all tests down.
| func (s *SharedServerSuite) TestWorkflow_Terminate_BatchWorkflowSuccess_Ratelimit() { | ||
| events, _ := s.testTerminateBatchWorkflow(2, 1, false) | ||
| delay := events[1].EventTime.AsTime().Sub(events[0].EventTime.AsTime()).Abs() | ||
| s.InDelta(delay, 1*time.Second, float64(100*time.Millisecond)) |
There was a problem hiding this comment.
This is the kind of test that grows flaky with Go runner slowness I've found. Expecting this margin of error is a bit rough. I think for this you may only need to test that it's set on the outbound request (e.g. w/ gRPC interceptor maybe), you don't need to test server behavior for something like this IMO (that's for the server to test).
stephanos
force-pushed
the
batch-rate-limit
branch
from
f7e612c to
096a1b7
Compare
stephanos
force-pushed
the
batch-rate-limit
branch
from
096a1b7 to
422c835
Compare
|
Let's make sure this is in the next CLI release. |
|
@stephanos I think the cli-rewrite branch is dead, main becoming the new target branch for the last two weeks, you may want to merge in main instead... @cretz Can you confirm? |
5 participants
What was changed
Added support for passing
--rpsflag to batch operations.ℹ️ FYI I've only added a single test for TerminateWorkflow, even though there are 3 batch APIs. Reason being that the API is the same and the only difference is the "operation". Let me know if I should add it to the others as well.
❓ Note sure why I'm seeing the CLA request on my PR.
Checklist
Closes
How was this tested: Added new tests.
Any docs updates needed?