Skip to content
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

Fixed test failure in windows ie, testEcho() method #119

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

Kshitiz-Mhto
Copy link

part of fixing #104

@fahad-israr
Copy link
Collaborator

Didn't get u here. Thought issue was around some text formatting? What was the actual isssue acc to you? How does using a process builder instead of process executor fix it?

@Kshitiz-Mhto
Copy link
Author

i thought the same first, that issue is causing due to some text formatting in windows. But using ProcessExecutor to run external command in my windows machine i am getting such error msg:

<testcase name="testEcho" classname="dev.starfix.StarfixTest" time="0.563">
    <error message="Could not execute [echo, This is some random String that I want to Echo]. Error=2, The system cannot find the file specified" type="org.zeroturnaround.exec.ProcessInitException">org.zeroturnaround.exec.ProcessInitException: Could not execute [echo, This is some random String that I want to Echo]. Error=2, The system cannot find the file specified
	at dev.starfix.StarfixTest.testEcho(StarfixTest.java:42)
Caused by: java.io.IOException: Cannot run program "echo": CreateProcess error=2, The system cannot find the file specified
	at dev.starfix.StarfixTest.testEcho(StarfixTest.java:42)
Caused by: java.io.IOException: CreateProcess error=2, The system cannot find the file specified
	at dev.starfix.StarfixTest.testEcho(StarfixTest.java:42)
</error>

the error is indicating it isnot able to find or execute the executable echo executable in windows.

so i went for the alternative choice using ProcessBuilder to run external command and it worked fine...

@fahad-israr
Copy link
Collaborator

fahad-israr commented Feb 21, 2023

can u plz try figuring out what's the issue with ProcessExecutor here? we should have a very valid reason when discarding ProcessExecutor and moving to ProcessBuilder imo .

@Kshitiz-Mhto
Copy link
Author

Kshitiz-Mhto commented Feb 21, 2023

can u plz try figuring out what's the issue with ProcessExecutor here? we should have a very valid reason when discarding ProcessExecutor and moving to ProcessBuilder imo .

no problem, i m going to figure this out.

@Kshitiz-Mhto
Copy link
Author

@fahad-israr done 👍. I fixed it using ProcessExecutor.
just one doubt, why we are using 3rd party library ProcessExecutor other than using built in library ProcessBuilder class for performing simpler task like for echo command? Is there specific reason to it?

@fahad-israr
Copy link
Collaborator

  • The function public static String runCommand(Path directory, String... command) referred here is a central function used to execute all of the commands not just echo, example: gitClone(), ide_launcher .
  • ZT ProcessExecutor is smooth and easy for handling/consuming stdout/sterr properly. Infact there are more modern libraries that can be more helpful to us and better than ZT Process Executor like JProc and Fluent Process.
  • Look at the code example for similar tasks here : https://ongres.com/blog/java-processes-and-streams/

if (isWindows()) {
try{
System.out.println("Running " + String.join(" ", command));
presult = new ProcessExecutor().command("CMD", "/C", command[0], command[1]).redirectOutput(System.out).redirectErrorStream(true).readOutput(true)
Copy link
Collaborator

Choose a reason for hiding this comment

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

why are you assuming that commands array will be of max size 2. can't there be more args than that ?
Infact there itself look at the gitClone Function which is calling run command with how many args:
runCommand(directory.getParent(), "git", "clone", originUrl, directory.toString());

Copy link
Author

Choose a reason for hiding this comment

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

my mistake. its done now I think.

@maxandersen
Copy link
Member

Zut process executor is afaik still the best around when it comes to manage the handling of input and output streams.

It takes care of the threads currently today required to be used to get output and empty/close the streams.

Which you need to do especially on windows where a process won't even execute if you don't continuously empty the stream.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants