Add CLI command for Reset activity API #732
Conversation
| "--address", s.Address(), | ||
| ) | ||
|
|
||
| s.NoError(res.Err) |
There was a problem hiding this comment.
Didn't mention it on the last review but I should have. Ideally these tests should confirm the invoked action actually occurs.
There was a problem hiding this comment.
We don't return any values for that. No error indicates that the command was processed successfully. I don't want to write more complicated test - it will be a duplicate of existing functional tests. In actual functional tests I'm faling activity few times, then reseting, then calling DescribeWorkflow to confirm that number of attempts becomes 1.
There was a problem hiding this comment.
How do you know this command did anything? Sometimes we do replicate functional tests in every place we do them to ensure end-to-end behavior. When these tests are written in each SDK (and the CLI is basically an SDK), we make sure the intended effect occurs, yes even if that technically is duplicated in each language.
Compare with the other tests in this repo. We technically could just run a command and check error, but we often want more.
There was a problem hiding this comment.
I know that this command reach the server and return no errors. Which means it was a valid call, and it was processed.
How do I know it was correctly processed? Unit tests, functional tests. canary test.
I don't want to replicate those functional tests here, don't want to test server functionality in CLI tests.
There was a problem hiding this comment.
I don't want to replicate those functional tests here, don't want to test server functionality in CLI tests.
Not all of them, just confirm it did something, like the other tests in this repository. It should be really easy to describe the workflow and check pending activities or whatever is needed.
There was a problem hiding this comment.
Unlike for example for updating activity there is no direct expected side effect - the response is empty.
If there will be any kind of error on server side - like "Not Found" - it will return an error.
Even If the response was not empty - one may apply the same argument "what if I just return expected result directly without calling OSS".
So the proper test will be:
- make sure that OSS call happens
- independently make sure that expected effect take place
In this case (just like in my functional test) for the proper testing I need to make activity fail few times, then make DescribeWorkflow call and verify that the number of attempts is greater then 1.
Then block it (otherwise test becomes non-deterministic), then make CLI reset activity call, then make DescribeWorkflow call and verify that the number of attempts is 1. This is literally functional test, just much harder.
There was a problem hiding this comment.
Yes, it'd basically be a functional test that resets after attempt > 1 and confirms it goes back to 1. You don't have to provide full coverage like you do where the implementation is, but at least one end-to-end assertion is worth it. We will surely be writing this same functional test for every SDK we provide this high-level activity reset call on.
There was a problem hiding this comment.
Yes, it'd basically be a functional test
Which already exist.
And that is why there is no need to do that.
There was a problem hiding this comment.
There is a need and we do these kinds of end-to-end tests in other tests in all of our SDKs (which includes the CLI). Right now if the CLI code did nothing, this assertion would pass. Or if it did something but as a developer you may have missed actually making the gRPC call, you'd never know. Even with the next test testing for failure, you don't even know what failure it might hit.
Lets validate at least some side effect occurred.
There was a problem hiding this comment.
Ok, talked offline. I will make a change to make a check that some result (of proper type) is coming from server
|
@ychebotarev Do you think you'll be able to get this merged soon? Would like to cut the next CLI release soonish (once a couple other things land) and would love to take this too if I can. |
| ResetHeartbeats bool | ||
| ServerResponse bool | ||
| }{ | ||
| ServerResponse: resp != nil, |
There was a problem hiding this comment.
Not sure users really need this information and not the biggest fan of adding user-facing information only needed for tests, but this does technically confirm server response so I guess good enough.
Co-authored-by: Chad Retz <[email protected]>
What was changed
Add CLI support for Reset activity API
Why?
Part of the activity API work.