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]
