SOLR-15788: Remove the use of legacyExampleCollection1SolrHome() in test setup by epugh · Pull Request #4147 · apache/solr

epugh · 2026-02-19T15:29:00Z

https://issues.apache.org/jira/browse/SOLR-15788

Description

Remove the use of legacyExampleCollection1SolrHome() in test setup in favour of our solrTestRule methods.

Solution

Back in 2021 I opened #410 and it was one of my first attempts to make sense of how crazy the Solr code base was! It never landed because it just didn't seem better.

Fastforward 5 years almost, and we have better ways of doing things. Turns our SolrTestRule's give us helpful builder methods that made it easy to remove this old legacy setup method. And simplified some tests too!

Tests

Existing tests pass.

…eprecated calls legacyExampleCollection1SolrHome() being called but didn't offer anything that just using solrTestRule.newCollection methods didn't provide!

Remove the use of custom setup code in favour of our solrTestRule methods.

epugh · 2026-02-19T15:33:50Z

@dsmiley we are sprinkling around EnvUtils.setProperty( ALLOW_PATHS_SYSPROP, ExternalPaths.SERVER_HOME.toAbsolutePath().toString()); a lot, I wonder if this should be set in a base class like SolrTestCase? Or we live with it...

epugh · 2026-02-19T15:35:04Z

solr/core/src/test/org/apache/solr/handler/admin/ShowFileRequestHandlerTest.java

@@ -119,26 +131,17 @@ public Set<String> getContentTypes() {
  }

  public void testContentTypeHtmlBecomesTextPlain() {


This method demonstrates what we have to start doing to get rid of h.getCore...

epugh · 2026-02-19T15:38:01Z

solr/core/src/test/org/apache/solr/handler/admin/ShowFileRequestHandlerTest.java

-        (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")) {


whyis this called "collection1"? that's ugly. SHouldn't it be called "core1"?

quite a while ago, we standardized on "collection1" no matter what Solr's mode is. I like it.

dsmiley

Thanks for working on this!

IMO, no test should need to set EnvUtils.setProperty(ALLOW_PATHS_SYSPROP ) since our tests shouldn't be constrained by a production concern. Our test infrastructure, on the other hand, may very well need to configure allow-paths in some way (or disable it).

dsmiley · 2026-02-19T16:34:33Z

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


you have made this comment obsolete (thankfully)

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?

maybe.. I was trying not to get too frisky with changing the tests to be better, I was focused on just the migration. I DO think we could tee up a whole set of analysis to run... What tests add value? what tests are duplicative? what tests can be simplified... let's get 5000 bucks of claude credits and have him go to town.

understood; let's not go to far afield in this PR

dsmiley · 2026-02-19T16:37:43Z

solr/core/src/test/org/apache/solr/handler/admin/ShowFileRequestHandlerTest.java

-        (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")) {


quite a while ago, we standardized on "collection1" no matter what Solr's mode is. I like it.

dsmiley · 2026-02-19T16:40:44Z

solr/solrj/src/test/org/apache/solr/client/solrj/embedded/SolrExampleEmbeddedTest.java

 */
-public class SolrExampleEmbeddedTest extends SolrExampleTests {
-
-  @BeforeClass


I'm confused that you removed this. Look at the name of this test. This test should be using EmbeddedSolrServer in some way.

Yeah, I don't know, because the line solrTestRule.startSolr(legacyExampleCollection1SolrHome()); doesn't start up solr... I looked a bit, and I think this class has been suing a solrTestRule that was inherited from SolrExampleTestsBase, and there it was defined as a SolrJettyTestRule. I will look about overriding the @ClassRule and see if we can have a EmbeddedSolrServerTestRule here...

Ah. I did a refactor and pushed it to this PR. Renamed this as I think this test is the best test to test EmbeddedSolrServer itself. Using EmbeddedSolrServer in createNewClient.

solr/solrj/src/test/org/apache/solr/client/solrj/embedded/SolrExampleJettyTest.java

solr/solrj/src/test/org/apache/solr/client/solrj/embedded/SolrExampleStreamingHttp2Test.java

dsmiley · 2026-02-19T16:46:00Z

solr/solrj/src/test/org/apache/solr/client/solrj/SolrExampleTests.java

+    client.deleteByQuery(DEFAULT_TEST_COLLECTION_NAME, "*:*");
    client.commit();


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

It's funny that everyting is passing with the issue on the client commit.... I kind of think maybe we just need to bite bullet and make core speicfic api calls require a core. collection specific api calls require a collection. And node ones not require either!

I asked claude and it turns out because the client was created with a default collection, if i pass it in great, and if I don't that's fine too. So, do we have a preference? I could move the tests to always taking a collection name, or maybe remove them everywhere, just to pick one of the other.

for a given test, either embrace the default if you can (ideal), or thoroughly pass the collection/core with every SolrJ call (which is freaking annoying, granted -- I don't like our API). Don't mix the two approaches!

…st.java to become EmbeddedSolrServerTest.java And Use EmbeddedSolrServer in createNewSolrClient.

epugh added 4 commits February 19, 2026 09:14

Do the migration, with some claude help.

019c088

and remove a test that doesnt' actually test anything unique!

834b060

Moving the shared collection set up to SolrExampleTests eliminating d…

79f1c66

…eprecated calls legacyExampleCollection1SolrHome() being called but didn't offer anything that just using solrTestRule.newCollection methods didn't provide!

Remove the use of legacyExampleCollection1SolrHome() in test setup

11c9590

Remove the use of custom setup code in favour of our solrTestRule methods.

epugh requested a review from dsmiley February 19, 2026 15:29

github-actions bot added test-framework client:solrj tests cat:cli labels Feb 19, 2026

epugh mentioned this pull request Feb 19, 2026

SOLR-15788 Remove configset/sample_techproducts_config from being used in unit tests #410

Closed

7 tasks

epugh added the no-changelog label Feb 19, 2026

epugh commented Feb 19, 2026

View reviewed changes

dsmiley reviewed Feb 19, 2026

View reviewed changes

Rename/combine TestEmbeddedSolrServer.java with SolrExampleEmbeddedTe…

a22fea3

…st.java to become EmbeddedSolrServerTest.java And Use EmbeddedSolrServer in createNewSolrClient.

		@@ -119,26 +131,17 @@ public Set<String> getContentTypes() {
		}

		public void testContentTypeHtmlBecomesTextPlain() {

		@@ -71,14 +80,17 @@ public void test404Locally() {

		// bypass TestHarness since it will throw any exception found in the

		client.deleteByQuery(DEFAULT_TEST_COLLECTION_NAME, ":");
		client.commit();

Conversation

epugh commented Feb 19, 2026

Description

Solution

Tests

Uh oh!

epugh commented Feb 19, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dsmiley left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Comments