Handle cross-platform shell execution and adapt test utilities accordingly#2226
Handle cross-platform shell execution and adapt test utilities accordingly#2226mkondratek wants to merge 1 commit intodevelopfrom
Conversation
There was a problem hiding this comment.
Tests in this class were failing before on Windows. Now they pass. 🟢
| Process process = new ProcessBuilder() | ||
| .command(command) | ||
| .directory(workingDirectory.toFile()) | ||
| .redirectErrorStream(true) |
There was a problem hiding this comment.
On Windows, the OS pipe buffer for process output is limited. In the original implementation, the Java process called process.waitFor() before reading from the output streams. When the script's output exceeded the buffer size, the script would block waiting for the buffer to be cleared, while the Java process was blocked waiting for the script to finish, resulting in a deadlock.
mkondratek
force-pushed
the
mk/chore/tests-on-windows
branch
from
d769fb4 to
cfaa4a2
Compare
|
@PawelLipski, let me know whether we want to get all these tests running and green on Windows. I think it should be possible (including the UI tests), but we’d also need Windows CI. Without it, we’ll likely end up with Windows-only failures again sooner or later. If setting up a Windows pipeline is too costly, maybe we should reconsider fixing these tests on Windows for now. Even this PR—which is mostly a preamble for follow-up changes—already adds quite a bit of Windows-specific boilerplate. If that code is likely to stay unused, maybe we shouldn’t introduce it yet. WDYT? |
Definitely, let's set up Windows CI if it isn't too difficult. In git-machete CLI, there's a dedicated Windows executor/job and even a macOS executor/job. Both are free for public repositories & generally smooth to use. |
| @@ -1,12 +1,7 @@ | |||
| package com.virtuslab.gitmachete.backend.integration; | |||
|
|
|||
| import static com.virtuslab.gitmachete.backend.api.SyncToRemoteStatus.AheadOfRemote; | |||
There was a problem hiding this comment.
Wow weird that these imports weren't removed, despite removeUnusedImports() settings.
Lemme open an issue for fixing that - #2228
| rootDirectoryPath.resolve("machete-post-slide-out-hook-executed") | ||
|
|
||
| fun Path.makeExecutable() { | ||
| if (System.getProperty("os.name").lowercase(getDefault()).contains("windows")) { |
There was a problem hiding this comment.
Let's extract this to some shared place in testCommon
| String shell = "bash"; | ||
| String osName = System.getProperty("os.name").toLowerCase(); | ||
| if (osName.contains("windows")) { | ||
| String gitPath = runProcessAndReturnStdout(5, "where", "git").trim().split(System.lineSeparator())[0]; |
There was a problem hiding this comment.
Can it just be where sh instead of where git? 🤔
|
|
||
| String stdout = IOUtils.toString(process.getInputStream(), StandardCharsets.UTF_8); | ||
| String stderr = IOUtils.toString(process.getErrorStream(), StandardCharsets.UTF_8); | ||
| boolean completed = process.waitFor(timeoutSeconds, TimeUnit.SECONDS); |
There was a problem hiding this comment.
Hmmm now that waitFor is after getInputStream()... can String stderr = IOUtils.toString(process.getErrorStream(), ...) be restored? or is redirectErrorStream still required for some reason?
| cd $dir || exit 1 | ||
| shift | ||
| git init "$@" | ||
| if [[ "$(uname -s)" != *MINGW*_NT* ]]; then |
2 participants
No description provided.