Skip to content

Commit bf68484

Browse files
GroxxGroxx
authored
Fixing unit_test failure detection, and tests for data converters (#1341)
Apparently these also failed on the original PR, but due to missing a `\` in #1303 it wasn't failing in CI because the env var was gone by the time `exit` ran. I should really take that as a hint to finally rewrite that chunk of the makefile, so it just does a normal `go test ./...` :\ but not today. Thankfully this seems to be the only set of tests that snuck past, and they were just incorrect. I'm not sure why I apparently didn't run them or something while writing them in #1331 but it seems pretty clear I didn't since the old pre-merge SHA fails too. Bleh. --- To correct the tests' original flaws: 1. memos *are* supposed to be encoded by custom dataconverters, and they are. we don't search them so they're not JSON like search attrs are, they're just blobs we expose in list APIs (and in workflow metadata). 2. `Equal(string, []byte)` just doesn't work, and the default dataconverter adds a trailing newline to separate args. 3. the `[]byte`-heavy copypasta meant the Input-checking tests were odd, so I changed them, and fixed the args to the dataconverter. Internally we splat the args in here, so it's not `[]interface{..}`-packed: https://github.com/uber-go/cadence-client/blob/6decfc78571a9d91d943815ae3a445a3bc115fa8/internal/internal_worker.go#L573-L579
1 parent f5ec359 commit bf68484

File tree

2 files changed

+77
-20
lines changed

2 files changed

+77
-20
lines changed

‎Makefile‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -375,14 +375,13 @@ test: unit_test integ_test_sticky_off integ_test_sticky_on ## run all tests (req
375375
unit_test: $(ALL_SRC) ## run all unit tests
376376
$Q mkdir -p $(COVER_ROOT)
377377
$Q echo "mode: atomic" > $(UT_COVER_FILE)
378-
$Q failed=0; \
378+
$Q FAIL=""; \
379379
for dir in $(UT_DIRS); do \
380380
mkdir -p $(COVER_ROOT)/"$$dir"; \
381-
go test "$$dir" $(TEST_ARG) -coverprofile=$(COVER_ROOT)/"$$dir"/cover.out || failed=1; \
381+
go test "$$dir" $(TEST_ARG) -coverprofile=$(COVER_ROOT)/"$$dir"/cover.out || FAIL="$$FAIL $$dir"; \
382382
cat $(COVER_ROOT)/"$$dir"/cover.out | grep -v "mode: atomic" >> $(UT_COVER_FILE); \
383-
done; \
383+
done; test -z "$$FAIL" || (echo "Failed packages; $$FAIL"; exit 1)
384384
cat $(UT_COVER_FILE) > .build/cover.out;
385-
exit $$failed
386385

387386
integ_test_sticky_off: $(ALL_SRC)
388387
$Q mkdir -p $(COVER_ROOT)

‎internal/internal_workflow_client_test.go‎

Lines changed: 74 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1150,14 +1150,14 @@ func (s *workflowClientTestSuite) TestStartWorkflow_WithDataConverter() {
11501150
ExecutionStartToCloseTimeout: timeoutInSeconds,
11511151
DecisionTaskStartToCloseTimeout: timeoutInSeconds,
11521152
}
1153-
f1 := func(ctx Context, r []byte) string {
1153+
f1 := func(ctx Context, r string) string {
11541154
return "result"
11551155
}
1156-
input := []byte("test")
1156+
input := "test" // note: not []byte as that bypasses encoding, and we are exercising the dataconverter
11571157

1158-
correctlyEncoded, err := dc.ToData([]interface{}{input})
1158+
correctlyEncoded, err := dc.ToData(input) // []any is spread to ToData args
11591159
require.NoError(s.T(), err, "test data converter should not fail on simple inputs")
1160-
defaultEncoded, err := getDefaultDataConverter().ToData([]interface{}{input})
1160+
defaultEncoded, err := getDefaultDataConverter().ToData(input)
11611161
require.NoError(s.T(), err, "default data converter should not fail on simple inputs")
11621162

11631163
// sanity check: we must be able to tell right from wrong
@@ -1176,6 +1176,35 @@ func (s *workflowClientTestSuite) TestStartWorkflow_WithDataConverter() {
11761176
s.Equal(createResponse.GetRunId(), resp.RunID)
11771177
}
11781178

1179+
func (s *workflowClientTestSuite) TestStartWorkflow_ByteBypass() {
1180+
// default DataConverter checks for []byte args, and does not re-encode them.
1181+
// this is NOT a general feature of DataConverter, though perhaps it should be
1182+
// as it's a quite useful escape hatch when making incompatible type changes.
1183+
client := NewClient(s.service, domain, nil)
1184+
options := StartWorkflowOptions{
1185+
ID: workflowID,
1186+
TaskList: tasklist,
1187+
ExecutionStartToCloseTimeout: timeoutInSeconds,
1188+
DecisionTaskStartToCloseTimeout: timeoutInSeconds,
1189+
}
1190+
f1 := func(ctx Context, r []byte) string {
1191+
return "result"
1192+
}
1193+
input := []byte("test") // intentionally bypassing dataconverter
1194+
1195+
createResponse := &shared.StartWorkflowExecutionResponse{
1196+
RunId: common.StringPtr(runID),
1197+
}
1198+
s.service.EXPECT().StartWorkflowExecution(gomock.Any(), gomock.Any(), gomock.Any()).Return(createResponse, nil).
1199+
Do(func(_ interface{}, req *shared.StartWorkflowExecutionRequest, _ ...interface{}) {
1200+
assert.Equal(s.T(), input, req.Input, "[]byte inputs should not be re-encoded by the default converter")
1201+
})
1202+
1203+
resp, err := client.StartWorkflow(context.Background(), options, f1, input)
1204+
s.NoError(err)
1205+
s.Equal(createResponse.GetRunId(), resp.RunID)
1206+
}
1207+
11791208
func (s *workflowClientTestSuite) TestStartWorkflow_WithMemoAndSearchAttr() {
11801209
memo := map[string]interface{}{
11811210
"testMemo": "memo value",
@@ -1198,8 +1227,9 @@ func (s *workflowClientTestSuite) TestStartWorkflow_WithMemoAndSearchAttr() {
11981227

11991228
s.service.EXPECT().StartWorkflowExecution(gomock.Any(), gomock.Any(), gomock.Any()).Return(startResp, nil).
12001229
Do(func(_ interface{}, req *shared.StartWorkflowExecutionRequest, _ ...interface{}) {
1201-
s.Equal(`"memo value"`, req.Memo.Fields["testMemo"], "memos must be JSON-encoded")
1202-
s.Equal(`"attr value"`, req.SearchAttributes.IndexedFields["testAttr"], "search attributes must be JSON-encoded")
1230+
// trailing newline is added by the dataconverter
1231+
s.Equal([]byte("\"memo value\"\n"), req.Memo.Fields["testMemo"], "memos use the dataconverter because they are user data")
1232+
s.Equal([]byte(`"attr value"`), req.SearchAttributes.IndexedFields["testAttr"], "search attributes must be JSON-encoded, not using dataconverter")
12031233
})
12041234

12051235
_, err := s.client.StartWorkflow(context.Background(), options, wf)
@@ -1266,8 +1296,9 @@ func (s *workflowClientTestSuite) TestSignalWithStartWorkflow_WithMemoAndSearchA
12661296
s.service.EXPECT().SignalWithStartWorkflowExecution(gomock.Any(), gomock.Any(), gomock.Any(),
12671297
gomock.Any(), gomock.Any(), gomock.Any()).Return(startResp, nil).
12681298
Do(func(_ interface{}, req *shared.SignalWithStartWorkflowExecutionRequest, _ ...interface{}) {
1269-
s.Equal(`"memo value"`, req.Memo.Fields["testMemo"], "memos must be JSON-encoded")
1270-
s.Equal(`"attr value"`, req.SearchAttributes.IndexedFields["testAttr"], "search attributes must be JSON-encoded")
1299+
// trailing newline is added by the dataconverter
1300+
s.Equal([]byte("\"memo value\"\n"), req.Memo.Fields["testMemo"], "memos use the dataconverter because they are user data")
1301+
s.Equal([]byte(`"attr value"`), req.SearchAttributes.IndexedFields["testAttr"], "search attributes must be JSON-encoded")
12711302
})
12721303

12731304
_, err := s.client.SignalWithStartWorkflow(context.Background(), "wid", "signal", "value", options, wf)
@@ -1297,8 +1328,9 @@ func (s *workflowClientTestSuite) TestSignalWithStartWorkflowAsync_WithMemoAndSe
12971328
gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, nil).
12981329
Do(func(_ interface{}, asyncReq *shared.SignalWithStartWorkflowExecutionAsyncRequest, _ ...interface{}) {
12991330
req := asyncReq.Request
1300-
s.Equal(`"memo value"`, req.Memo.Fields["testMemo"], "memos must be JSON-encoded")
1301-
s.Equal(`"attr value"`, req.SearchAttributes.IndexedFields["testAttr"], "search attributes must be JSON-encoded")
1331+
// trailing newline is added by the dataconverter
1332+
s.Equal([]byte("\"memo value\"\n"), req.Memo.Fields["testMemo"], "memos use the dataconverter because they are user data")
1333+
s.Equal([]byte(`"attr value"`), req.SearchAttributes.IndexedFields["testAttr"], "search attributes must be JSON-encoded")
13021334
})
13031335

13041336
_, err := s.client.SignalWithStartWorkflowAsync(context.Background(), "wid", "signal", "value", options, wf)
@@ -1384,16 +1416,16 @@ func (s *workflowClientTestSuite) TestStartWorkflowAsync_WithDataConverter() {
13841416
ExecutionStartToCloseTimeout: timeoutInSeconds,
13851417
DecisionTaskStartToCloseTimeout: timeoutInSeconds,
13861418
}
1387-
f1 := func(ctx Context, r []byte) string {
1419+
f1 := func(ctx Context, r string) string {
13881420
return "result"
13891421
}
1390-
input := []byte("test")
1422+
input := "test" // note: not []byte as that bypasses encoding, and we are exercising the dataconverter
13911423

13921424
// this test requires that the overridden test-data-converter is used, so we need to make sure the encoded input looks correct.
13931425
// the test data converter's output doesn't actually matter as long as it's noticeably different.
1394-
correctlyEncoded, err := dc.ToData([]interface{}{input})
1426+
correctlyEncoded, err := dc.ToData(input) // []any is spread to ToData args
13951427
require.NoError(s.T(), err, "test data converter should not fail on simple inputs")
1396-
wrongDefaultEncoding, err := getDefaultDataConverter().ToData([]interface{}{input})
1428+
wrongDefaultEncoding, err := getDefaultDataConverter().ToData(input)
13971429
require.NoError(s.T(), err, "default data converter should not fail on simple inputs")
13981430

13991431
// sanity check: we must be able to tell right from wrong
@@ -1408,6 +1440,31 @@ func (s *workflowClientTestSuite) TestStartWorkflowAsync_WithDataConverter() {
14081440
s.NoError(err)
14091441
}
14101442

1443+
func (s *workflowClientTestSuite) TestStartWorkflowAsync_ByteBypass() {
1444+
// default DataConverter checks for []byte args, and does not re-encode them.
1445+
// this is NOT a general feature of DataConverter, though perhaps it should be
1446+
// as it's a quite useful escape hatch when making incompatible type changes.
1447+
client := NewClient(s.service, domain, nil)
1448+
options := StartWorkflowOptions{
1449+
ID: workflowID,
1450+
TaskList: tasklist,
1451+
ExecutionStartToCloseTimeout: timeoutInSeconds,
1452+
DecisionTaskStartToCloseTimeout: timeoutInSeconds,
1453+
}
1454+
f1 := func(ctx Context, r []byte) string {
1455+
return "result"
1456+
}
1457+
input := []byte("test") // intentionally bypassing dataconverter
1458+
1459+
s.service.EXPECT().StartWorkflowExecutionAsync(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, nil).
1460+
Do(func(_ interface{}, req *shared.StartWorkflowExecutionAsyncRequest, _ ...interface{}) {
1461+
assert.Equal(s.T(), input, req.Request.Input, "[]byte inputs should not be re-encoded by the default converter")
1462+
})
1463+
1464+
_, err := client.StartWorkflowAsync(context.Background(), options, f1, input)
1465+
s.NoError(err)
1466+
}
1467+
14111468
func (s *workflowClientTestSuite) TestStartWorkflowAsync_WithMemoAndSearchAttr() {
14121469
memo := map[string]interface{}{
14131470
"testMemo": "memo value",
@@ -1430,8 +1487,9 @@ func (s *workflowClientTestSuite) TestStartWorkflowAsync_WithMemoAndSearchAttr()
14301487
s.service.EXPECT().StartWorkflowExecutionAsync(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, nil).
14311488
Do(func(_ interface{}, asyncReq *shared.StartWorkflowExecutionAsyncRequest, _ ...interface{}) {
14321489
req := asyncReq.Request
1433-
s.Equal(`"memo value"`, req.Memo.Fields["testMemo"], "memos must be JSON-encoded")
1434-
s.Equal(`"attr value"`, req.SearchAttributes.IndexedFields["testAttr"], "search attributes must be JSON-encoded")
1490+
// trailing newline is added by the dataconverter
1491+
s.Equal([]byte("\"memo value\"\n"), req.Memo.Fields["testMemo"], "memos use the dataconverter because they are user data")
1492+
s.Equal([]byte(`"attr value"`), req.SearchAttributes.IndexedFields["testAttr"], "search attributes must be JSON-encoded")
14351493
})
14361494

14371495
_, err := s.client.StartWorkflowAsync(context.Background(), options, wf)

0 commit comments

Comments
 (0)