Hi David, Thanks for bringing the PR up for attention here. Had a few thoughts on the setter and build() naming, as you called out, and commented on those in the PR. But otherwise, it looks very nice so far!
Best, Jason On Tue, Jan 3, 2023 at 1:06 AM David Smiley <dsmi...@apache.org> wrote: > Happy New Year everyone. > > Joshua and I have some good progress on a new EmbeddedSolrServerTestRule > https://issues.apache.org/jira/browse/SOLR-16573 + PR (read the JIRA > first, > always) > Keep in mind this is one incremental step, and I expect it will be > evolutionary (The API will evolve as we address more tests for more > scenarios). > > Here's an example from RootFieldTest, which subclasses > EmbeddedSolrServerTestBase: > > @BeforeClass > public static void beforeTest() throws Exception { > useRootSchema = random().nextBoolean(); > // schema15.xml declares _root_ field, while schema-rest.xml does not. > String schema = useRootSchema ? "schema15.xml" : "schema-rest.xml"; > SolrTestCaseJ4.newRandomConfig(); > solrClientTestRule > .build() > .setSolrHome(Path.of(SolrTestCaseJ4.TEST_HOME())) > .useTempDataDir() > .setSchemaFile(schema) > .init(); > } > > > The "set" needs to become "with" to match the builder style. > The ".build()" method instead of calling a constructor of an inner Builder > is debatable; I like this FWIW. The constructor of this thing does > nothing; it's initialized later by the test using it as seen above. The > rule is declared like: > > @ClassRule > public static EmbeddedSolrServerTestRule solrClientTestRule = new > EmbeddedSolrServerTestRule(); > > I had initially wanted to initialize this thing at the spot it's declared > instead of having an instance that isn't yet initialized. But it's common > for the construction to specify a bunch of things that might in turn access > the randomized context (e.g. LuceneTestCase.createTempDir() will, > surprisingly). But for a statically declared field, the randomized context > doesn't exist yet in the lifecycle of a test. So we use the builder > pattern post-construction for the initialization. > > After Joshua converts some more tests, the PR will be ready. > Maybe EmbeddedSolrServerTestBase should be removed here as well, as it'll > do nothing other than declare this rule and a delegating method to get the > client. > > After this issue, we should do an equivalent implementation for HTTP / > Jetty. > > ~ David Smiley > Apache Lucene/Solr Search Developer > http://www.linkedin.com/in/davidwsmiley >
