Fixing unit_test failure detection, and tests for data converters #1341
Conversation
Apparently these also failed on the original PR, but due to missing a `\` in cadence-workflow#1303 it wasn't failing in CI. Thankfully this seems to be the only set of tests, and they were just incorrect.
Groxx
requested review from
shijiesheng,
agautam478,
jakobht,
3vilhamster,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted filessee 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| cat $(COVER_ROOT)/"$$dir"/cover.out | grep -v "mode: atomic" >> $(UT_COVER_FILE); \ | ||
| done; \ | ||
| done; test -z "$$FAIL" || (echo "Failed packages; $$FAIL"; exit 1) |
There was a problem hiding this comment.
copied this "report the packages" from server, and ran it all with failing and fixed tests to double check :|
it's much nicer for reading when something fails, and "on the same line" like this is hopefully a bit more resilient to the bug here.
| @@ -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 | |||
There was a problem hiding this comment.
previously I thought this was a general thing, but no! apparently not.
this might be something we can change in v2, since I don't think anyone really depends on it... or at least change the default. an explicit "no, encode byte slices too" seems fine, it's just unfortunate to lose the escape hatch without opting into it.
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 timeexitran.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:
Equal(string, []byte)just doesn't work, and the default dataconverter adds a trailing newline to separate args.[]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