kotman12 commented on code in PR #4279:
URL: https://github.com/apache/solr/pull/4279#discussion_r3069514510
##########
solr/core/src/test/org/apache/solr/handler/admin/UpgradeCoreIndexActionTest.java:
##########
@@ -365,10 +365,192 @@ public void
testUpgradeCoreIndexFailsWithNestedDocuments() throws Exception {
coreName),
resp));
- // Verify the exception message indicates nested documents are not
supported
+ // Verify the exception message indicates child documents are not
supported
assertThat(
thrown.getMessage(),
- containsString("does not support indexes containing nested
documents"));
+ containsString("does not support indexes containing child
documents"));
+ } finally {
+ admin.shutdown();
+ admin.close();
+ }
+ }
+
+ // --- Child docs detection tests ---
Review Comment:
These are indeed LLM generated but I was very intentional about what
scenarios to test, i.e. hasChildDocs X hasUpdates X hasDeletes. These variables
have different effects on the distinct count vs `docCount` of both `id` and
`_root_` and would like any hypothetical future refactoring to take into
account each since the logic is a function of these variables. I am not
convinced a randomized test would have been effective here without knowing what
edge cases you are looking for a priori. I suppose in theory it could have
caught _other_ things I hadn't considered although _seems_ unlikely given the
relatively simple logic and few moving parts (famous last words). Thus, there
is advantage to deterministically validating these known edge cases with each
build. That being said, it feels like overkill to test these in
`UpgradeCoreIndex` as this check is a rather small component of the overall
operation. Perhaps this check should live in some utility function? Then we
could _also_ call it
from, say, the LukeHandler and expose a `hasNested/ChildDocs` in the index
info. The benefit of such thorough testing would be more obvious in a targeted
utility. What do you think?
As an aside, is your reflexive urge to use randomized testing here motivated
by _aesthetic_ reasons? If so, I could consider parameterizing them. I think it
should be possible.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
