Github user dsmiley commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/410#discussion_r201076987
--- Diff:
solr/core/src/test/org/apache/solr/update/TestNestedUpdateProcessor.java ---
@@ -120,25 +122,41 @@ public void before() throws Exception {
@Test
public void testDeeplyNestedURPGrandChild() throws Exception {
+ final String[] tests = {
+ "/response/docs/[0]/id=='" + grandChildId + "'",
+ "/response/docs/[0]/" + IndexSchema.NEST_PATH_FIELD_NAME +
"=='children#0/grandChild#'"
+ };
indexSampleData(jDoc);
- assertJQ(req("q", IndexSchema.NEST_PATH_FIELD_NAME + ":*" +
PATH_SEP_CHAR + "grandChild" + NUM_SEP_CHAR + "*" + NUM_SEP_CHAR,
+ assertJQ(req("q", IndexSchema.NEST_PATH_FIELD_NAME + ":*" +
PATH_SEP_CHAR + "grandChild" + NUM_SEP_CHAR + "*",
"fl","*",
"sort","id desc",
"wt","json"),
- "/response/docs/[0]/id=='" + grandChildId + "'");
+ tests);
}
@Test
public void testDeeplyNestedURPChildren() throws Exception {
--- End diff --
This test tests the search behavior more so than literally what the URP is
doing. Can you make this more of a unit test around the result of the URP
without actually indexing/searching anything? And I would much prefer simpler
test assertions that check a complete string value instead of making reference
to many variables/constants that need to be concatenated. This makes it
plainly clear what the nest path will be; no mental gymnastics are needed to
chase down vars/constants to figure it out. I've mentioned before Yonik's
advise on avoiding some constants in tests as it helps tests make us aware if
in the future we might have a backwards-breaking change; so there are virtues
to this way of thinking. It would make this easier to review too.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
