Skip to content

SOLR-18566: Fixing BATs for PicoCLI migration - #5204

Open
jaykay12 wants to merge 29 commits into
apache:mainfrom
jaykay12:SOLR-18566-picocli-bats-fixed
Open

jaykay12 wants to merge 29 commits into
apache:mainfrom
jaykay12:SOLR-18566-picocli-bats-fixed

Conversation

@jaykay12

Copy link
Copy Markdown
Contributor

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

Description

Follow-up PR for:

Solution

Used Cursor for debugging help, code contribution is fully vetted by Jalaz Kumar.

Tests

4 bats tests:

  • test_basic_auth.bats
  • test_stream.bats
  • test_post.bats
  • test_stream.bats

Checklist

Please review the following and check all that apply:

  • I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
  • I have created a Jira issue and added the issue ID to my pull request title.
  • I have given Solr maintainers access to contribute to my PR branch. (optional but recommended, not available for branches on forks living under an organisation)
  • I have developed this patch against the main branch.
  • I have run ./gradlew check.
  • I have added tests for my changes.

@jaykay12
jaykay12 marked this pull request as draft October 23, 2026 07:08
@github-actions github-actions Bot added the documentation Improvements or additions to documentation label Oct 23, 2026
@jaykay12
jaykay12 marked this pull request as ready for review October 23, 2026 09:53
names = {"--solr-include-file"},
paramLabel = "FILE",
required = true,
// Always supplied by bin/solr (from SOLR_INCLUDE); hidden because a user-supplied copy would

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

maybe more concise comment possible? get to one line?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes, done.

Comment thread solr/packaging/test/bats_helper.bash Outdated
}

# Utility function for asserting between the commons-cli wording ($1), or, when running against the picocli CLI, the picocli wording ($2)
# Todo: Remove this once commons-cli cleanup happens

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i think in java code it's "TODO", do we actually have any reports that highlihgt todo code items?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Renamed to TODO.
Actually I am not aware of any such report which picks up our TODOs, i added it as a marker for the cleanup of commons-cli.

# The auth command exports some system variables that are injected as basic auth username and password,
# however that defeats our test so fake that out via --solr-include-file param specifing a bogus path.
solr auth enable --type basicAuth --credentials name:password --solr-include-file /force/credentials/to/be/supplied
SOLR_INCLUDE=/force/credentials/to/be/supplied solr auth enable --type basicAuth --credentials name:password

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

if the way to pass this setting in is via a system property, that maybe begs teh question why do we have the parameter at all?

@jaykay12 jaykay12 Oct 26, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

not fully sure about this aspect as of now. will have to dig little deeper here.


@test "solr help flag prints help" {
run -1 solr --help
assert_output --partial 'Usage: solr COMMAND OPTIONS'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i know we are doing "the right thing", but a part of me just wishes we removed the commons-cli testing at this point... but this is "the right way"...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes, this is to facilitate a smoother migration.
+1 this feels like the right way, once we ensure stablity of picocli, this all goes away then.

*-s*, *--solr-connection*=_<solrConnection>_::
Zookeeper or HTTP(s) connection string; unnecessary if SOLR_CONNECTION is defined in solr.in.sh; otherwise, defaults to localhost:9983.

*--solr-include-file*=_FILE_::

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why can't we just keep this? Or is this just a property that no one uses, so we simplifeid by making it a system property?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

i am not fully confident on this aspect, will try digging more here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cat:cli documentation Improvements or additions to documentation tool:build

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants