[BEAM-11188] Adding unit test for mergeExpandedWithPipeline #13370
Conversation
This is meant to be part of a larger change adding unit tests for the various pipeline transformations in graphx/xlang.go.
|
R: @lostluck |
lostluck
left a comment
lostluck
left a comment
There was a problem hiding this comment.
Fair enough. Dealing with protos is a Pain in the Butt to synthesize.
The only other way I can think of testing this is writing a Go side expansion service that can handle a single test transform, and simply plug it all together in process. However that seems much more implementation heavy until a Go side expansion service handler exists. Using the integration tests are reasonable for coverage. Thanks for trying!
| wantTransform := exp.Transform.(*pipepb.PTransform) | ||
| var found bool | ||
| for _, gotTransform := range gotComps.GetTransforms() { | ||
| if d := cmp.Diff(wantTransform, gotTransform, protocmp.Transform()); d == "" { |
There was a problem hiding this comment.
In this case, you probably just want cmp.Equal instead of cmp.Diff, since we never use the diff result.
There was a problem hiding this comment.
Whoops, kinda embarrassed I missed that. Done.
| } | ||
| } | ||
|
|
||
| func validateComponents(t *testing.T, wantComps, gotComps *pipepb.Components) { |
There was a problem hiding this comment.
I'm confused why this is broken out rather than simply using a single cmp.Diff(wantComps, gotComps, protocmp.Transform()) ?
I'm assuming that you wanted additional granularity on the parts that were missing/extra for each component, and the full diff was less clear than this output?
Consider adding a comment documenting that reasoning.
There was a problem hiding this comment.
Actually, the main reason was because the pipeline object can contain the components from multiple external transforms, so just diffing the components wouldn't work. What I needed to validate is that the components of the external transform are a subset of the pipeline components, not that they're exactly equal.
I'll add a comment explaining that, because I agree it's not immediately obvious.
2 participants
This is meant to be part of a larger change adding unit tests for the various pipeline transformations in graphx/xlang.go.
After how long it took to figure out how to implement this, and the realization that each other transformation is going to be similarly difficult and they're already somewhat tested by the xlang examples (and soon-to-be integration tests), I think I'd rather not implement the rest anymore. But I already implemented most of this test so I just finished it up and am sending it out in a PR.
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
R: @username).[BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replaceBEAM-XXXwith the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
Post-Commit Tests Status (on master branch)
Pre-Commit Tests Status (on master branch)
See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI.