dsmiley commented on code in PR #4147:
URL: https://github.com/apache/solr/pull/4147#discussion_r2828933303
##########
solr/solrj/src/test/org/apache/solr/client/solrj/SolrExampleTests.java:
##########
@@ -95,11 +99,22 @@
public abstract class SolrExampleTests extends SolrExampleTestsBase {
private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+ @BeforeClass
+ public static void beforeTest() throws Exception {
+ EnvUtils.setProperty(
+ ALLOW_PATHS_SYSPROP,
ExternalPaths.SERVER_HOME.toAbsolutePath().toString());
+ solrTestRule.startSolr(createTempDir());
+ solrTestRule
+ .newCollection(DEFAULT_TEST_COLLECTION_NAME)
+ .withConfigSet(ExternalPaths.TECHPRODUCTS_CONFIGSET)
+ .create();
+ }
+
@Before
public void emptyCollection() throws Exception {
SolrClient client = getSolrClient();
// delete everything!
- client.deleteByQuery("*:*");
+ client.deleteByQuery(DEFAULT_TEST_COLLECTION_NAME, "*:*");
client.commit();
Review Comment:
be careful! You are referencing a specific collection on line 117 but
neglect to on line 118 (for the commit). This is so subtle.
This is my biggest concern with SolrJ API... making collection name optional
on all the SolrClient methods... it's a mess. IMO there ought to have been a
separate abstraction that has the resolved collection name that you call
various methods on to do collection/core specific operations on. CC
@gerlowskija
##########
solr/core/src/test/org/apache/solr/handler/admin/ShowFileRequestHandlerTest.java:
##########
@@ -71,14 +80,17 @@ public void test404Locally() {
// bypass TestHarness since it will throw any exception found in the
Review Comment:
you have made this comment obsolete (thankfully)
##########
solr/solrj/src/test/org/apache/solr/client/solrj/embedded/SolrExampleJettyTest.java:
##########
@@ -46,11 +45,6 @@
@SuppressSSL(bugUrl = "https://issues.apache.org/jira/browse/SOLR-5776";)
public class SolrExampleJettyTest extends SolrExampleTests {
- @BeforeClass
- public static void beforeTest() throws Exception {
Review Comment:
I'm confused that you removed this. Look at the name of this test. This
test should be using The Jetty test rule in some way.
##########
solr/core/src/test/org/apache/solr/handler/admin/ShowFileRequestHandlerTest.java:
##########
@@ -119,26 +131,17 @@ public Set<String> getContentTypes() {
}
public void testContentTypeHtmlBecomesTextPlain() {
- SolrRequestHandler handler = h.getCore().getRequestHandler("/admin/file");
- SolrQueryRequest req =
- new SolrQueryRequestBase(
- h.getCore(), params("file", "schema.xml", "contentType",
"text/html"));
- SolrQueryResponse rsp = new SolrQueryResponse();
- handler.handleRequest(req, rsp);
- ContentStreamBase.FileStream content =
- (ContentStreamBase.FileStream) rsp.getValues().get("content");
- assertEquals("text/plain", content.getContentType());
- }
-
- public void testContentTypeHtmlDefault() {
- SolrRequestHandler handler = h.getCore().getRequestHandler("/admin/file");
- SolrQueryRequest req = new SolrQueryRequestBase(h.getCore(),
params("file", "example.html"));
- SolrQueryResponse rsp = new SolrQueryResponse();
- handler.handleRequest(req, rsp);
- ContentStreamBase.FileStream content =
- (ContentStreamBase.FileStream) rsp.getValues().get("content");
- // System attempts to guess content type, but will only return XML, JSON,
CSV, never HTML
- assertEquals("application/xml", content.getContentType());
+ try (SolrCore core =
solrTestRule.getCoreContainer().getCore("collection1")) {
Review Comment:
quite a while ago, we standardized on "collection1" no matter what Solr's
mode is. I like it.
##########
solr/core/src/test/org/apache/solr/handler/admin/ShowFileRequestHandlerTest.java:
##########
@@ -71,14 +80,17 @@ public void test404Locally() {
// bypass TestHarness since it will throw any exception found in the
Review Comment:
but moreover, I don't see why this test uses Solr internals. Can't we query
Solr normally / with SolrJ, and check the exception type and check for the 404?
##########
solr/solrj/src/test/org/apache/solr/client/solrj/embedded/SolrExampleEmbeddedTest.java:
##########
@@ -17,17 +17,10 @@
package org.apache.solr.client.solrj.embedded;
import org.apache.solr.client.solrj.SolrExampleTests;
-import org.junit.BeforeClass;
/**
* This runs SolrServer test using
*
* @since solr 1.3
*/
-public class SolrExampleEmbeddedTest extends SolrExampleTests {
-
- @BeforeClass
Review Comment:
I'm confused that you removed this. Look at the name of this test. This
test should be using EmbeddedSolrServer in some way.
##########
solr/solrj/src/test/org/apache/solr/client/solrj/embedded/SolrExampleStreamingHttp2Test.java:
##########
@@ -30,19 +30,13 @@
import org.apache.solr.client.solrj.request.XMLRequestWriter;
import org.apache.solr.client.solrj.response.XMLResponseParser;
import org.apache.solr.common.SolrInputDocument;
-import org.junit.BeforeClass;
/**
* A subclass of SolrExampleTests that explicitly uses the HTTP2 client and
the streaming update
* client for communication.
*/
public class SolrExampleStreamingHttp2Test extends SolrExampleTests {
- @BeforeClass
- public static void beforeTest() throws Exception {
Review Comment:
same
--
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]
