-
Notifications
You must be signed in to change notification settings - Fork 804
SOLR-18068: POC of keeping existing assertQ logic with solrTestRule #4042
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
| @BeforeClass | ||
| public static void beforeClass() throws Exception { | ||
| initCore("solrconfig-phrasesuggest.xml", "schema-phrasesuggest.xml"); | ||
| assertQ(req("qt", URI, "q", "", SpellingParams.SPELLCHECK_BUILD, "true")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to move the assertQ into the public void test() because appparently this is static and assertQ and req couldn't be called from a static method.
dsmiley
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for starting work on this!
| solrTestRule | ||
| .newCollection() | ||
| .withConfigSet("../collection1") | ||
| .withConfigFile("conf/solrconfig-phrasesuggest.xml") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is that leading "conf" really necessary? I hope not. Something to improve... the presence of those intermediate conf directories is a sad accident of history.
| QueryResponse rsp = req.process(getSolrClient()); | ||
| NamedList<Object> rawResponse = rsp.getResponse(); | ||
| InputStream stream = (InputStream) rawResponse.get("stream"); | ||
| String response = new String(stream.readAllBytes(), StandardCharsets.UTF_8); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are assuming an InputStreamResponseParser requesting XML but don't see the logic for it.
| } | ||
|
|
||
| public SolrClient getSolrClient(){ | ||
| throw new RuntimeException("This method needs to be overridden"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then make it abstract?!
| throw new RuntimeException("XPath is invalid", e1); | ||
| } catch (Throwable e3) { | ||
| log.error("REQUEST FAILED: {}", req.getParams(), e3); | ||
| throw new RuntimeException("Exception during query", e3); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not assertionFailure?
| // This was part of the SolrTestCaseJ4.setupTestCases method and appears to be needed. Ugh. | ||
| // Is this a direction we want, this randomness and need in SolrTestCase? | ||
| SolrTestCaseJ4Test.newRandomConfig(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, this is a struggle/tension. On one hand, I want to see SolrTestCase leaner; not a kitchen sink that STCJ4 became. And I want it to be usable by 3rd parties as well. Including logic for newRandomConfig definitely fails the latter. Maybe we need another subclass for first-party. Or have STC be able to recognize 1P from 3P and be graceful... which I think for some specific methods it may (and/or STCJ4 does. TEST_HOME() is one). Or maybe put certain functionality as static methods in other classes, like TEST_HOME() -- like a 1PTest.TEST_Home().
| if (path != null) { | ||
| req.setPath(path); | ||
| } | ||
| req.setResponseParser(new InputStreamResponseParser("xml")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO this doesn't belong here.
| params.set("wt", "xml"); | ||
| params.set("indent", params.get("indent", "off")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wt should not be be set since it's the job of the ResponseParser to declare it's wt
| "The length of the string array (query arguments) needs to be even"); | ||
| } | ||
| for (int i = 0; i < q.length; i += 2) { | ||
| params.set(q[i], q[i + 1]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use "add" not "set" since params can repeat
| QueryResponse rsp = req.process(getSolrClient()); | ||
| NamedList<Object> rawResponse = rsp.getResponse(); | ||
| InputStream stream = (InputStream) rawResponse.get("stream"); | ||
| String response = new String(stream.readAllBytes(), StandardCharsets.UTF_8); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
call this responseXml to clearly show it's format
https://issues.apache.org/jira/browse/SOLR-18068
Description
Experimenting with how to move us from extending SolrTestCaseJ4 and using
initCoreto using a SolrTestRuleSolution
I didn't want to change any of the assert logic like:
It is all xpath based, and to change that to the json equivalents would be a lot of error prone work.
I introduced a class
SolrXPathTestCasebetweenTestPhraseSuggestionsandSolrTestCase, cutting out the old extension onSolrTestCaseJ4. I then added thereqandassertQmethods to that class, but with the various new types.Tests
Modified existing test and made sure it still passes.