risdenk commented on code in PR #1158:
URL: https://github.com/apache/solr/pull/1158#discussion_r1014303007
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/BasicHttpSolrClientTest.java:
##########
@@ -476,9 +484,20 @@ public void testUpdate() throws Exception {
// parameter encoding
assertEquals(1, DebugServlet.parameters.get("a").length);
assertEquals("\u1234", DebugServlet.parameters.get("a")[0]);
+ }
+ DebugServlet.clear();
+ // XML response and writer
+ try (HttpSolrClient client =
+ new HttpSolrClient.Builder(url)
+ .withRequestWriter(new RequestWriter())
+ .withResponseParser(new XMLResponseParser())
+ .build()) {
+ UpdateRequest req = new UpdateRequest();
+ req.add(new SolrInputDocument());
+ req.setParam("a", "\u1234");
- // XML response and writer
- client.setParser(new XMLResponseParser());
+ // Have not been able to move this to the Builder pattern, the below
check for application/xml
+ // always returns as application/javabin when .setRequestWriter() is
commented out.
Review Comment:
Needs to be looked at again.
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/Http2SolrClientTest.java:
##########
@@ -360,7 +359,7 @@ public void testQuery() throws Exception {
assertEquals(EXPECTED_USER_AGENT,
DebugServlet.headers.get("user-agent"));
// XML/POST
- client.setParser(new XMLResponseParser());
+ // client.setParser(new XMLResponseParser());
Review Comment:
This can be removed?
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/Http2SolrClientTest.java:
##########
@@ -379,7 +378,7 @@ public void testQuery() throws Exception {
assertEquals(EXPECTED_USER_AGENT,
DebugServlet.headers.get("user-agent"));
assertEquals("application/x-www-form-urlencoded",
DebugServlet.headers.get("content-type"));
- client.setParser(new XMLResponseParser());
+ // client.setParser(new XMLResponseParser());
Review Comment:
This can be removed?
##########
solr/test-framework/src/java/org/apache/solr/SolrTestCaseJ4.java:
##########
@@ -2562,20 +2562,20 @@ public static Object skewed(Object likely, Object
unlikely) {
* A variant of {@link
org.apache.solr.client.solrj.impl.CloudHttp2SolrClient.Builder} that will
* randomize some internal settings.
*/
- public static class CloudHttp2SolrClientBuilder extends
CloudHttp2SolrClient.Builder {
+ public static class RandomizingCloudHttp2SolrClientBuilder extends
CloudHttp2SolrClient.Builder {
Review Comment:
Since this name change didn't actually affect any other files - is this
class even used???
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/Http2SolrClientTest.java:
##########
@@ -518,10 +527,16 @@ public void testUpdate() throws Exception {
assertEquals("application/xml; charset=UTF-8",
DebugServlet.headers.get("content-type"));
assertEquals(1, DebugServlet.parameters.get("a").length);
assertEquals("\u1234", DebugServlet.parameters.get("a")[0]);
+ }
+ try (Http2SolrClient client =
+ new Http2SolrClient.Builder(url)
+ .withRequestWriter(new BinaryRequestWriter())
+ .withResponseParser(new BinaryResponseParser())
+ .build()) {
// javabin request
- client.setParser(new BinaryResponseParser());
- client.setRequestWriter(new BinaryRequestWriter());
+ // client.setParser(new BinaryResponseParser());
+ // client.setRequestWriter(new BinaryRequestWriter());
Review Comment:
This can be removed?
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/Http2SolrClientTest.java:
##########
@@ -499,10 +502,16 @@ public void testUpdate() throws Exception {
// parameter encoding
assertEquals(1, DebugServlet.parameters.get("a").length);
assertEquals("\u1234", DebugServlet.parameters.get("a")[0]);
+ }
+ try (Http2SolrClient client =
+ new Http2SolrClient.Builder(url)
+ .withRequestWriter(new RequestWriter())
+ .withResponseParser(new XMLResponseParser())
+ .build()) {
// XML response and writer
- client.setParser(new XMLResponseParser());
- client.setRequestWriter(new RequestWriter());
+ // client.setParser(new XMLResponseParser());
+ // client.setRequestWriter(new RequestWriter());
Review Comment:
This can be removed?
##########
solr/test-framework/src/java/org/apache/solr/SolrTestCaseJ4.java:
##########
@@ -2872,9 +2872,16 @@ public static LBHttpSolrClient
getLBHttpSolrClient(String... solrUrls)
}
/**
- * This method <i>may</i> randomize unspecified aspects of the resulting
SolrClient. Tests that do
- * not wish to have any randomized behavior should use the {@link
Review Comment:
Hmm I wonder if there should be more randomization here? I think the keyword
is `may` but not yet? If we had tests use these methods we could randomize
stuff easier?
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/Http2SolrClientTest.java:
##########
@@ -542,7 +557,32 @@ public void testUpdate() throws Exception {
}
@Test
- public void testRedirect() throws Exception {
+ public void testFollowRedirect() throws Exception {
+ final String clientUrl = jetty.getBaseUrl().toString() + "/redirect/foo";
+ try (Http2SolrClient client =
+ new
Http2SolrClient.Builder(clientUrl).withFollowRedirects(true).build()) {
+ SolrQuery q = new SolrQuery("*:*");
+ client.query(q);
+ }
+ }
+
+ @Test
+ public void testDoNotFollowRedirect() throws Exception {
+ final String clientUrl = jetty.getBaseUrl().toString() + "/redirect/foo";
+ try (Http2SolrClient client =
+ new
Http2SolrClient.Builder(clientUrl).withFollowRedirects(false).build()) {
+ SolrQuery q = new SolrQuery("*:*");
+ try {
+ client.query(q);
+ fail("Should have thrown an exception.");
Review Comment:
Better to use `assertThrows` if possible.
##########
solr/solrj/src/test/org/apache/solr/client/solrj/SolrExampleTests.java:
##########
@@ -602,58 +602,60 @@ public void testUnicode() throws Exception {
SolrClient client = getSolrClient();
// save the old parser, so we can set it back.
- ResponseParser oldParser = null;
- if (client instanceof HttpSolrClient) {
- HttpSolrClient httpSolrClient = (HttpSolrClient) client;
- oldParser = httpSolrClient.getParser();
- }
-
- try {
- for (int iteration = 0; iteration < numIterations; iteration++) {
- // choose format
- if (client instanceof HttpSolrClient) {
- if (random.nextBoolean()) {
- ((HttpSolrClient) client).setParser(new BinaryResponseParser());
- } else {
- ((HttpSolrClient) client).setParser(new XMLResponseParser());
- }
- }
-
- int numDocs = TestUtil.nextInt(random(), 1, 10 * RANDOM_MULTIPLIER);
-
- // Empty the database...
- client.deleteByQuery("*:*"); // delete everything!
-
- List<SolrInputDocument> docs = new ArrayList<>();
- for (int i = 0; i < numDocs; i++) {
- // Now add something...
- SolrInputDocument doc = new SolrInputDocument();
- doc.addField("id", "" + i);
- doc.addField("unicode_s", randomTestString(30));
- docs.add(doc);
- }
+ // ResponseParser oldParser = null;
+ // if (client instanceof HttpSolrClient) {
+ // HttpSolrClient httpSolrClient = (HttpSolrClient) client;
+ // oldParser = httpSolrClient.getParser();
+ // }
Review Comment:
Not sure where my other comment went :) I had one typed up about this but
looks like it went poof.
Since the test is called `testUnicode` - I wonder if
`SolrExampleBinaryTest` or `SolrExampleXMLTest` test unicode? maybe this is
specific to unicode and thats why the parser is randomized?
##########
solr/solrj/src/test/org/apache/solr/client/solrj/TestSolrJErrorHandling.java:
##########
@@ -102,16 +102,19 @@ public void showExceptions() throws Exception {
@Test
public void testWithXml() throws Exception {
- HttpSolrClient client = (HttpSolrClient) getSolrClient();
- client.setRequestWriter(new RequestWriter());
+ client =
+ new HttpSolrClient.Builder(getServerUrl()).withRequestWriter(new
RequestWriter()).build();
+
Review Comment:
`try-with-resources` since we create new clients here.
##########
solr/test-framework/src/java/org/apache/solr/SolrTestCaseJ4.java:
##########
@@ -2562,20 +2562,20 @@ public static Object skewed(Object likely, Object
unlikely) {
* A variant of {@link
org.apache.solr.client.solrj.impl.CloudHttp2SolrClient.Builder} that will
* randomize some internal settings.
*/
- public static class CloudHttp2SolrClientBuilder extends
CloudHttp2SolrClient.Builder {
+ public static class RandomizingCloudHttp2SolrClientBuilder extends
CloudHttp2SolrClient.Builder {
Review Comment:
Name change makes sense.
##########
solr/solrj/src/test/org/apache/solr/client/solrj/embedded/SolrExampleJettyTest.java:
##########
@@ -72,15 +65,15 @@ public void testBadSetup() {
@Test
public void testArbitraryJsonIndexing() throws Exception {
- HttpSolrClient client = (HttpSolrClient) getSolrClient();
+ SolrClient client = getSolrClient();
Review Comment:
should be closed eventually?
##########
solr/solrj/src/test/org/apache/solr/client/solrj/TestBatchUpdate.java:
##########
@@ -46,24 +46,28 @@ public static void beforeTest() throws Exception {
@Test
public void testWithXml() throws Exception {
- HttpSolrClient client = (HttpSolrClient) getSolrClient();
- client.setRequestWriter(new RequestWriter());
+ client =
Review Comment:
I'm of the opinion - we should close the Solr client where we create the
Solr client. Since this change is making new Solr clients for each test - they
should be `try-with-resources`.
##########
solr/prometheus-exporter/src/test/org/apache/solr/prometheus/scraper/SolrStandaloneScraperTest.java:
##########
@@ -63,13 +63,14 @@ public static void setupBeforeClass() throws Exception {
configuration =
Helpers.loadConfiguration("conf/prometheus-solr-exporter-scraper-test-config.xml");
- solrClient = new
Http2SolrClient.Builder(restTestHarness.getAdminURL()).build();
- solrScraper = new SolrStandaloneScraper(solrClient, executor, "test");
-
NoOpResponseParser responseParser = new NoOpResponseParser();
responseParser.setWriterType("json");
Review Comment:
I think this is redundant now?
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/Http2SolrClientTest.java:
##########
@@ -423,9 +422,12 @@ public void testDelete() throws Exception {
client.getParser().getVersion(),
DebugServlet.parameters.get(CommonParams.VERSION)[0]);
// agent
assertEquals(EXPECTED_USER_AGENT,
DebugServlet.headers.get("user-agent"));
+ }
+ // XML
+ try (Http2SolrClient client =
+ new Http2SolrClient.Builder(url).withResponseParser(new
XMLResponseParser()).build()) {
- // XML
- client.setParser(new XMLResponseParser());
+ // client.setParser(new XMLResponseParser());
Review Comment:
This can be removed?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
