Repository navigation
Port DatastoreV1Test off DoFnTester - #40866
Rishavbhattarai wants to merge 2 commits into
Conversation
The fns under test hold Mockito mocks that can't be serialized, so TestPipeline doesn't fit. Add a small processBundle helper that runs the fn in-process through DoFnRunners.simpleRunner and use it at the 10 DoFnTester call sites. Fixes apache#18692
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
📋 Review Summary
This PR cleanly migrates DatastoreV1Test off the deprecated DoFnTester by introducing an in-process processBundle helper built on DoFnRunners.simpleRunner. This approach is appropriate here because the DoFns under test hold non-serializable Mockito mocks and only rely on @StartBundle, @ProcessElement, and @FinishBundle lifecycle methods.
🔍 General Feedback
- The
processBundlehelper accurately preserves the single-bundle, non-cloning execution semantics previously configured withCloningBehavior.DO_NOT_CLONE. - Minor cleanup needed:
processBundlewas inserted betweenFakeWriteBatcherand its Javadoc comment.
💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.
| runner.finishBundle(); | ||
| return outputs; | ||
| } | ||
|
|
There was a problem hiding this comment.
🟢 Low: processBundle was inserted between FakeWriteBatcher and its Javadoc comment (lines 1359–1362), leaving a duplicate/dangling Javadoc block above processBundle. Please move the WriteBatcher Javadoc back down to FakeWriteBatcher.
There was a problem hiding this comment.
Fixed in 994a46b, the Javadoc is back above FakeWriteBatcher.
|
Assigning reviewers: R: @kennknowles for label java. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
Fixes #18692.
Ports
DatastoreV1Testoff the deprecatedDoFnTester.TestPipelinedoesn't fit here.DatastoreWriterFn,SplitQueryFnandReadFnget their client frommockDatastoreFactory, a Mockito mock that can't be serialized, and the tests verify calls on that mock after the bundle. That's why everyDoFnTestercall usedCloningBehavior.DO_NOT_CLONE.So I added one helper to the test,
processBundle(fn, inputs). It runs the fn in-process throughDoFnRunners.simpleRunner(start bundle, process each input in the global window, finish bundle) and returns the main outputs. The 10 call sites now call it, and the comments about disabling cloning are gone since nothing gets cloned.:runners:core-javais already animplementationdependency of this module, so there's no build change.Testing:
./gradlew :sdks:java:io:google-cloud-platform:test --tests 'org.apache.beam.sdk.io.gcp.datastore.DatastoreV1Test*'passes (62/62 acrossSingletonTestsandParameterizedTests), andspotlessCheckis clean.