Skip to content
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.

Sign up for GitHub

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

Add CLI command for Reset activity API #732

Merged
merged 4 commits into from
Jan 8, 2025
Merged

Add CLI command for Reset activity API #732

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0353241
Add CLI command for Reset activity API
ychebotarev Dec 19, 2024
9920b8b
Add more test checks
ychebotarev Jan 7, 2025
9779186
Update temporalcli/commands.activity.go
ychebotarev Jan 8, 2025
e305d5c
Merge branch 'next-server' into y_reset
ychebotarev Jan 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion temporalcli/commands.activity.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,10 +271,22 @@ func (c *TemporalActivityResetCommand) run(cctx *CommandContext, args []string)
ResetHeartbeat: c.ResetHeartbeats,
}

_, err = cl.WorkflowService().ResetActivityById(cctx, request)
resp, err := cl.WorkflowService().ResetActivityById(cctx, request)
if err != nil {
return fmt.Errorf("unable to reset an Activity: %w", err)
}

resetResponse := struct {
NoWait bool
ResetHeartbeats bool
ServerResponse bool
ychebotarev marked this conversation as resolved.
Show resolved Hide resolved
}{
ServerResponse: resp != nil,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

NoWait: c.NoWait,
ResetHeartbeats: c.ResetHeartbeats,
}

_ = cctx.Printer.PrintStructured(resetResponse, printer.StructuredOptions{})

return nil
}
7 changes: 7 additions & 0 deletions temporalcli/commands.activity_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (

"go.temporal.io/api/enums/v1"
"go.temporal.io/api/history/v1"
"go.temporal.io/api/serviceerror"
"go.temporal.io/sdk/client"
)

Expand Down Expand Up @@ -247,6 +248,9 @@ func (s *SharedServerSuite) TestActivityReset() {
)

s.NoError(res.Err)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't mention it on the last review but I should have. Ideally these tests should confirm the invoked action actually occurs.

Copy link
Contributor Author

@ychebotarev ychebotarev Jan 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@cretz cretz Jan 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@ychebotarev ychebotarev Jan 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@cretz cretz Jan 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@ychebotarev ychebotarev Jan 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it'd basically be a functional test

Which already exist.
And that is why there is no need to do that.

Copy link
Member

@cretz cretz Jan 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, talked offline. I will make a change to make a check that some result (of proper type) is coming from server

// make sure we receive a server response
out := res.Stdout.String()
s.ContainsOnSameLine(out, "ServerResponse", "true")

// reset should fail because activity is not found

Expand All @@ -260,6 +264,9 @@ func (s *SharedServerSuite) TestActivityReset() {
)

s.Error(res.Err)
// make sure we receive a NotFound error from the server`
var notFound *serviceerror.NotFound
s.ErrorAs(res.Err, &notFound)
}

// Test helpers
Expand Down
Loading