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']")
There was a problem hiding this comment.
I ended up going the reqWithPath route here.
To answer my earlier question:SQR.getPath is used in production code but prior to this PR wasn't really used anywhere in tests, but there doesn't (afaict) appear to be any driving reason for that - it's just a consequence of how 'assertQ' and other helpers call TestHarness.query() rather than submitting a full HTTP request and utilizing Solr servers "natural" dispatching code.
I guess I don't think they are terrible... |
| CommonParams.FQ, "str_s:b"), | ||
| reqWithPath( | ||
| "/elevate", | ||
| CommonParams.Q, |
There was a problem hiding this comment.
I keep trying to combine the param name+value into a single line in these invocations, but tidy really doesn't like it.
Not sure if it's a line length thing or something else; kindof a bummer : (
|
Alright, I've reworked this a bit into a setup I'm happier with. Since assertQ and others already take in a SolrQueryRequest, we now rely on that object to provide the handler/path. This saves us from needing to create a bunch of overloads of assertQ. A conceptually cleaner solution, and should scale better as we won't need to duplicate tons of test helpers. Taking it out of "draft" |
|
('check' and tests pass locally) |
| * Validates a query against the named handler matches some XPath test expressions and closes the | ||
| * query | ||
| * The handler that should process {@code req}: its {@link SolrQueryRequest#getPath()} if set, | ||
| * otherwise falls back to the deprecated "qt" request param. |
There was a problem hiding this comment.
out of curiosity, would it make sense to have some sort of warning be logged about using the deprecated qt? assuming you don't finish the migration, then others might see the message and pick up on it?
Kind of like we emit a warning for deprecated system environment names.
Though with the verbosity that our tests have, maybe no one sees it...
There was a problem hiding this comment.
Good question, idk.
If we do anything, I think deprecating CommonParams.QT might be a better route.
That would gives devs a visible signal in their IDE's without being a massive source of noise in the tests. If we logged a warning right now I think it'd be very very erbose. There's still > 500 usages of this in test helpers, let alone any production code that's currently using it that the tests would trigger. A log.warn seems better once (a) the volume is much lower than it currently is and (b) we've got a plan for the remaining usages of QT on the server side and are certain that the deprecation will "stick".
There was a problem hiding this comment.
Yeah... We actually DID have CommonParams.QT deprecated, and then it got undeprecated a few months ago due (some reasons I don't recall right now! ) Maybe streaming and sql usage? But they have been fixed..
agreed we have to have a plan for the production code... I'm assuming these test changes are pretty mechanical?
| boolean failed = true; | ||
| try { | ||
| response = h.query(req); | ||
| response = h.query(handler, req); |
There was a problem hiding this comment.
I see you are updating many of this methods to pass the handler to TestHarness.query. Why not update TestHarness in one spot instead of all these callers?
There was a problem hiding this comment.
I'm not sure I follow - can you be a bit more explicit?
| * | ||
| * @see #req(String...) | ||
| */ | ||
| public static SolrQueryRequest reqWithPath(String path, String... params) { |
There was a problem hiding this comment.
nice.
FWIW I find params to be easy and I like the explicitness of separation with other params. So if you were to simply have a req(String,SolrParams) (caller in practice then uses params(...) -- IMO that'd be fine.
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.