Re: [PR] Tests for examples failing on Windows [solr]
epugh commented on PR #3378: URL: https://github.com/apache/solr/pull/3378#issuecomment-2919665135 Can you add a changes entry? And then when the tests pass I'll merge. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] Tests for examples failing on Windows [solr]
rahulgoswami commented on code in PR #3378:
URL: https://github.com/apache/solr/pull/3378#discussion_r2114128683
##
solr/core/src/test/org/apache/solr/cli/TestSolrCLIRunExample.java:
##
@@ -102,7 +102,7 @@ public int execute(org.apache.commons.exec.CommandLine cmd)
throws IOException {
commandsExecuted.add(cmd);
String exe = cmd.getExecutable();
- if (exe.endsWith("solr")) {
+ if (exe.endsWith("solr") || exe.endsWith("solr.cmd")) {
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Tests for examples failing on Windows [solr]
epugh commented on code in PR #3378:
URL: https://github.com/apache/solr/pull/3378#discussion_r2114086764
##
solr/core/src/test/org/apache/solr/cli/TestSolrCLIRunExample.java:
##
@@ -102,7 +102,7 @@ public int execute(org.apache.commons.exec.CommandLine cmd)
throws IOException {
commandsExecuted.add(cmd);
String exe = cmd.getExecutable();
- if (exe.endsWith("solr")) {
+ if (exe.endsWith("solr") || exe.endsWith("solr.cmd")) {
Review Comment:
Love it! Thanks for the explanation and the simplification. I love
elminating if loops.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Tests for examples failing on Windows [solr]
rahulgoswami commented on code in PR #3378:
URL: https://github.com/apache/solr/pull/3378#discussion_r2114054937
##
solr/core/src/test/org/apache/solr/cli/TestSolrCLIRunExample.java:
##
@@ -102,7 +102,7 @@ public int execute(org.apache.commons.exec.CommandLine cmd)
throws IOException {
commandsExecuted.add(cmd);
String exe = cmd.getExecutable();
- if (exe.endsWith("solr")) {
+ if (exe.endsWith("solr") || exe.endsWith("solr.cmd")) {
Review Comment:
I agree. An assert is better. I'll chop off the "else".
Quick background: This change only impacts ***tests*** on Windows. Post the
fix for jvm-opts, command line execution runs fine.
The start flow via solr.cmd passes a "--script" parameter (which our tests
don't) and uses a different executor inside RunExampleTool from what the tests
use (RunExampleExecutor). Prior to recently merged fix for jvm-opts, because of
these reasons, the tests on Windows would also try to prepare a command line
with bin/solr (instead of bin/solr.cmd). Hence those tests would pass getting
into the "if" block in this PR, although in an unintended way.
So basically by fixing the code to do the right thing, the tests on Windows
are failing ;)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Tests for examples failing on Windows [solr]
rahulgoswami commented on code in PR #3378:
URL: https://github.com/apache/solr/pull/3378#discussion_r2114054937
##
solr/core/src/test/org/apache/solr/cli/TestSolrCLIRunExample.java:
##
@@ -102,7 +102,7 @@ public int execute(org.apache.commons.exec.CommandLine cmd)
throws IOException {
commandsExecuted.add(cmd);
String exe = cmd.getExecutable();
- if (exe.endsWith("solr")) {
+ if (exe.endsWith("solr") || exe.endsWith("solr.cmd")) {
Review Comment:
I agree. An assert is better. I'll chop off the "else".
Quick background: This change only impacts ***tests*** on Windows. Post the
fix for jvm-opts, command line execution runs fine.
The start flow via solr.cmd passes a "--script" parameter (which our tests
don't) and uses a different executor inside RunExampleTool from what the tests
use (RunExampleExecutor). Prior to recently merged fix for jvm-opts, because of
these reasons, the tests on Windows would also try to prepare a command line
with bin/solr (instead of bin/solr.cmd). Hence those tests would pass getting
into the "if" block in this PR, although in an unintended way.
So basically by fixing the code to do the right thing, the tests on Windows
are failing ;)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Tests for examples failing on Windows [solr]
rahulgoswami commented on code in PR #3378:
URL: https://github.com/apache/solr/pull/3378#discussion_r2114054937
##
solr/core/src/test/org/apache/solr/cli/TestSolrCLIRunExample.java:
##
@@ -102,7 +102,7 @@ public int execute(org.apache.commons.exec.CommandLine cmd)
throws IOException {
commandsExecuted.add(cmd);
String exe = cmd.getExecutable();
- if (exe.endsWith("solr")) {
+ if (exe.endsWith("solr") || exe.endsWith("solr.cmd")) {
Review Comment:
I agree. An assert is better. I'll chop off the "else".
Quick background: This change only impacts ***tests*** on Windows. Post the
fix for jvm-opts, command line execution runs fine. The start flow via solr.cmd
passes a "--script" parameter (which our tests don't) and uses a different
executor inside RunExampleTool from what the tests use (RunExampleExecutor).
Prior to recently merged fix for jvm-opts, because of these reasons, the tests
on Windows would also try to prepare a command line with bin/solr (instead of
bin/solr.cmd). Hence those tests would pass getting into the "if" block in this
PR, although in an unintended way.
So basically by fixing the code to do the right thing, the tests on Windows
are failing ;)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Tests for examples failing on Windows [solr]
rahulgoswami commented on code in PR #3378:
URL: https://github.com/apache/solr/pull/3378#discussion_r2114054937
##
solr/core/src/test/org/apache/solr/cli/TestSolrCLIRunExample.java:
##
@@ -102,7 +102,7 @@ public int execute(org.apache.commons.exec.CommandLine cmd)
throws IOException {
commandsExecuted.add(cmd);
String exe = cmd.getExecutable();
- if (exe.endsWith("solr")) {
+ if (exe.endsWith("solr") || exe.endsWith("solr.cmd")) {
Review Comment:
I agree. An assert is better. I'll chop off the "else".
Quick background: This change only impacts ***tests*** on Windows. Post the
fix for jvm-opts, command line execution is now fine. The start flow via
solr.cmd passes a "--script" parameter (which our tests don't) and uses a
different executor inside RunExampleTool from what the tests use
(RunExampleExecutor). Prior to recently merged fix for jvm-opts, because of
these reasons, the tests on Windows would also try to prepare a command line
with bin/solr (instead of bin/solr.cmd). Hence those tests would pass getting
into the "if" block in this PR, although in an unintended way.
So basically by fixing the code to do the right thing, the tests on Windows
are failing ;)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Tests for examples failing on Windows [solr]
rahulgoswami commented on code in PR #3378:
URL: https://github.com/apache/solr/pull/3378#discussion_r2114054937
##
solr/core/src/test/org/apache/solr/cli/TestSolrCLIRunExample.java:
##
@@ -102,7 +102,7 @@ public int execute(org.apache.commons.exec.CommandLine cmd)
throws IOException {
commandsExecuted.add(cmd);
String exe = cmd.getExecutable();
- if (exe.endsWith("solr")) {
+ if (exe.endsWith("solr") || exe.endsWith("solr.cmd")) {
Review Comment:
I agree. An assert is better. I'll chop off the "else".
Quick background: This change only impacts **tests** on Windows. Post the
fix for jvm-opts, command line execution is now fine. The start flow via
solr.cmd passes a "--script" parameter (which our tests don't) and uses a
different executor inside RunExampleTool from what the tests use
(RunExampleExecutor). Prior to recently merged fix for jvm-opts, because of
these reasons, the tests on Windows would also try to prepare a command line
with bin/solr (instead of bin/solr.cmd). Hence those tests would pass getting
into the "if" block in this PR, although in an unintended way.
So basically by fixing the code to do the right thing, the tests on Windows
are failing ;)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Tests for examples failing on Windows [solr]
epugh commented on code in PR #3378:
URL: https://github.com/apache/solr/pull/3378#discussion_r2113850972
##
solr/core/src/test/org/apache/solr/cli/TestSolrCLIRunExample.java:
##
@@ -102,7 +102,7 @@ public int execute(org.apache.commons.exec.CommandLine cmd)
throws IOException {
commandsExecuted.add(cmd);
String exe = cmd.getExecutable();
- if (exe.endsWith("solr")) {
+ if (exe.endsWith("solr") || exe.endsWith("solr.cmd")) {
Review Comment:
i looked at the class... and it's super specific to testing Solr CLI, so I
can't imagine a world where anything BUT a solr or solr.cmd is used??? In
fact, I think an assert would be more apprporiate, to fail the test if you used
this class with anything but solr or solr.cmd?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] Tests for examples failing on Windows [solr]
epugh commented on code in PR #3378:
URL: https://github.com/apache/solr/pull/3378#discussion_r2113847898
##
solr/core/src/test/org/apache/solr/cli/TestSolrCLIRunExample.java:
##
@@ -102,7 +102,7 @@ public int execute(org.apache.commons.exec.CommandLine cmd)
throws IOException {
commandsExecuted.add(cmd);
String exe = cmd.getExecutable();
- if (exe.endsWith("solr")) {
+ if (exe.endsWith("solr") || exe.endsWith("solr.cmd")) {
Review Comment:
somethign I am confused by How would the exe ever NOT be either solr
or solr.cmd??? Is there a way this code can be run that doesn't include one
of those? I am just wondering if we need this if statement?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
[PR] Tests for examples failing on Windows [solr]
rahulgoswami opened a new pull request, #3378: URL: https://github.com/apache/solr/pull/3378 https://issues.apache.org/jira/browse/SOLR-X # Description Running gradlew -p solr/core test --tests "org.apache.solr.cli.TestSolrCLIRunExample" on Windows causes tests for techproducts, films and other examples to fail. # Solution Tests initiated via TestSolrCLIRunExample use a custom executor (RunExampleExecutor). The execute(cmd) method of this executor executes one logic of starting an embedded test solr cluster when the script is bin/solr and a default alternate flow otherwise. This condition should instead check for either of bin/solr or bin/solr.cmd since starting a test cluster in embedded mode essentially makes this flow platform independent. One could argue that the way we spawn a test cluster for these tests doesn't *exactly* replicate a CLI flow, but that could be tackled as a separate fix to make the tests *better*. # Tests gradlew -p solr/core test --tests "org.apache.solr.cli.TestSolrCLIRunExample" succeeds on Windows. # Checklist Please review the following and check all that apply: - [ ] I have reviewed the guidelines for [How to Contribute](https://github.com/apache/solr/blob/main/CONTRIBUTING.md) 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](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) 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. - [ ] I have added documentation for the [Reference Guide](https://github.com/apache/solr/tree/main/solr/solr-ref-guide) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
