Skip to content

Commit

Permalink
Fixing unit_test failure detection, and tests for data converters (#1341
Browse files Browse the repository at this point in the history 
)

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
  • Loading branch information
@Groxx
Groxx committed Jun 7, 2024
1 parent f5ec359 commit bf68484
Show file tree
Hide file tree
Showing 2 changed files with 77 additions and 20 deletions.
7 changes: 3 additions & 4 deletions Makefile
View file
Original file line number Diff line number Diff line change
Expand Up @@ -375,14 +375,13 @@ test: unit_test integ_test_sticky_off integ_test_sticky_on ## run all tests (req
unit_test: $(ALL_SRC) ## run all unit tests
$Q mkdir -p $(COVER_ROOT)
$Q echo "mode: atomic" > $(UT_COVER_FILE)
$Q failed=0; \
$Q FAIL=""; \
for dir in $(UT_DIRS); do \
mkdir -p $(COVER_ROOT)/"$$dir"; \
go test "$$dir" $(TEST_ARG) -coverprofile=$(COVER_ROOT)/"$$dir"/cover.out || failed=1; \
go test "$$dir" $(TEST_ARG) -coverprofile=$(COVER_ROOT)/"$$dir"/cover.out || FAIL="$$FAIL $$dir"; \
cat $(COVER_ROOT)/"$$dir"/cover.out | grep -v "mode: atomic" >> $(UT_COVER_FILE); \
done; \
done; test -z "$$FAIL" || (echo "Failed packages; $$FAIL"; exit 1)
cat $(UT_COVER_FILE) > .build/cover.out;
exit $$failed

integ_test_sticky_off: $(ALL_SRC)
$Q mkdir -p $(COVER_ROOT)
Expand Down
90 changes: 74 additions & 16 deletions internal/internal_workflow_client_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1150,14 +1150,14 @@ func (s *workflowClientTestSuite) TestStartWorkflow_WithDataConverter() {
ExecutionStartToCloseTimeout: timeoutInSeconds,
DecisionTaskStartToCloseTimeout: timeoutInSeconds,
}
f1 := func(ctx Context, r []byte) string {
f1 := func(ctx Context, r string) string {
return "result"
}
input := []byte("test")
input := "test" // note: not []byte as that bypasses encoding, and we are exercising the dataconverter

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

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

func (s *workflowClientTestSuite) TestStartWorkflow_ByteBypass() {
// default DataConverter checks for []byte args, and does not re-encode them.
// this is NOT a general feature of DataConverter, though perhaps it should be
// as it's a quite useful escape hatch when making incompatible type changes.
client := NewClient(s.service, domain, nil)
options := StartWorkflowOptions{
ID: workflowID,
TaskList: tasklist,
ExecutionStartToCloseTimeout: timeoutInSeconds,
DecisionTaskStartToCloseTimeout: timeoutInSeconds,
}
f1 := func(ctx Context, r []byte) string {
return "result"
}
input := []byte("test") // intentionally bypassing dataconverter

createResponse := &shared.StartWorkflowExecutionResponse{
RunId: common.StringPtr(runID),
}
s.service.EXPECT().StartWorkflowExecution(gomock.Any(), gomock.Any(), gomock.Any()).Return(createResponse, nil).
Do(func(_ interface{}, req *shared.StartWorkflowExecutionRequest, _ ...interface{}) {
assert.Equal(s.T(), input, req.Input, "[]byte inputs should not be re-encoded by the default converter")
})

resp, err := client.StartWorkflow(context.Background(), options, f1, input)
s.NoError(err)
s.Equal(createResponse.GetRunId(), resp.RunID)
}

func (s *workflowClientTestSuite) TestStartWorkflow_WithMemoAndSearchAttr() {
memo := map[string]interface{}{
"testMemo": "memo value",
Expand All @@ -1198,8 +1227,9 @@ func (s *workflowClientTestSuite) TestStartWorkflow_WithMemoAndSearchAttr() {

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

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

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

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

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

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

func (s *workflowClientTestSuite) TestStartWorkflowAsync_ByteBypass() {
// default DataConverter checks for []byte args, and does not re-encode them.
// this is NOT a general feature of DataConverter, though perhaps it should be
// as it's a quite useful escape hatch when making incompatible type changes.
client := NewClient(s.service, domain, nil)
options := StartWorkflowOptions{
ID: workflowID,
TaskList: tasklist,
ExecutionStartToCloseTimeout: timeoutInSeconds,
DecisionTaskStartToCloseTimeout: timeoutInSeconds,
}
f1 := func(ctx Context, r []byte) string {
return "result"
}
input := []byte("test") // intentionally bypassing dataconverter

s.service.EXPECT().StartWorkflowExecutionAsync(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, nil).
Do(func(_ interface{}, req *shared.StartWorkflowExecutionAsyncRequest, _ ...interface{}) {
assert.Equal(s.T(), input, req.Request.Input, "[]byte inputs should not be re-encoded by the default converter")
})

_, err := client.StartWorkflowAsync(context.Background(), options, f1, input)
s.NoError(err)
}

func (s *workflowClientTestSuite) TestStartWorkflowAsync_WithMemoAndSearchAttr() {
memo := map[string]interface{}{
"testMemo": "memo value",
Expand All @@ -1430,8 +1487,9 @@ func (s *workflowClientTestSuite) TestStartWorkflowAsync_WithMemoAndSearchAttr()
s.service.EXPECT().StartWorkflowExecutionAsync(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, nil).
Do(func(_ interface{}, asyncReq *shared.StartWorkflowExecutionAsyncRequest, _ ...interface{}) {
req := asyncReq.Request
s.Equal(`"memo value"`, req.Memo.Fields["testMemo"], "memos must be JSON-encoded")
s.Equal(`"attr value"`, req.SearchAttributes.IndexedFields["testAttr"], "search attributes must be JSON-encoded")
// trailing newline is added by the dataconverter
s.Equal([]byte("\"memo value\"\n"), req.Memo.Fields["testMemo"], "memos use the dataconverter because they are user data")
s.Equal([]byte(`"attr value"`), req.SearchAttributes.IndexedFields["testAttr"], "search attributes must be JSON-encoded")
})

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

0 comments on commit bf68484

Please sign in to comment.