-
Notifications
You must be signed in to change notification settings - Fork 186
fix: Stop local appium process with SIGTERM #1006
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
Changes from all commits
65afbdc
b9d7824
d2f3655
77577eb
54d3cb5
fed3efb
d7c8cc9
3d1c6f6
bd92a11
ec9a8f2
d0bf781
e398ede
8cb7034
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -22,6 +22,7 @@ | |||||||||||||||
| using System.Net; | ||||||||||||||||
| using System.Net.Http; | ||||||||||||||||
| using System.Runtime.CompilerServices; | ||||||||||||||||
| using System.Runtime.InteropServices; | ||||||||||||||||
| using System.Threading.Tasks; | ||||||||||||||||
|
|
||||||||||||||||
| namespace OpenQA.Selenium.Appium.Service | ||||||||||||||||
|
|
@@ -144,19 +145,86 @@ private async Task StartAsync() | |||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| private bool TryGracefulShutdownOnWindows(Process process, int timeoutMs = 5000) | ||||||||||||||||
| { | ||||||||||||||||
| if (process == null) | ||||||||||||||||
| return true; | ||||||||||||||||
|
|
||||||||||||||||
| // Safely check HasExited, handling disposed process | ||||||||||||||||
| bool hasExited; | ||||||||||||||||
| try | ||||||||||||||||
| { | ||||||||||||||||
| hasExited = process.HasExited; | ||||||||||||||||
| } | ||||||||||||||||
| catch (InvalidOperationException) | ||||||||||||||||
| { | ||||||||||||||||
|
||||||||||||||||
| { | |
| { | |
| // Process is not running / not started; treat as exited | |
| return true; | |
| } | |
| catch (ObjectDisposedException) | |
| { |
Copilot
AI
Feb 8, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TryGracefulShutdownOnWindows uses CloseMainWindow/WaitForExit, but the Appium process is started with CreateNoWindow=true (console Node process), so it typically has no main window handle and CloseMainWindow will be a no-op. This means the method will almost always fall back to Kill() and won't provide the intended graceful shutdown. Consider implementing actual console CTRL+C/CTRL+BREAK delivery (e.g., GenerateConsoleCtrlEvent + process group setup) or another Appium-supported graceful stop mechanism.
Dor-bl marked this conversation as resolved.
Show resolved
Hide resolved
Copilot
AI
Feb 8, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR description/title mention SIGTERM/CTRL+C-based shutdown, but DestroyProcess still uses Kill() on non-Windows (SIGKILL on Unix) and the Windows path uses CloseMainWindow rather than CTRL+C. Either update the implementation to match the intended signal-based shutdown behavior or adjust the PR description/title to reflect what is actually implemented.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,185 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| using NUnit.Framework; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| using System; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| using System.Diagnostics; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| using System.IO; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| using System.Reflection; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| using System.Runtime.InteropServices; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| using OpenQA.Selenium.Appium.Service; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| using OpenQA.Selenium.Appium.Service.Options; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| namespace Appium.Net.Integration.Tests.ServerTests | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| [TestFixture] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public class AppiumLocalServiceTests | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private AppiumLocalService appiumServer; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| [OneTimeSetUp] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public void GlobalSetup() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // OptionCollector can be customized as needed | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| var optionCollector = new OptionCollector(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Try to get Appium path from environment variable or npm root -g | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| string appiumPath = Environment.GetEnvironmentVariable(AppiumServiceConstants.AppiumBinaryPath); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (string.IsNullOrEmpty(appiumPath)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| bool isWindows = RuntimeInformation.IsOSPlatform(OSPlatform.Windows); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| var startInfo = new ProcessStartInfo | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // On Windows, using 'cmd /c' is often more reliable for resolving PATHs | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| FileName = isWindows ? "cmd.exe" : "npm", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Arguments = isWindows ? "/c npm root -g" : "root -g", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| RedirectStandardOutput = true, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| RedirectStandardError = true, // Capture errors too! | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| UseShellExecute = false, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| CreateNoWindow = true | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| using var process = Process.Start(startInfo); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| string output = process.StandardOutput.ReadToEnd()?.Trim(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| string error = process.StandardError.ReadToEnd()?.Trim(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| process.WaitForExit(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!string.IsNullOrEmpty(output)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| appiumPath = Path.Combine(output, "appium", "build", "lib", "main.js"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| else if (!string.IsNullOrEmpty(error)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Console.WriteLine($"NPM Error: {error}"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+44
to
+54
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| string output = process.StandardOutput.ReadToEnd()?.Trim(); | |
| string error = process.StandardError.ReadToEnd()?.Trim(); | |
| process.WaitForExit(); | |
| if (!string.IsNullOrEmpty(output)) | |
| { | |
| appiumPath = Path.Combine(output, "appium", "build", "lib", "main.js"); | |
| } | |
| else if (!string.IsNullOrEmpty(error)) | |
| { | |
| Console.WriteLine($"NPM Error: {error}"); | |
| if (process == null) | |
| { | |
| Console.WriteLine("Failed to start npm process to locate global Appium installation."); | |
| } | |
| else | |
| { | |
| const int npmTimeoutMilliseconds = 15000; | |
| if (!process.WaitForExit(npmTimeoutMilliseconds)) | |
| { | |
| try | |
| { | |
| process.Kill(); | |
| } | |
| catch (Exception killEx) | |
| { | |
| Console.WriteLine($"Failed to kill npm process after timeout: {killEx.Message}"); | |
| } | |
| Assert.Ignore( | |
| $"Timed out waiting for 'npm root -g' to complete after {npmTimeoutMilliseconds} ms. " + | |
| "Skipping AppiumLocalServiceTests to avoid hanging the test run."); | |
| } | |
| string output = process.StandardOutput.ReadToEnd()?.Trim(); | |
| string error = process.StandardError.ReadToEnd()?.Trim(); | |
| if (!string.IsNullOrEmpty(output)) | |
| { | |
| appiumPath = Path.Combine(output, "appium", "build", "lib", "main.js"); | |
| } | |
| else if (!string.IsNullOrEmpty(error)) | |
| { | |
| Console.WriteLine($"NPM Error: {error}"); | |
| } |
Copilot
AI
Feb 8, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This setup duplicates logic that already exists in test/integration/helpers (Paths + Npm.GetNpmPrefixPath). Reusing the shared helper would reduce duplication and keep Appium path resolution consistent across integration tests.
Copilot
AI
Feb 8, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OneTimeSetUp always starts a real Appium server even though only one test (TryGracefulShutdownOnWindows_RealProcess_DoesNotThrow) needs it. This makes the whole suite slower and causes all tests to be skipped if Appium isn’t available. Consider starting the server only in the specific test that requires it (or moving the real-process test into its own fixture).
| // OptionCollector can be customized as needed | |
| var optionCollector = new OptionCollector(); | |
| // Try to get Appium path from environment variable or npm root -g | |
| string appiumPath = Environment.GetEnvironmentVariable(AppiumServiceConstants.AppiumBinaryPath); | |
| if (string.IsNullOrEmpty(appiumPath)) | |
| { | |
| try | |
| { | |
| bool isWindows = RuntimeInformation.IsOSPlatform(OSPlatform.Windows); | |
| var startInfo = new ProcessStartInfo | |
| { | |
| // On Windows, using 'cmd /c' is often more reliable for resolving PATHs | |
| FileName = isWindows ? "cmd.exe" : "npm", | |
| Arguments = isWindows ? "/c npm root -g" : "root -g", | |
| RedirectStandardOutput = true, | |
| RedirectStandardError = true, // Capture errors too! | |
| UseShellExecute = false, | |
| CreateNoWindow = true | |
| }; | |
| using var process = Process.Start(startInfo); | |
| string output = process.StandardOutput.ReadToEnd()?.Trim(); | |
| string error = process.StandardError.ReadToEnd()?.Trim(); | |
| process.WaitForExit(); | |
| if (!string.IsNullOrEmpty(output)) | |
| { | |
| appiumPath = Path.Combine(output, "appium", "build", "lib", "main.js"); | |
| } | |
| else if (!string.IsNullOrEmpty(error)) | |
| { | |
| Console.WriteLine($"NPM Error: {error}"); | |
| } | |
| } | |
| catch (Exception ex) | |
| { | |
| Console.WriteLine($"Process failed: {ex.Message}"); | |
| } | |
| } | |
| if (string.IsNullOrEmpty(appiumPath) || !File.Exists(appiumPath)) | |
| { | |
| Assert.Ignore("Appium is not installed or not found on this machine. Skipping AppiumLocalServiceTests."); | |
| } | |
| appiumServer = new AppiumServiceBuilder() | |
| .WithAppiumJS(new FileInfo(appiumPath)) | |
| .WithIPAddress("127.0.0.1") | |
| .UsingAnyFreePort() | |
| .WithArguments(optionCollector) | |
| .Build(); | |
| appiumServer.Start(); | |
| // Check that the server is running after startup | |
| Assert.That(appiumServer.IsRunning, Is.True, "Appium server should be running after Start()"); | |
| // Intentionally left empty. | |
| // Starting a real Appium server in OneTimeSetUp makes all tests in this fixture | |
| // depend on a local Appium installation and pay the startup cost, even when | |
| // they do not require a real server. Any test that truly needs a running | |
| // Appium server should start (and stop) it explicitly within that test. |
Copilot
AI
Feb 8, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CreateService constructs an AppiumLocalService (which creates an HttpClient) but the created instances are never disposed in the tests. Dispose the service instances (e.g., via using/try-finally) to avoid resource leaks across the test run.
Dor-bl marked this conversation as resolved.
Show resolved
Hide resolved
Copilot
AI
Feb 8, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test disposes a Process and then expects an InvalidOperationException path. In practice, invoking TryGracefulShutdownOnWindows may surface ObjectDisposedException (or a different exception type) depending on runtime implementation, which would make the test flaky. Prefer creating a real short-lived process that exits, or explicitly assert/handle ObjectDisposedException if the intention is to cover disposed processes.
Uh oh!
There was an error while loading. Please reload this page.