-
Notifications
You must be signed in to change notification settings - Fork 804
SOLR-18067: Migrate tests from EmbeddedSolrServerTestBase to EmbeddedSolrServerTestRule #4041
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR migrates test classes from the deprecated EmbeddedSolrServerTestBase base class to the modern EmbeddedSolrServerTestRule test rule pattern, modernizing the test infrastructure.
Changes:
- Removed the deprecated
EmbeddedSolrServerTestBaseclass - Migrated 8 test classes to use
EmbeddedSolrServerTestRulewith@ClassRule - Updated all references from
getSolrClient()tosolrTestRule.getSolrClient()
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
solr/test-framework/src/java/org/apache/solr/EmbeddedSolrServerTestBase.java |
Removed deprecated base class |
solr/solrj/src/test/org/apache/solr/client/solrj/response/TestSpellCheckResponse.java |
Migrated to use EmbeddedSolrServerTestRule |
solr/solrj/src/test/org/apache/solr/client/solrj/response/TermsResponseTest.java |
Migrated to use EmbeddedSolrServerTestRule |
solr/solrj/src/test/org/apache/solr/client/solrj/request/json/DirectJsonQueryRequestFacetingEmbeddedTest.java |
Migrated to use EmbeddedSolrServerTestRule |
solr/solrj/src/test/org/apache/solr/client/solrj/request/SolrPingTest.java |
Migrated to use EmbeddedSolrServerTestRule |
solr/solrj/src/test/org/apache/solr/client/solrj/LargeVolumeTestBase.java |
Migrated to use EmbeddedSolrServerTestRule |
solr/solrj/src/test/org/apache/solr/client/solrj/GetByIdTest.java |
Migrated to use EmbeddedSolrServerTestRule |
solr/core/src/test/org/apache/solr/update/processor/AbstractAtomicUpdatesMultivalueTestBase.java |
Migrated to use EmbeddedSolrServerTestRule |
solr/core/src/test/org/apache/solr/update/RootFieldTest.java |
Migrated to use EmbeddedSolrServerTestRule |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import org.junit.ClassRule; | ||
|
|
||
| @Deprecated // simply use EmbeddedSolrServerTestRule | ||
| public abstract class EmbeddedSolrServerTestBase extends SolrTestCase { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notice this extends SolrTestCase. I definitely like that; STC is the futre, not STCJ4. Thus the tests you changed should subclass STC, not STCJ4.
| @Before | ||
| public void before() throws SolrServerException, IOException { | ||
| getSolrClient().deleteByQuery("*:*"); | ||
| solrTestRule.getSolrClient().deleteByQuery("*:*"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kinda wonder if STR.client() should exist to shorten our calls a bit more since everyone will want a client from STR; that's it's primary purpose in life.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question!!!
Still have the STCj4 as an import due to various methods called there.
https://issues.apache.org/jira/browse/SOLR-18067
Description
Migrate to modern test infrastructure.
Solution
Asked Claude using same prompt as I used for the SolrJettyTestRule, however this went much faster/much simpler.
Now I get a bit why we like embedded ;-)
Tests
Re-ran the tests.