Skip to content

SolrClientTestRule usage conformance#4217

Merged
dsmiley merged 1 commit intoapache:mainfrom
dsmiley:SolrClientTestRule-usage-conformance
Mar 17, 2026
Merged

SolrClientTestRule usage conformance#4217
dsmiley merged 1 commit intoapache:mainfrom
dsmiley:SolrClientTestRule-usage-conformance

Conversation

@dsmiley
Copy link
Contributor

@dsmiley dsmiley commented Mar 16, 2026

  • startSolr don't specify temp dir
  • newCollection don't specify collection1
  • getSolrClient don't specify collection1
  • withConfigSet use Path if possible

org.apache.solr.SolrTestCaseJ4.getFile should return an absolute file to reduce ambiguity

* startSolr don't specify temp dir
* newCollection don't specify collection1
* getSolrClient don't specify collection1
* withConfigSet use Path if possible

org.apache.solr.SolrTestCaseJ4.getFile should return an absolute file to reduce ambiguity
Copy link
Contributor

@epugh epugh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm. We need to establish a single STRONG pattern for how things are done. We all cut n paste when we write tests, and I suspect ai does that too! If there are three ways of doing something then they get propagated. Can we add some methods to forbidden api? Like the LucenetestCase?

@dsmiley
Copy link
Contributor Author

dsmiley commented Mar 16, 2026

Most of these lines were last touched by you Eric when you did some SolrJettyTestRule conversion. I wasn't paying enough attention as a reviewer. Basically, if you can lean on defaults and not provide something -- then do that.

@dsmiley
Copy link
Contributor Author

dsmiley commented Mar 16, 2026

There's nothing "forbidden" here or wrong. It's simplification. The simpler it is, the easier to understand / maintain, and also the less barriers/issues if someone wants to try another SolrClientTestRule type. For example a SolrClientTestRule using docker wouldn't even have a "solr home".

@epugh
Copy link
Contributor

epugh commented Mar 16, 2026

There's nothing "forbidden" here or wrong. It's simplification. The simpler it is, the easier to understand / maintain, and also the less barriers/issues if someone wants to try another SolrClientTestRule type. For example a SolrClientTestRule using docker wouldn't even have a "solr home".

I think I'm not communciating well. It feels like there are too many ways of doing things, and most people don't really dig into the "what is the best way of doing something", instead we see a pattern and just copy and paste it. This may not be a good thing. So, maybe in the refacotring work I was doing, I got a pattern and jsut reused it. What I want is a way to keep our code clean over time, so that these types of refacotrings/usage conformance don't need to be constnatly redone over time as code changes.

At any rate, this looks good to me!

@dsmiley
Copy link
Contributor Author

dsmiley commented Mar 16, 2026

It'd be interesting if there was a static analyzer that could identify shortcuts. It's common for methods/constructors to be overloaded with a quick call over to another that takes more arguments.

Failing that... maybe whenever a shortcut is added, we do a follow-up to try convert every opportunity to use it. But in practice... we're busy and don't prioritize.

@epugh
Copy link
Contributor

epugh commented Mar 16, 2026

It'd be interesting if there was a static analyzer that could identify shortcuts. It's common for methods/constructors to be overloaded with a quick call over to another that takes more arguments.

Failing that... maybe whenever a shortcut is added, we do a follow-up to try convert every opportunity to use it. But in practice... we're busy and don't prioritize.

Create a JIRA and mark it newdev. Plus, those are my favorites, so please cue me ;-).

@dsmiley dsmiley merged commit e2f9a50 into apache:main Mar 17, 2026
5 of 6 checks passed
@dsmiley dsmiley deleted the SolrClientTestRule-usage-conformance branch March 17, 2026 22:58
dsmiley added a commit that referenced this pull request Mar 20, 2026
* startSolr don't specify temp dir
* newCollection don't specify collection1
* getSolrClient don't specify collection1
* withConfigSet use Path if possible

org.apache.solr.SolrTestCaseJ4.getFile should return an absolute file to reduce ambiguity
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants