Skip to content

Handle cross-platform shell execution and adapt test utilities accordingly #2226

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.

Sign up for GitHub

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

Open
mkondratek wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Handle cross-platform shell execution and adapt test utilities accordingly #2226

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 21 additions & 2 deletions backend/api/src/main/java/com/virtuslab/gitmachete/backend/hooks/BaseHookExecutor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import java.util.Arrays;
import java.util.concurrent.TimeUnit;

import com.virtuslab.qual.guieffect.IgnoreUIThreadUnsafeCalls;
import io.vavr.collection.List;
import io.vavr.collection.Map;
import kr.pe.kwonnam.slf4jlambda.LambdaLogger;
Expand All @@ -24,11 +25,13 @@ public abstract class BaseHookExecutor {
protected final String name;
protected final File rootDirectory;
protected final File hookFile;
protected final @Nullable String gitConfigCoreHooksPath;

protected BaseHookExecutor(String name, Path rootDirectoryPath, Path mainGitDirectoryPath,
@Nullable String gitConfigCoreHooksPath) {
this.name = name;
this.rootDirectory = rootDirectoryPath.toFile();
this.gitConfigCoreHooksPath = gitConfigCoreHooksPath;

val hooksDirPath = gitConfigCoreHooksPath != null
? Paths.get(gitConfigCoreHooksPath)
Expand All @@ -39,6 +42,7 @@ protected BaseHookExecutor(String name, Path rootDirectoryPath, Path mainGitDire
protected abstract LambdaLogger log();

@UIThreadUnsafe
@IgnoreUIThreadUnsafeCalls("com.virtuslab.gitmachete.testcommon.TestFileUtils")
protected @Nullable ExecutionResult executeHook(int timeoutSeconds, OnExecutionTimeout onTimeout,
Map<String, String> environment, String... args)
throws GitMacheteException {
Expand All @@ -54,8 +58,23 @@ protected BaseHookExecutor(String name, Path rootDirectoryPath, Path mainGitDire

log().debug(() -> "Executing ${name} hook (${hookFilePath}) for ${argsToString} in cwd=${rootDirectory}");
ProcessBuilder pb = new ProcessBuilder();
String[] commandAndArgs = List.of(args).prepend(hookFilePath).toJavaArray(String[]::new);
pb.command(commandAndArgs);
val command = List.of(args).prepend(hookFilePath);
val osName = System.getProperty("os.name").toLowerCase();
if (osName.contains("windows")) {
String shell = "sh";
try {
val testFileUtilsClass = Class.forName("com.virtuslab.gitmachete.testcommon.TestFileUtils");
shell = (String) testFileUtilsClass.getMethod("getShellExecutable").invoke(null);
} catch (ClassNotFoundException ignored) {
// This is expected when the hook is executed from the plugin, not from the tests.
// In that case, we rely on the shell being in the PATH.
} catch (Exception e) {
log().warn("Could not determine shell executable for ${name} hook: ${e.getMessage()}. Falling back to 'sh'");
}
pb.command(command.prepend(shell).toJavaArray(String[]::new));
} else {
pb.command(command.toJavaArray(String[]::new));
}
for (val item : environment) {
pb.environment().put(item._1(), item._2());
}
Expand Down
5 changes: 0 additions & 5 deletions ...a/com/virtuslab/gitmachete/backend/integration/StatusAndDiscoverIntegrationTestSuite.java
View file
Open in desktop
Copy link
Collaborator Author

@mkondratek mkondratek Jan 31, 2026

Choose a reason for hiding this comment

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

Tests in this class were failing before on Windows. Now they pass. 🟢

Original file line number Diff line number Diff line change
@@ -1,12 +1,7 @@
package com.virtuslab.gitmachete.backend.integration;

import static com.virtuslab.gitmachete.backend.api.SyncToRemoteStatus.AheadOfRemote;
Copy link
Collaborator

@PawelLipski PawelLipski Feb 2, 2026

Choose a reason for hiding this comment

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

Wow weird that these imports weren't removed, despite removeUnusedImports() settings.

Lemme open an issue for fixing that - #2228

import static com.virtuslab.gitmachete.backend.api.SyncToRemoteStatus.BehindRemote;
import static com.virtuslab.gitmachete.backend.api.SyncToRemoteStatus.DivergedFromAndNewerThanRemote;
import static com.virtuslab.gitmachete.backend.api.SyncToRemoteStatus.DivergedFromAndOlderThanRemote;
import static com.virtuslab.gitmachete.backend.api.SyncToRemoteStatus.InSyncToRemote;
import static com.virtuslab.gitmachete.backend.api.SyncToRemoteStatus.NoRemotes;
import static com.virtuslab.gitmachete.backend.api.SyncToRemoteStatus.Untracked;
import static com.virtuslab.gitmachete.testcommon.SetupScripts.ALL_SETUP_SCRIPTS;
import static com.virtuslab.gitmachete.testcommon.TestFileUtils.cleanUpDir;
import static org.junit.jupiter.api.Assertions.assertEquals;
Expand Down
5 changes: 4 additions & 1 deletion src/uiTest/kotlin/com/virtuslab/gitmachete/uitest/BaseUITestSuite.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import com.intellij.ide.starter.driver.engine.BackgroundRun
import com.intellij.ide.starter.driver.engine.runIdeWithDriver
import com.intellij.ide.starter.ide.IdeProductProvider
import com.intellij.ide.starter.models.TestCase
import com.intellij.ide.starter.plugins.PluginConfigurator
import com.intellij.ide.starter.project.ProjectInfoSpec
import com.intellij.ide.starter.runner.Starter
import com.intellij.remoterobot.RemoteRobot
Expand All @@ -28,6 +27,7 @@ import java.io.File
import java.nio.file.Files
import java.nio.file.Path
import java.nio.file.attribute.PosixFilePermission.*
import java.util.Locale.getDefault
import kotlin.time.Duration.Companion.minutes

abstract class BaseUITestSuite : TestGitRepository(SetupScripts.SETUP_WITH_SINGLE_REMOTE) {
Expand Down Expand Up @@ -256,6 +256,9 @@ abstract class BaseUITestSuite : TestGitRepository(SetupScripts.SETUP_WITH_SINGL
rootDirectoryPath.resolve("machete-post-slide-out-hook-executed")

fun Path.makeExecutable() {
if (System.getProperty("os.name").lowercase(getDefault()).contains("windows")) {
Copy link
Collaborator

@PawelLipski PawelLipski Feb 2, 2026

Choose a reason for hiding this comment

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

Let's extract this to some shared place in testCommon

return
}
val attributes = Files.getPosixFilePermissions(this)
attributes.add(OWNER_EXECUTE)
attributes.add(GROUP_EXECUTE)
Expand Down
31 changes: 27 additions & 4 deletions testCommon/src/testFixtures/java/com/virtuslab/gitmachete/testcommon/TestFileUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,10 @@
package com.virtuslab.gitmachete.testcommon;

import static com.virtuslab.gitmachete.testcommon.TestProcessUtils.runProcessAndReturnStdout;

import java.io.File;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.nio.file.StandardCopyOption;
import java.util.Comparator;

Expand All @@ -23,9 +22,33 @@ public static void copyScriptFromResources(String scriptName, Path targetDir) {
StandardCopyOption.REPLACE_EXISTING);
}

@SneakyThrows
public static String runProcessAndReturnStdout(int timeoutSeconds, String... command) {
Path currentDir = Paths.get(".").toAbsolutePath().normalize();
return TestProcessUtils.runProcessAndReturnStdout(currentDir, timeoutSeconds, command);
}

public static String getShellExecutable() {
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];
Copy link
Collaborator

@PawelLipski PawelLipski Feb 2, 2026

Choose a reason for hiding this comment

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

Can it just be where sh instead of where git? 🤔

if (gitPath.endsWith("cmd\\git.exe")) {
shell = gitPath.replace("cmd\\git.exe", "bin\\sh.exe");
} else if (gitPath.endsWith("bin\\git.exe")) {
shell = gitPath.replace("bin\\git.exe", "bin\\sh.exe");
} else {
shell = "sh"; // fall back to PATH
}
}
return shell;
}

@SneakyThrows
public static void prepareRepoFromScript(String scriptName, Path workingDir) {
runProcessAndReturnStdout(/* workingDirectory */ workingDir, /* timeoutSeconds */ 60,
/* command */ "bash", workingDir.resolve(scriptName).toString());
String shell = getShellExecutable();
String scriptPath = workingDir.resolve(scriptName).toString();
TestProcessUtils.runProcessAndReturnStdout(/* workingDirectory */ workingDir, /* timeoutSeconds */ 60, shell, scriptPath);
}

@SneakyThrows
Expand Down
8 changes: 4 additions & 4 deletions testCommon/src/testFixtures/java/com/virtuslab/gitmachete/testcommon/TestProcessUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,16 +20,16 @@ public static String runProcessAndReturnStdout(Path workingDirectory, int timeou
Process process = new ProcessBuilder()
.command(command)
.directory(workingDirectory.toFile())
.redirectErrorStream(true)
Copy link
Collaborator Author

@mkondratek mkondratek Jan 31, 2026

Choose a reason for hiding this comment

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

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.

.start();
boolean completed = process.waitFor(timeoutSeconds, TimeUnit.SECONDS);

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);
Copy link
Collaborator

@PawelLipski PawelLipski Feb 2, 2026

Choose a reason for hiding this comment

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

Hmmm now that waitFor is after getInputStream()... can String stderr = IOUtils.toString(process.getErrorStream(), ...) be restored? or is redirectErrorStream still required for some reason?


String commandRepr = Arrays.toString(command);
String NL = System.lineSeparator();
String stdoutMessage = "Stdout of " + commandRepr + ": " + NL + stdout;
String stderrMessage = "Stderr of " + commandRepr + ": " + NL + stderr;
String joinedMessage = NL + NL + stdoutMessage + NL + stderrMessage + NL;
String joinedMessage = NL + NL + stdoutMessage + NL;

assertTrue(
completed, "command " + commandRepr + " has not completed within " + timeoutSeconds + " seconds" + joinedMessage);
Expand Down
10 changes: 4 additions & 6 deletions testCommon/src/testFixtures/resources/common.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,12 +43,10 @@ function create_repo() {
cd $dir || exit 1
shift
git init "$@"
if [[ "$(uname -s)" != *MINGW*_NT* ]]; then
Copy link
Collaborator

@PawelLipski PawelLipski Feb 2, 2026

Choose a reason for hiding this comment

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

🤔 not needed anymore?

mkdir -p .git/hooks/
local hook_path=.git/hooks/machete-status-branch
echo "$status_branch_hook" > $hook_path
chmod +x $hook_path
fi
mkdir -p .git/hooks/
local hook_path=.git/hooks/machete-status-branch
echo "$status_branch_hook" > $hook_path
chmod +x $hook_path
# `--local` (per-repository) is the default when writing git config... let's put it here for clarity anyway.
git config --local user.email "circleci@example.com"
git config --local user.name "CircleCI"
Expand Down