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 Versioning Intents to Commands #342

Merged
merged 2 commits into from
Jul 11, 2023
Merged

Add Versioning Intents to Commands #342

merged 2 commits into from
Jul 11, 2023

Conversation

Sushisource
Copy link
Member

As is apparently tradition I forgot to do this in the first PR. ๐Ÿ˜ฎโ€๐Ÿ’จ

@cretz Not sure what the best way to test this is. Ideally I would just test that the commands get created the right way, but there are no obvious hooks on where to do that.

Maybe just make an interceptor? But, the interceptor is applied before converting things to what Core sees, which is what I want to test.

@Sushisource Sushisource requested a review from a team as a code owner July 10, 2023 23:41
@@ -49,6 +49,7 @@
import temporalio.exceptions
import temporalio.workflow

from ..workflow import VersioningIntent
Copy link
Member

Choose a reason for hiding this comment

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

Use temporalio.workflow.VersioningIntent qualified in this file instead of this relative import

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 what the best way to test this is. Ideally I would just test that the commands get created the right way, but there are no obvious hooks on where to do that.

Maybe just make an interceptor? But, the interceptor is applied before converting things to what Core sees, which is what I want to test.

Ideally you can test behavior of intent. If it's knowable from history, you can check history. Otherwise, and I know it seems a bit tedious, I would write a test with three workers: First worker runs the test workflow, second worker runs the activity, child workflow, etc when there is no versioning intent set, third worker runs the activity, child workflow, etc when there is versioning intent set. Then confirm that the work was distributed properly. Or something like that.

Whatever it takes to test the behavior of versioning intent. But if you want to add this in the features repo instead and do it later, works for me.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah that just feels weird to me, the behavior is already tested like that, in core. There's not really any point repeating that here, we just need to make sure the right thing gets sent to core. Otherwise we end up redundantly testing the same stuff over and over.

In other tests where that's a matter of just "run the workflow and it goes" that's no issue, but ones where you've gotta to a bunch of extra hoop jumping it seems silly.

I'll leave it for features, then.

Copy link
Member

@cretz cretz Jul 11, 2023

Choose a reason for hiding this comment

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

๐Ÿ‘ No prob. I am hesitant to add a bunch of unit-test-level introspection hooks to assert what is sent to core without testing behavior. Basically this whole project is driven by behavior integration tests which I think is clearest from a dev POV. If we test that behavior somewhere else though, no prob.

@Sushisource Sushisource merged commit 317dd9b into main Jul 11, 2023
@Sushisource Sushisource deleted the versioning-intent branch July 11, 2023 18:00
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