Fix testing suite name conflict for workflow and activity (#887) #1371
Conversation
|
|
itechdima
force-pushed
the
feature/split-workflow-and-activity-mocks
branch
from
d07dca5 to
2eb70d7
Compare
| if !(e.workflowMock.AssertCalled(dummyT, methodName, arguments...) || e.activityMock.AssertCalled(dummyT, methodName, arguments...)) { | ||
| return e.workflowMock.AssertCalled(t, methodName, arguments...) && e.activityMock.AssertCalled(t, methodName, arguments...) | ||
| } | ||
| return true | ||
| } | ||
|
|
||
| // AssertNotCalled asserts that the method was not called with the given arguments. | ||
| // See AssertCalled for more info. | ||
| func (e *TestWorkflowEnvironment) AssertNotCalled(t mock.TestingT, methodName string, arguments ...interface{}) bool { |
There was a problem hiding this comment.
I think this fix is acceptable. I don't believe it is backwards compatible technically but if fixing this breaks their test the test was probably wrong to begin with.
Do you think we need workflow and activity versions of this and AssertCalled that differentiate between activity and workflow mocks?
There was a problem hiding this comment.
also AssertNumberOfCalls
There was a problem hiding this comment.
@cretz in the issue you mention backwards compatibility concerns. Did you have other concerns then these 3 functions?
There was a problem hiding this comment.
Do you mean AssertActivityCalled/AssertWorkflowCalled and others?
We might add them as well with keeping general AssertCalled (and others)
There was a problem hiding this comment.
Did you have other concerns then these 3 functions?
That was my only concern I can recall, our high-level assertion calls. I just want to make sure we don't break anyone's existing testing code.
Do you mean AssertActivityCalled/AssertWorkflowCalled and others?
We might add them as well with keeping general AssertCalled (and others)
Hrmm, I don't have a strong opinion here. I am guessing we don't want to give mock.Mock to the user, so I guess we might as well add those 3 for activity and workflows.
There was a problem hiding this comment.
I just want to make sure we don't break anyone's existing testing
I don't know the best approach here, but I can agree with @Quinn-With-Two-Ns that if these changes break anyones test, the test itself was logically wrong
Another point is that temporal allows you to use same names for activity and workflows, but SDK doesn't allow you to test it :)
There was a problem hiding this comment.
@cretz to be clear I think this change could cause some tests that passed before to fail, but those tests depended on workflow and activity mocks overriding each other which is a bug. I am also not sure how popular these functions even are. I think it is worth the risk of breaking some bad tests to fix this weird footgun for users.
There was a problem hiding this comment.
Yeah, I definitely don't mind if someone was relying on the name ambiguity and they break. But other than that, if we feel proper tests won't break, I'm good.
|
@itechdima I am seeing very consistent CI failures do you know what causes them? |
|
@Quinn-With-Two-Ns |
|
I'm not sure why another test has failed.. |
|
Rerunning to see |
|
@cretz |
What was changed
Splitted activity and workflow mocks so you're able to use same names to mock workflows and activities, but the logic of all exported functions kept the same
Why?
To be able to use same workflow and activity function names when they are struct methods (avoid tautology, etc.)
Checklist
Closes
TestSuite: Workflow names seemingly collide with activity names in mock expectations #887
How was this tested:
Added
Test_SameWorkflowAndActivityNamesfunction that uses same workflow and activity nameNo