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

Remove dummy registrations in sdk integration test #2349

Merged
merged 1 commit into from
Jan 13, 2022

Conversation

dnr
Copy link
Member

@dnr dnr commented Jan 6, 2022

What changed?
Remove dummy registrations that aren't needed anymore.

Why?
They can cause name conflicts with other tests that use anonymous functions as workflows/activities.

How did you test it?
they are tests

Potential risks

Is hotfix candidate?

@dnr dnr requested a review from a team January 6, 2022 19:04
@yiminc
Copy link
Member

yiminc commented Jan 6, 2022

Do you observe conflict? The SetupTest() would re-create worker for each test and registration is per worker, so it should not have problem.

@dnr
Copy link
Member Author

dnr commented Jan 6, 2022

Do you observe conflict? The SetupTest() would re-create worker for each test and registration is per worker, so it should not have problem.

It was breaking my cron test rewrite until I gave it an explicit name. To test the theory, I rearranged code in the existing tests and got one of them to conflict too. I didn't look into how the worker is recreated so I don't fully understand it either

Copy link
Member

@yiminc yiminc left a comment

Choose a reason for hiding this comment

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

it is worth to spend sometime understand why it could happen, each worker should have their own registry.

@dnr
Copy link
Member Author

dnr commented Jan 10, 2022

I investigated a little more. I think the problem is that clientIntegrationSuite.SetupTest registers a dummy workflow and activity, and those were getting registered with names func1 and func2, which are easy to conflict with anonymous workflow and activity functions in the actual tests. Even if I add a RegisterOptions to those calls, it still conflicts, since the sdk still keeps the actual function names as aliases.

If I remote the dummy registrations entirely, the tests pass for me, though. It looks you added them in #1934. Do you remember why they were needed? If they are needed after all, I think the best option would be to make them top-level functions.

@yiminc
Copy link
Member

yiminc commented Jan 12, 2022

@dnr we can remove the dummy func registration. They were added because the SDK would skip worker start if no func was registered. That has been changed in SDK so you could start worker without registration and add registration later after start. It is safe to remove them now.

@dnr dnr changed the title Override names for anonymous functions used as workflows or activities Remove dummy registrations in sdk integration test Jan 13, 2022
@dnr dnr enabled auto-merge (squash) January 13, 2022 17:41
@dnr dnr merged commit 72e5b67 into temporalio:master Jan 13, 2022
@dnr dnr deleted the anon-wf-names branch January 20, 2022 03:06
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