Re: [PR] SOLR-17650: Fix tests for unordered buffered updates [solr]
HoustonPutman merged PR #3197: URL: https://github.com/apache/solr/pull/3197 -- 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: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17650: Fix tests for unordered buffered updates [solr]
dsmiley commented on code in PR #3197:
URL: https://github.com/apache/solr/pull/3197#discussion_r1969829463
##
solr/test-framework/src/java/org/apache/solr/SolrTestCaseJ4.java:
##
@@ -1027,6 +1028,64 @@ public static String assertJQ(SolrQueryRequest req,
double delta, String... test
}
}
+ public static String assertThatJQ(SolrQueryRequest req, Matcher test)
throws Exception {
+return assertThatJQ(req, "", test);
+ }
+
+ /**
+ * Validates a query completes and, using JSON deserialization, returns an
object that passes the
+ * given Matcher test.
+ *
+ * Please use this with care: this makes it easy to match complete
structures, but doing so can
+ * result in fragile tests if you are matching more than what you want to
test.
+ *
+ * @param req Solr request to execute
+ * @param message Failure message for test
+ * @param test Matcher for the given object returned from deserializing the
response
+ * @return The request response as a JSON String if the test matcher passes
+ */
+ @SuppressWarnings("unchecked")
+ public static String assertThatJQ(SolrQueryRequest req, String message,
Matcher test)
Review Comment:
That's fair
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17650: Fix tests for unordered buffered updates [solr]
HoustonPutman commented on code in PR #3197:
URL: https://github.com/apache/solr/pull/3197#discussion_r1968316944
##
solr/test-framework/src/java/org/apache/solr/SolrTestCaseJ4.java:
##
@@ -1027,6 +1028,64 @@ public static String assertJQ(SolrQueryRequest req,
double delta, String... test
}
}
+ public static String assertThatJQ(SolrQueryRequest req, Matcher test)
throws Exception {
+return assertThatJQ(req, "", test);
+ }
+
+ /**
+ * Validates a query completes and, using JSON deserialization, returns an
object that passes the
+ * given Matcher test.
+ *
+ * Please use this with care: this makes it easy to match complete
structures, but doing so can
+ * result in fragile tests if you are matching more than what you want to
test.
+ *
+ * @param req Solr request to execute
+ * @param message Failure message for test
+ * @param test Matcher for the given object returned from deserializing the
response
+ * @return The request response as a JSON String if the test matcher passes
+ */
+ @SuppressWarnings("unchecked")
+ public static String assertThatJQ(SolrQueryRequest req, String message,
Matcher test)
Review 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.
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17650: Fix tests for unordered buffered updates [solr]
markrmiller commented on code in PR #3197:
URL: https://github.com/apache/solr/pull/3197#discussion_r1964347792
##
solr/test-framework/src/java/org/apache/solr/util/SolrMatchers.java:
##
@@ -0,0 +1,81 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.util;
+
+import java.util.List;
+import org.hamcrest.Description;
+import org.hamcrest.Matcher;
+import org.hamcrest.TypeSafeDiagnosingMatcher;
+
+public class SolrMatchers {
+
+ public static Matcher> subListMatches(
Review Comment:
Could add "Matches a segment of a list between two indices against a
provided matcher, useful for checking ordered sub-sequences in a larger list"
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
Re: [PR] SOLR-17650: Fix tests for unordered buffered updates [solr]
dsmiley commented on code in PR #3197:
URL: https://github.com/apache/solr/pull/3197#discussion_r1962529603
##
solr/test-framework/src/java/org/apache/solr/SolrTestCaseJ4.java:
##
@@ -1027,6 +1028,64 @@ public static String assertJQ(SolrQueryRequest req,
double delta, String... test
}
}
+ public static String assertThatJQ(SolrQueryRequest req, Matcher test)
throws Exception {
+return assertThatJQ(req, "", test);
+ }
+
+ /**
+ * Validates a query completes and, using JSON deserialization, returns an
object that passes the
+ * given Matcher test.
+ *
+ * Please use this with care: this makes it easy to match complete
structures, but doing so can
+ * result in fragile tests if you are matching more than what you want to
test.
+ *
+ * @param req Solr request to execute
+ * @param message Failure message for test
+ * @param test Matcher for the given object returned from deserializing the
response
+ * @return The request response as a JSON String if the test matcher passes
+ */
+ @SuppressWarnings("unchecked")
+ public static String assertThatJQ(SolrQueryRequest req, String message,
Matcher test)
Review 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.
##
solr/test-framework/src/java/org/apache/solr/util/SolrMatchers.java:
##
@@ -0,0 +1,81 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.util;
+
+import java.util.List;
+import org.hamcrest.Description;
+import org.hamcrest.Matcher;
+import org.hamcrest.TypeSafeDiagnosingMatcher;
+
+public class SolrMatchers {
Review Comment:
add a bit of javadoc please. Like reference "Hamcrest", and javadoc on the
subListMatches method.
##
solr/test-framework/src/java/org/apache/solr/SolrTestCaseJ4.java:
##
@@ -1027,6 +1028,64 @@ public static String assertJQ(SolrQueryRequest req,
double delta, String... test
}
}
+ public static String assertThatJQ(SolrQueryRequest req, Matcher test)
throws Exception {
+return assertThatJQ(req, "", test);
+ }
+
+ /**
+ * Validates a query completes and, using JSON deserialization, returns an
object that passes the
+ * given Matcher test.
+ *
+ * Please use this with care: this makes it easy to match complete
structures, but doing so can
+ * result in fragile tests if you are matching more than what you want to
test.
+ *
+ * @param req Solr request to execute
+ * @param message Failure message for test
+ * @param test Matcher for the given object returned from deserializing the
response
+ * @return The request response as a JSON String if the test matcher passes
+ */
+ @SuppressWarnings("unchecked")
+ public static String assertThatJQ(SolrQueryRequest req, String message,
Matcher test)
+ throws Exception {
+SolrParams params = null;
+try {
+ params = req.getParams();
Review Comment:
you could get & set at the declaration rather than split across the `try`.
Could then be final.
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
