search

uber-go / cadence-client

Framework for authoring workflows and activities running on top of the Cadence orchestration engine.
https://cadenceworkflow.io
MIT License
339 stars 128 forks source link

Revert breaking changes in v1.2.8 #1315

Closed ketsiambaku closed 4 months ago

ketsiambaku commented 4 months ago

What changed? Reverting the 2 commits below in this order: https://github.com/uber-go/cadence-client/pull/1256 https://github.com/uber-go/cadence-client/pull/1257

Why? At least one of these commits caused unit test failures for Uber internal cadence users when attempting to release v1.2.8

How did you test it? unit test

Potential risks

agautam478 commented 4 months ago

Can you confirm which one is causing failures? These were minor changes introduced to enable effective mocking. One is a complete test file and the other is a mini fix that makes registering Workflow and Activity registration optional when they are mocked.

ketsiambaku commented 4 months ago

Can you confirm which one is causing failures? These were minor changes introduced to enable effective mocking. One is a complete test file and the other is a mini fix that makes registering Workflow and Activity registration optional when they are mocked.

@agautam478 We observed unit test failures due to Duplicate Registration errors. #1256 seems to set DisableAlreadyRegisteredCheck flag to true so I thought it was a good candidate. reverting this did in fact fix the test failures I reverted #1257 because I felt it was built on top of the first PR but reverting one might just be enough in this case.

ketsiambaku commented 4 months ago

@edmondop I thought you should be aware :( We will be happy to re-review your PRs

Can you confirm which one is causing failures? These were minor changes introduced to enable effective mocking. One is a complete test file and the other is a mini fix that makes registering Workflow and Activity registration optional when they are mocked.

@agautam478 We observed unit test failures due to Duplicate Registration errors. #1256 seems to set DisableAlreadyRegisteredCheck flag to true so I thought it was a good candidate. reverting this did in fact fix the test failures I reverted #1257 because I felt it was built on top of the first PR but reverting one might just be enough in this case.

edmondop commented 4 months ago

Thanks, the change with the interceptor should not be able to break anything.

@agautam478 can you explain the issue you are seeing? Were you by chance registering an activity twice? I can look into it