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.

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 Signal-with-Start #111

Merged
merged 3 commits into from
Aug 15, 2024
Merged

Conversation

stephanos
Copy link
Contributor

@stephanos stephanos commented Aug 15, 2024

What was changed

Add Signal-with-Start to fuzzer.

Why?

First part of adding Signal-with-Start and soon Update-with-Start.

Checklist

  1. Closes

  2. How was this tested:

  1. Any docs updates needed?

@stephanos stephanos force-pushed the signal-with-start branch 2 times, most recently from 78e8093 to 713b8ef Compare August 15, 2024 20:33
@@ -65,6 +66,7 @@ message DoSignal {
// Send an arbitrary signal
HandlerInvocation custom = 2;
}
bool with_start = 3;
Copy link
Contributor Author

@stephanos stephanos Aug 15, 2024

Choose a reason for hiding this comment

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

I also considered adding it to ClientAction instead.

PS: How are we dealing with breaking changes; ie could I move it later or is there a backwards-compatibility guarantee?

Copy link
Member

Choose a reason for hiding this comment

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

Breaking changes aren't a problem here

@@ -15,6 +15,7 @@ import "temporal/api/enums/v1/workflow.proto";
message TestInput {
WorkflowInput workflow_input = 1;
ClientSequence client_sequence = 2;
ClientAction with_start_action = 3;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had considered using the client sequence's first item, but that seemed too surprising and messy (ie how do you represent "just start").

Copy link
Member

Choose a reason for hiding this comment

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

I prefer it here as well

@stephanos stephanos force-pushed the signal-with-start branch 2 times, most recently from 5906881 to adac1d6 Compare August 15, 2024 21:54
Comment on lines +189 to +195
- name: Upload generator diff
uses: actions/upload-artifact@v4
if: always()
with:
name: generator-diff
path: generator.diff
if-no-files-found: ignore
Copy link
Contributor Author

@stephanos stephanos Aug 15, 2024

Choose a reason for hiding this comment

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

This was a suggestion I added to get the protoc results from the CI, download them and apply them locally. Managing to get the right version locally, at least on OSX, is a nightmare.

Not sure if this should/can be documented somewhere?

Copy link
Member

Choose a reason for hiding this comment

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

Could add it to the README somewhere

@stephanos stephanos marked this pull request as ready for review August 15, 2024 22:42
@stephanos stephanos requested a review from Sushisource August 15, 2024 22:42
Copy link
Member

@Sushisource Sushisource left a comment

Choose a reason for hiding this comment

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

Looking pretty reasonable to me - just suggest one change to break out a new type.

@@ -15,6 +15,7 @@ import "temporal/api/enums/v1/workflow.proto";
message TestInput {
WorkflowInput workflow_input = 1;
ClientSequence client_sequence = 2;
ClientAction with_start_action = 3;
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be a new type though. Query doesn't make sense as a with start action, for example.

Comment on lines 307 to 313
let with_start_action = if u.ratio(80, 100)? {
None
} else {
let mut signal_action: DoSignal = u.arbitrary()?;
signal_action.with_start = true;
Some(ClientAction { variant: Some(client_action::Variant::DoSignal(signal_action)) })
};
Copy link
Member

Choose a reason for hiding this comment

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

If we apply my other suggestion of using a specific WithStartClientAction type, then this can become the Arbitrary implementation for that, which fits nicely.

@stephanos stephanos force-pushed the signal-with-start branch 3 times, most recently from f6a3a64 to 9854a5d Compare August 15, 2024 23:45
Copy link
Member

@Sushisource Sushisource left a comment

Choose a reason for hiding this comment

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

Noice.

Any cool outcomes from running it?

@stephanos
Copy link
Contributor Author

@Sushisource so far I've only run it for a few seconds to make sure my change was working

@stephanos stephanos merged commit 4b12728 into temporalio:main Aug 15, 2024
9 checks passed
@stephanos stephanos deleted the signal-with-start branch August 15, 2024 23:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants