SOLR-15788: Remove the use of legacyExampleCollection1SolrHome() in test setup #4147
SOLR-15788: Remove the use of legacyExampleCollection1SolrHome() in test setup #4147epugh wants to merge 8 commits intoapache:mainfrom
Conversation
…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.
|
@dsmiley we are sprinkling around |
| @@ -119,26 +131,17 @@ public Set<String> getContentTypes() { | |||
| } | |||
|
|
|||
| public void testContentTypeHtmlBecomesTextPlain() { | |||
There was a problem hiding this comment.
This method demonstrates what we have to start doing to get rid of h.getCore...
| (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")) { |
There was a problem hiding this comment.
whyis this called "collection1"? that's ugly. SHouldn't it be called "core1"?
There was a problem hiding this comment.
quite a while ago, we standardized on "collection1" no matter what Solr's mode is. I like it.
dsmiley
left a comment
dsmiley
left a comment
There was a problem hiding this comment.
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).
solr/core/src/test/org/apache/solr/handler/admin/ShowFileRequestHandlerTest.java
Outdated
Show resolved
Hide resolved
| (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")) { |
There was a problem hiding this comment.
quite a while ago, we standardized on "collection1" no matter what Solr's mode is. I like it.
solr/solrj/src/test/org/apache/solr/client/solrj/embedded/SolrExampleEmbeddedTest.java
Show resolved
Hide resolved
solr/solrj/src/test/org/apache/solr/client/solrj/embedded/SolrExampleJettyTest.java
Show resolved
Hide resolved
solr/solrj/src/test/org/apache/solr/client/solrj/embedded/SolrExampleStreamingHttp2Test.java
Show resolved
Hide resolved
solr/solrj/src/test/org/apache/solr/client/solrj/SolrExampleTests.java
Outdated
Show resolved
Hide resolved
…st.java to become EmbeddedSolrServerTest.java And Use EmbeddedSolrServer in createNewSolrClient.
|
Okay, I think I've dealt with all the feedback. Thanks @dsmiley for fixing that test.. I will aim to merge this on Monday, leaving it open a few days more. Nice clean up! |
dsmiley
left a comment
dsmiley
left a comment
There was a problem hiding this comment.
approving... but maybe we want to remove the need for ALLOW_PATHS_SYSPROP in tests first.
They are in lots of other places as well across our test code... How about you create a JIRA and I'll make a dedicated PR? |
2 participants
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.