gerlowskija commented on code in PR #1218: URL: https://github.com/apache/solr/pull/1218#discussion_r1060810965
########## solr/test-framework/src/java/org/apache/solr/util/EmbeddedSolrServerTestRule.java: ########## @@ -0,0 +1,187 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.solr.util; + +import static org.apache.solr.SolrTestCaseJ4.DEFAULT_TEST_COLLECTION_NAME; + +import java.io.IOException; +import java.nio.file.Path; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import org.apache.lucene.tests.util.LuceneTestCase; +import org.apache.solr.client.solrj.embedded.EmbeddedSolrServer; +import org.apache.solr.client.solrj.embedded.EmbeddedSolrServer.RequestWriterSupplier; +import org.apache.solr.client.solrj.impl.HttpClientUtil; +import org.apache.solr.core.CoreContainer; +import org.apache.solr.core.CoreDescriptor; +import org.apache.solr.core.NodeConfig; +import org.apache.solr.update.UpdateShardHandlerConfig; + +/** TODO NOCOMMIT document */ +public class EmbeddedSolrServerTestRule extends SolrClientTestRule { + + private EmbeddedSolrServer client = null; + + public Builder build() { + return new Builder(); + } + + public class Builder { + private Path solrHome; // mandatory + private Path dataDir; + private String collectionName = DEFAULT_TEST_COLLECTION_NAME; + private String configFile; + private String schemaFile; + private RequestWriterSupplier requestWriterSupplier = RequestWriterSupplier.JavaBin; + + public Builder setSolrHome(Path solrHome) { + this.solrHome = solrHome; + return this; + } + + public Builder useTempDataDir() { + this.dataDir = LuceneTestCase.createTempDir("data-dir"); + return this; + } + + public Builder setSchemaFile(String schemaFile) { Review Comment: [0] Eric Pugh has been working recently on a Builder pattern for creating Solr clients that has methods with names like: `withFoo`, `withBar`, etc. I love that you're consistent across this Builder here with using `set`. But is there any value in being consistent with the SolrClient builders and using `with` instead of `set`? (Maybe there's not - solrj and solr-test-framework are different animals. Just bringing it up for conversation in case it hasn't been raised yet.) ########## solr/test-framework/src/java/org/apache/solr/util/EmbeddedSolrServerTestRule.java: ########## @@ -0,0 +1,187 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.solr.util; + +import static org.apache.solr.SolrTestCaseJ4.DEFAULT_TEST_COLLECTION_NAME; + +import java.io.IOException; +import java.nio.file.Path; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import org.apache.lucene.tests.util.LuceneTestCase; +import org.apache.solr.client.solrj.embedded.EmbeddedSolrServer; +import org.apache.solr.client.solrj.embedded.EmbeddedSolrServer.RequestWriterSupplier; +import org.apache.solr.client.solrj.impl.HttpClientUtil; +import org.apache.solr.core.CoreContainer; +import org.apache.solr.core.CoreDescriptor; +import org.apache.solr.core.NodeConfig; +import org.apache.solr.update.UpdateShardHandlerConfig; + +/** TODO NOCOMMIT document */ +public class EmbeddedSolrServerTestRule extends SolrClientTestRule { + + private EmbeddedSolrServer client = null; + + public Builder build() { Review Comment: [0] Definitely getting into "personal-preference territory" here, but I don't love the `build()/init()` method names that are used to create the builder and finish its initialization. In most of the builders I've worked with (e.g. [ToStringBuilder](https://commons.apache.org/proper/commons-lang/apidocs/org/apache/commons/lang3/builder/ToStringBuilder.html#build--), [HttpClientBuilder](https://hc.apache.org/httpcomponents-client-4.5.x/current/httpclient/apidocs/org/apache/http/impl/client/HttpClientBuilder.html#build()), and of course [HttpSolrClient.Builder](https://solr.apache.org/docs/7_4_0/solr-solrj/org/apache/solr/client/solrj/impl/HttpSolrClient.Builder.html#build--)), `build()` is the final method in the call chain that takes all of the provided settings and uses them to actually create an instance of the object in question. Using `build()` to create the builder itself breaks that common convention and (I think) will be confusing for readers/users. Is there a good reason we're not just sticking to the common convention here? ########## solr/solrj/src/test/org/apache/solr/client/solrj/response/TestSpellCheckResponse.java: ########## @@ -37,14 +40,18 @@ public class TestSpellCheckResponse extends EmbeddedSolrServerTestBase { @BeforeClass public static void beforeClass() throws Exception { - initCore(); + solrClientTestRule + .build() + .setSolrHome(Paths.get(SolrJettyTestBase.legacyExampleCollection1SolrHome())) + .init(); } static String field = "name"; + SolrClient client = getSolrClient(); Review Comment: [0] I don't think it's _wrong_ per se, but I'd rather see this in a method (maybe at the end of `beforeClass` or in a separate `@Before` annotated method? Not a strong opinion though; if there's a reason you've done it this way then ignore me. ########## solr/test-framework/src/java/org/apache/solr/util/EmbeddedSolrServerTestRule.java: ########## @@ -0,0 +1,187 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.solr.util; + +import static org.apache.solr.SolrTestCaseJ4.DEFAULT_TEST_COLLECTION_NAME; + +import java.io.IOException; +import java.nio.file.Path; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import org.apache.lucene.tests.util.LuceneTestCase; +import org.apache.solr.client.solrj.embedded.EmbeddedSolrServer; +import org.apache.solr.client.solrj.embedded.EmbeddedSolrServer.RequestWriterSupplier; +import org.apache.solr.client.solrj.impl.HttpClientUtil; +import org.apache.solr.core.CoreContainer; +import org.apache.solr.core.CoreDescriptor; +import org.apache.solr.core.NodeConfig; +import org.apache.solr.update.UpdateShardHandlerConfig; + +/** TODO NOCOMMIT document */ +public class EmbeddedSolrServerTestRule extends SolrClientTestRule { + + private EmbeddedSolrServer client = null; + + public Builder build() { + return new Builder(); + } + + public class Builder { + private Path solrHome; // mandatory + private Path dataDir; + private String collectionName = DEFAULT_TEST_COLLECTION_NAME; + private String configFile; + private String schemaFile; + private RequestWriterSupplier requestWriterSupplier = RequestWriterSupplier.JavaBin; + + public Builder setSolrHome(Path solrHome) { + this.solrHome = solrHome; + return this; + } + + public Builder useTempDataDir() { Review Comment: [Q] Maybe trimming down the functionality exposed is beyond scope of this issue/PR, since at this point we need to support the tests that exist currently. But I am curious: what's the value of exposing this option to users? Do tests actually care about where data lives, or is this just something that has historically been exposed to support tests that need multiple EmbeddedSolrServers? If the latter, could we just always use a temp-dir and eliminate this method entirely? -- 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: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org
