SOLR-18332: More qt-removal from tests, rd 3 - #4721
Conversation
The 'qt' parameter and several related methods in SolrJ are deprecated. This deprecation may not stick, but it's still worth minimizing use of this feature as much as possible. Many tests rely on it unnecessarily; this PR is one in a number of batches slowly removing these usages. This one focuses on solr-core tests that dispatch through the req()/assertQ/assertJQ/assertQEx helpers; passing the handler explicitly instead of embedding it as a 'qt' request param.
|
Not quite ready for primetime yet - I don't like all of the method overloads for Ideas welcome! 🤔 |
| // now switch the order: | ||
| booster.setTopQueryResults(reader, query, false, new String[] {"a", "x"}, null); | ||
| assertQ( | ||
| null, |
There was a problem hiding this comment.
Is this null because you didn't want to add a message on failure?
There was a problem hiding this comment.
In short, yes. This is the main thing I dislike about this PR.
I'm 100% happy with the default assertQ error message here. In an ideal world I'd create an assertQ override for cases like this, where I need a non-default query endpoint but am 100% happy with the default assertQ error message. But there's no great way to do that since "message" and "requestHandler" are both strings and the signature of the reqHandler-but-no-message method would by indistinguishable from the existing "message-but-no-reqHandler" method.
I toyed a bit with a fluent-ish solution here, where the message gets provided by a wrapping call and could be dropped from all of our little assertFoo helpers. e.g.
withMessage(
"This is my message on failure",
() -> assertQ(...))
Another alternative: assertQ and friends already take in a SolrQueryRequest (typically created via req() calls) and SQR has a getPath method that could probably be used rather than providing the requestHandler as a separate method param in assertQ. This seems like the simplest solution, except that SolrQueryRequest.getPath seems to be largely unused, and I don't have quite enough context to know why. If we could go this route it'd look like:
assertQ(
reqWithPath("/elevate", baseParams),
"//([@numFound='4']")
| @@ -1663,9 +1598,9 @@ public void testOnlyRepresentativeIsVisibleWhenCollapsing() throws Exception { | |||
| // only representative elevated doc visible | |||
| assertQ( | |||
| "", | |||
There was a problem hiding this comment.
I think it's worth adding a proper message in for the null or "", and the failure messages help you understand what the test is about..
There was a problem hiding this comment.
In theory, I agree. In practice:
- It's hard to write a good error message for an assertion like this that's running many XPath checks. Taking this particular case as an example: what would a good message look like, that's better than what assertQ does by default (which prints both the XPath expression that caused the failure and corresponding section of the results)?
- Bandwidth. If I stop to spruce up every little thing that could be improved I'm worried the larger qt effort will never get across the line.
There was a problem hiding this comment.
Fair enough. In this case, perfection is the enemy of ever getting to the end of this effort ;-)
I guess I don't think they are terrible... |
| String exceptionMessage, | ||
| SolrQueryRequest req, | ||
| SolrException.ErrorCode code, | ||
| String handler) { |
There was a problem hiding this comment.
It's a matter of taste but shouldn't the handler & request come first?
| @Test | ||
| public void testBoolOuterAndInnerNamesComposed() throws Exception { | ||
| assertJQ( | ||
| HANDLER, |
There was a problem hiding this comment.
maybe instead of overloading various assertion methods, we add a wrapping method like handler(String,SolrQueryRequest) that puts the handler into the SQR, like in the context I guess. Could use "qt". The ultimate point is to get the tests to not use "qt"; it's not wrong if we find it convenient for the plumbing/infra to maybe use it behind the scenes.
3 participants
The 'qt' parameter and several related methods in SolrJ are deprecated.
This deprecation may not stick, but it's still worth minimizing use of this
feature as much as possible.
Many tests rely on it unnecessarily; this PR is one in a number of batches
slowly removing these usages. This one focuses on solr-core tests that
dispatch through the req()/assertQ/assertJQ/assertQEx helpers; passing the
handler explicitly instead of embedding it as a 'qt' request param.