SOLR-17650: Fix tests for unordered buffered updates #3197
Conversation
solr/test-framework/src/java/org/apache/solr/SolrTestCaseJ4.java
Outdated
Show resolved
Hide resolved
| * @return The request response as a JSON String if the test matcher passes | ||
| */ | ||
| @SuppressWarnings("unchecked") | ||
| public static <T> String assertThatJQ(SolrQueryRequest req, String message, Matcher<T> test) |
There was a problem hiding this comment.
You are adding more methods that uses the almost but not quite yet deprecated TestHarness. Can you please instead consider changing the approach of this method to look like org.apache.solr.SolrTestCaseHS#assertJQ which takes a SolrClient? Let's say you agree to do that. My next thought is... should SolrTestCase have such extensive utility methods or should they live on a helper class (a new one I guess). I lean to the latter but I haven't thought about that much yet.
There was a problem hiding this comment.
I thought about this, but the entire test is set up to use the test harness, so I wanted to implement this test to fit in with the surrounding tests as best as possible.
Adding one more convenience method to the test harness to fit the existing workflow I think is pretty minor compared to starting a whole new way for this test to behave. Deprecating the TestHarness (and actually removing it) is going to be a MAJOR ordeal, and honestly IMO changing a test to 1/2 use the test harness and 1/2 use something else will probably make that harder in the end.
(cherry picked from commit 0a50eda)
https://issues.apache.org/jira/browse/SOLR-17650
According to the discussion in the JIRA, the behavior here is fine. We just need to update the tests now that buffered updates can be done in parallel.
The new testing stuff is heavy handed, but mostly needed to 1) test this at all and 2) give us a good error message that we can actually use.