Move process output handling to separate class & add tests #101
Conversation
This allows testing process handling in general without having to run certain Git commands, hoping that they behave in a certain way (exit code and console output), which requires complicated test setups and which could break for future Git versions.
| // return false to indicate we don't need to read more content | ||
| return false; |
There was a problem hiding this comment.
Removed this logic to stop reading from the output. It is always necessary to read from the output, even if the lines are then discarded, otherwise the output buffer can get full and the process gets stuck writing to stdout.
| final Future<Optional<RuntimeException>> stderrFuture = executorService.submit( | ||
| new CallableBufferedStreamReader(proc.getErrorStream(), | ||
| line -> { | ||
| errMsg.append(line); |
There was a problem hiding this comment.
This might have been missing a \n after every line.
| } catch (ExecutionException e) { | ||
| throw new ExecutionException("Failed waiting for stdout", e.getCause()); | ||
| } | ||
| try { | ||
| stderrOutput = stderrFuture.get(); | ||
| } catch (ExecutionException e) { | ||
| throw new ExecutionException("Failed waiting for stderr", e.getCause()); | ||
| } |
There was a problem hiding this comment.
Not completely sure about this unwrapping with e.getCause() and then rewrapping as ExecutionException. Could also directly propagate e, but then you would have to inspect the stack trace to understand whether the exception occurred for the stdout or stderr thread.
| private static void runJavaProcess(int exitCode, int outRepeatCount, long sleepMillis, TestAction testAction) { | ||
| // Creates a small Java program and starts it directly from the source file (JEP 330) | ||
| // This is done instead of using shell scripts to be more independent of the OS running the test | ||
| String javaBinaryPath = System.getProperty("java.home") + File.separator + "bin" + File.separator + "java"; |
There was a problem hiding this comment.
Constructing the javaBinaryPath this way might be a bit brittle, but it is probably the most reliable approach.
I also thought about obtaining the java path using java.lang.ProcessHandle.current() assuming that the current test process is a Java process, but that might be too speculative. For example when running the test in Eclipse this was actually javaw, which is probably fine as well, but shows that making assumptions about the current running process is not reliable enough.
Any suggestions for alternative ways for obtaining the java path are welcome.
(Though I would really like to avoid using shell scripting instead of a Java source file, because that would require OS-specific shell scripts and might be quite error-prone.)
There was a problem hiding this comment.
Thanks for your contribution! I like the overall change and it looks good to me. However this tests feels a bit off in my view. I clearly would agree that adding tests always bring a benefit (e.g. to see if something is broken that worked before), but I'm not sure if we somehow want to rely on executing java.
I'll need to think about that...I can't think of a real alternative that would work os independant. Certainly hacking some shell scripts is IMHO even worse...
There was a problem hiding this comment.
@TheSnoozer, have you made a decision yet? No hurry in case you haven't yet.
Would it be an option to include these changes, and if this test causes problems in the future or becomes difficult to maintain, add @Ignore to it (or in the worst case even remove it again)?
I think the main problem (if any) with this is obtaining the java path and making sure it is the correct one. The other parts of this test should be quite reliable.
One way in which this test can be improved though could be to add a proper DummyProgram.java file or similar under src/test/resources/process-handler/1 instead of the current approach of creating temp files for the Java source code. Should I change that?
Footnotes
-
The reason why I would move it under
src/test/resourcesinstead ofsrc/test/javais that there is no need to create a.classfile for it, it might look weird if there is this seemingly unused Java source file undersrc/test/javaand one might be tempted to use some of the test dependencies in it, even though this would not work at runtime.
That would however require moving the current Git submodule atsrc/test/resourcesone level down, e.g. tosrc/test/resources/git-repos. ↩
There was a problem hiding this comment.
Indeed the main problem is obtaining the java executable....
I guess you are right, in case this becomes a hassle in the future I still can remove the tests, or add @ignore...
Thanks again for your contribution I'm going ahead and merge now!
Context
This allows testing process handling in general without having to run certain Git commands, hoping that they behave in a certain way (exit code and console output), which requires complicated test setups and which could break for future Git versions. See also git-commit-id/git-commit-id-maven-plugin#483 (this hopefully allows removing
BigDiffTest).I have added additional GitHub review comments below to highlight certain aspects of these changes.
Contributor Checklist
mvn clean packagecheckstylecoding style definition:mvn clean verify -Pcheckstyle -Dmaven.test.skip=true -B