-
Notifications
You must be signed in to change notification settings - Fork 132
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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 uber-go#1303 it wasn't failing in CI. Thankfully this seems to be the only set of tests, and they were just incorrect.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 timeexit
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:
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:cadence-client/internal/internal_worker.go
Lines 573 to 579 in 6decfc7