[Supersaiyan101]

Fixes #18690.

Description

This pull request ports SolrIOTest in sdks/java/io/solr off the deprecated DoFnTester test harness in favor of standard TestPipeline execution.

• Removed deprecated import org.apache.beam.sdk.transforms.DoFnTester.
• Migrated testWriteWithMaxBatchSize() to execute through TestPipeline with Create.of(...) and write, verifying document indexing into Solr.

Checklist

• Small/focused contribution (ICLA not required).
• Description of change and motivation provided above.
• Linked issue (Fixes #18690).
• CHANGES.md update not required (internal test refactoring).

Verification

• Tested locally: .\gradlew.bat :sdks:java:io:solr:test (12/12 passed)
• Code style check: .\gradlew.bat :sdks:java:io:solr:spotlessCheck (PASSED)

[github-actions]

📋 Review Summary

This PR ports SolrIOTest.testWriteWithMaxBatchSize off the deprecated DoFnTester to TestPipeline (resolving #18690). However, because DirectRunner partitions Create.of(...) elements into small bundles of only a few elements each, replacing DoFnTester with a standard Create.of + SolrIO.write() pipeline causes the batch size threshold (BATCH_SIZE = 200) to never be reached in @ProcessElement, leaving the mid-bundle flush logic in WriteFn completely untested.

🔍 General Feedback

• Consider either setting a very small maxBatchSize (e.g., 1) so that the mid-bundle flush branch in WriteFn.processElement is guaranteed to execute even with small runner bundles, or forcing the test elements into a single bundle (for example via a preceding GroupByKey and ParDo that emits all documents within a single @ProcessElement invocation) while asserting intermediate flush behavior.

---

💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.

💬 Inline Findings

• sdks/java/io/solr/src/test/java/org/apache/beam/sdk/io/solr/SolrIOTest.java (line 236): 🟠 High: DirectRunner's Create.Values splits the input List into many small bundles (typically ~1–4 elements per bundle). Because BATCH_SIZE is 200, batch.size() >= spec.getMaxBatchSize() inside WriteFn.processElement is never triggered during pipeline.run(); every bundle simply flushes in @FinishBundle instead.

Additionally, the previous test specifically verified that documents were flushed to Solr before the bundle finished when maxBatchSize was reached. As written, testWriteWithMaxBatchSize is now nearly identical to testWrite and no longer exercises the maxBatchSize threshold in WriteFn.processElement.

To ensure TestPipeline actually exercises the batch size limit in @ProcessElement, you should either:

1. Force all NUM_DOCS documents into a single bundle (e.g., by grouping onto a single key and emitting all documents from a single DoFn.ProcessElement) or use a maxBatchSize of 1 so that batch.size() >= spec.getMaxBatchSize() is guaranteed to be hit during @ProcessElement.
2. Ideally, preserve the intermediate check (e.g., via a custom DoFn or SolrClient spy/wrapper, or by halting/checking state mid-bundle) to verify that flushing happens during @ProcessElement rather than only in @FinishBundle.

[github-actions]

Assigning reviewers:

R: @kennknowles for label java.
Backup reviewer: ahmedabu98 (if the main reviewer is unresponsive, you can escalate to them by tagging their username)

Note: If you would like to opt out of this review, comment assign to next reviewer.

Available commands:

• stop reviewer notifications - opt out of the automated review tooling
• remind me after tests pass - tag the comment author after tests pass
• waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

The PR bot will only process comments in the main thread (not review comments).

[Supersaiyan101]

/gemini review

[github-actions]

📋 Review Summary

This PR cleanly migrates SolrIOTest.testWriteWithMaxBatchSize off the deprecated DoFnTester harness onto TestPipeline (resolving #18690). The updated test groups documents into a single bundle and configures withMaxBatchSize(1) so that mid-bundle flushing during @ProcessElement in SolrIO.Write.WriteFn is exercised.

🔍 General Feedback

• The previous review feedback regarding bundle sizing in DirectRunner and exercising the maxBatchSize flush condition during @ProcessElement has been addressed. LGTM!

---

💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.

[Supersaiyan101]

R: @damccorm
Hi @damccorm hope you're having a great week!
When you have a moment, could you please take a quick look at this PR? It ports SolrIOTest off the deprecated DoFnTester (#18690).
All pre-commit CI checks are green, single-bundle & mid-bundle flush logic has been addressed, and the test passed clean.
Thank you so much!

[github-actions]

Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment assign set of reviewers

[kennknowles]

Looks good! It does change the test, but I think it makes it better. Before, the test was checking implementation details (like the fact that it only flushes at the end of a bundle) when tests are better if they are testing real end-to-end properties.

It makes it look more like an integration test, but I see it is a local fake so it is great!

[Supersaiyan101]

Thanks @kennknowles! That was exactly the goal moving away from micro testing internal lifecycle details like bundle flushes via DoFnTester, and instead testing the transform's contract end to end against TestPipeline.....Using the embedded/local Solr server keeps it completely hermetic and fast while giving us much higher confidence in actual execution semantics....