Add client integration test for data converter #809
Conversation
|
|
||
| func TestClientIntegrationSuite(t *testing.T) { | ||
| flag.Parse() | ||
| if *integration { |
There was a problem hiding this comment.
in onebox.go. This provide a flag to control whether to run integration or not, default to run integration test. It exists in all integration test suites.
host/client_integration_test.go
Outdated
| ao := workflow.ActivityOptions{ | ||
| ScheduleToStartTimeout: time.Minute, | ||
| StartToCloseTimeout: time.Minute, | ||
| HeartbeatTimeout: 20 * time.Second, |
There was a problem hiding this comment.
remove HeartbeatTimeout as we dont need it here
host/client_integration_test.go
Outdated
| return "", err1 | ||
| } | ||
|
|
||
| logger.Info("Parent execution completed.", zap.String("Result", result+result1)) |
There was a problem hiding this comment.
could you assert the result and result1 here?
host/client_integration_test.go
Outdated
| } | ||
|
|
||
| // testDataConverter implements encoded.DataConverter using gob | ||
| type testDataConverter struct{} |
There was a problem hiding this comment.
could you add 2 counter field to testDataConverter and increase them when ToData() and FromData() is called. We need to assert at the end of the test that they are called expected number of times.
There was a problem hiding this comment.
This doesn't look like integration test, because from this test level you don't care/know what is the correct call times. Also those counters will change if client change somewhere, which confuse people.
I added a TestClientDataConverter_Failed case to cover that different data converter is actually used.
There was a problem hiding this comment.
I think it is valuable to verify that count number, and it is also good to verify that we don't call this encoding/decoding more than needed. For example, if there is a regression happen on client code that do this encoding
There was a problem hiding this comment.
Ok, I am fine with that. Added.
host/client_integration_test.go
Outdated
| if event.GetEventId() == 13 { | ||
| s.Equal(shared.EventTypeActivityTaskFailed, event.GetEventType()) | ||
| } | ||
| if event.GetEventId() == 17 { |
There was a problem hiding this comment.
if there is no event with id 17, then this won't be called, and this test will pass without verifying what we want to do.
There was a problem hiding this comment.
Good point. Add assertion.
The new added testSuite can be used to write tests using cadence-client.
This pull request contains a test for client feature DataConverter.
On mac, running this test always cause a popup dialog asking you to "accept incoming connections".This Yarpc related issue is fixed.Currently there is no good solution to get rid of this, you can press ESC each time or just turn off firewall.