Add Process tests for MacCatalyst and fix MacCatalyst support - #126306

Merged
adamsitnik merged 17 commits into
mainfrom
copilot/implement-process-tests-maccatalyst
Apr 2, 2026
Merged

Add Process tests for MacCatalyst and fix MacCatalyst support#126306
adamsitnik merged 17 commits into
mainfrom
copilot/implement-process-tests-maccatalyst

Conversation

CopilotAI commented Mar 30, 2026

Copy link
Copy Markdown
Contributor

All existing Process tests rely on RemoteExecutor, which is unsupported on MacCatalyst. This adds mobile-platform Process tests using simple Unix tools (ls, sleep) available in both the Android and MacCatalyst environments, and fixes MacCatalyst-specific bugs in the Process implementation.

Changes

  • ProcessTests.Mobile.cs (new, replaces separate ProcessTests.Android.cs and ProcessTests.MacCatalyst.cs): Six mobile Process tests shared between Android and MacCatalyst via [PlatformSpecific(TestPlatforms.Android | TestPlatforms.MacCatalyst)], covering:

    • Basic process start with inherited I/O
    • Redirected stdout/stderr
    • Process.Kill() termination
    • Non-zero exit code on command failure
    • ProcessName availability for a running process
    • StandardInputHandle/StandardOutputHandle/StandardErrorHandle via anonymous pipes
  • SafeProcessHandle.Unix.cs:

    • Extracted a new private static bool UsesTerminal(SafeFileHandle?, SafeFileHandle?, SafeFileHandle?) helper method (with explanatory comments preserved) called from both StartCore and StartWithShellExecute, eliminating the previous inconsistency between the two call sites. UsesTerminal now delegates to ProcessUtils.IsTerminal (see below), removing the inline Interop.Sys.IsATty calls and the PlatformSupportsConsole guard from this site.
    • Updated comment to mention both Android and MacCatalyst as platforms that don't support Console.Open* methods
  • ProcessUtils.ConfigureTerminalForChildProcesses.iOS.cs: Added internal static bool IsTerminal(SafeFileHandle? _) => false; — on iOS/tvOS/MacCatalyst there is no terminal, so this always returns false without any P/Invoke reference to Interop.Sys.IsATty.

  • ProcessUtils.ConfigureTerminalForChildProcesses.Unix.cs: Added internal static bool IsTerminal(SafeFileHandle? standardHandle) => standardHandle is not null && Interop.Sys.IsATty(standardHandle); — on other Unix platforms, delegates to the native IsATty check.

  • ProcessUtils.cs:

    • Added PlatformSupportsConsole (!(IsAndroid() || IsMacCatalyst())) to gate Console.OpenStandard*Handle() calls
    • Added PlatformSupportsProcessStartAndKill (replaces the previous double-negative PlatformDoesNotSupportProcessStartAndKill) with a positive sense consistent with PlatformSupportsConsole; all call sites updated to if (!ProcessUtils.PlatformSupportsProcessStartAndKill)
    • Moved the platform support check to the shared file (from ProcessUtils.Unix.cs) and added early checks in Process.Start() and SafeProcessHandle.Start()
  • ProcessTests.cs: Added TestPlatforms.MacCatalyst to the [SkipOnPlatform] attributes on ProcessStart_UseShellExecute_Executes and ProcessStart_UseShellExecute_WorkingDirectory, alongside the existing iOS and tvOS skips, since MacCatalyst shares the same shell script execution restrictions

  • ProcessHandlesTests.cs: Extended [SkipOnPlatform] to include iOS, tvOS, and MacCatalyst (in addition to Android); updated skip reason to "sh is not available in the mobile platform sandbox" for greater precision

  • System.Diagnostics.Process.Tests.csproj:

    • Added $(NetCoreAppCurrent)-maccatalyst to TargetFrameworks
    • Included maccatalyst in the Unix compile condition (picks up ProcessTests.Unix.cs, ProcessThreadTests.Unix.cs, and related interop)
    • Consolidated the android and maccatalyst ItemGroup entries into a single group for ProcessTests.Mobile.cs

CopilotAI linked an issue Mar 30, 2026 that may be closed by this pull request
@github-actionsgithub-actionsBot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Mar 30, 2026
- Add $(NetCoreAppCurrent)-maccatalyst to TargetFrameworks
- Include Unix interop files for maccatalyst target
- Create ProcessTests.MacCatalyst.cs with tests for Process.Start,
I/O redirection, process killing, exit codes, and process names
modeled on ProcessTests.Android.cs
Fixes: tests cannot run on MacCatalyst because RemoteExecutor is not supported
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/6cbdf5a5-19f7-46b1-acab-3a4a9f5a6dde
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
CopilotAI changed the title [WIP] Add Process tests for MacCatalyst supportAdd Process tests for MacCatalystMar 30, 2026
CopilotAI requested a review from adamsitnikMarch 30, 2026 11:15
@github-actionsgithub-actionsBot added area-System.Diagnostics.Process and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Mar 30, 2026
@adamsitnik

Copy link
Copy Markdown
Member

/azp list

@azure-pipelines

This comment was marked as resolved.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/area-system-diagnostics-process
See info in area-owners.md if you want to be subscribed.

@adamsitnik

Copy link
Copy Markdown
Member

/azp run runtime-maccatalyst

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@adamsitnik

adamsitnik commented Mar 30, 2026

Copy link
Copy Markdown
Member

@rolfbjarne@akoeplinger@vitek-karas I am going to need some help, as this failure does not tell me anything:

[08:54:54]info:Preparingrun for maccatalyst
[08:54:54]info:Gettingapp bundle information from '/tmp/helix/working/B5070A06/w/A65C0914/e/System.Diagnostics.Process.Tests.app'..
[08:54:54] dbug: Property UIRequiredDeviceCapabilities not present in Info.plist, assuming 32-bitis not supported.Error: Failed to get bundle information: Print: Entry,"UIRequiredDeviceCapabilities",Does Not Exist[08:54:54]dbug:***Executing 'System.Diagnostics.Process.Tests' on MacCatalyst ***

@jkotas

Copy link
Copy Markdown
Member

All existing Process tests rely on RemoteExecutor, which is unsupported on MacCatalyst.

Why is RemoteExecutor not unsupported on MacCatalyst?

If we are able to start child processes on MacCatalyst, RemoteExecutor should work just fine.

@adamsitnik

Copy link
Copy Markdown
Member

Why is RemoteExecutor not unsupported on MacCatalyst?

If we are able to start child processes on MacCatalyst, RemoteExecutor should work just fine.

I don't know, but I agree it would be the best long-term solution.

@rolfbjarne

Copy link
Copy Markdown
Member
dbug:PropertyUIRequiredDeviceCapabilities not present in Info.plist,assuming32-bitis not supported.Error: Failed to get bundle information: Print: Entry,"UIRequiredDeviceCapabilities",DoesNot Exist

This comes from here, and just looks diagnostic.

The problem is that there were test failures:

[08:55:13] info: Application finished the test run successfully with some failed tests
[08:55:13] info: Tests run: 299 Passed: 130 Inconclusive: 0 Failed: 28 Ignored: 141

which can be seen in the Tests tab here: https://dev.azure.com/dnceng-public/public/_build/results?buildId=1357894&view=ms.vss-test-web.build-test-results-tab&runId=37954004&resultId=157642&paneView=debug

@adamsitnikadamsitnik added the NO-REVIEW Experimental/testing PR, do NOT review it label Mar 31, 2026
@adamsitnik

Copy link
Copy Markdown
Member

/azp run runtime-maccatalyst

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Co-authored-by: Adam Sitnik <adam.sitnik@gmail.com>
@adamsitnik

Copy link
Copy Markdown
Member

/azp run runtime-maccatalyst

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

…rocessStartAndKill
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/634abc6a-3308-4bf9-8cbc-0ee291fe1533
Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
CopilotAI requested a review from jkotasApril 2, 2026 14:52
@adamsitnik

Copy link
Copy Markdown
Member

/azp run runtime-maccatalyst

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@adamsitnik
adamsitnik enabled auto-merge (squash) April 2, 2026 15:07
@github-actions

Copy link
Copy Markdown
Contributor

🤖 Copilot Code Review — PR #126306

Note

This review was generated by Copilot.

Holistic Assessment

Motivation: This PR adds MacCatalyst platform support to the System.Diagnostics.Process test suite and refactors platform-specific checks for clarity. The motivation is well-justified — MacCatalyst can run processes (unlike iOS/tvOS) but lacks Console support (like Android), and the existing code/tests didn't account for this.

Approach: The approach is clean and well-structured. It centralizes platform-specific logic into named properties (PlatformSupportsProcessStartAndKill, PlatformSupportsConsole), extracts IsTerminal into platform files (Unix: IsATty, iOS: false), deduplicates the UsesTerminal check, and consolidates Android+MacCatalyst tests into a shared ProcessTests.Mobile.cs.

Summary: ✅ LGTM. The code changes are correct and improve the codebase. The renamed property uses positive naming conventions, all callers are properly updated with no orphaned references, the PlatformNotSupportedException guard covers all public entry points, and no new public API surface is introduced. Minor observations noted below.


Detailed Findings

✅ Platform check refactoring is correct and complete

  • PlatformDoesNotSupportProcessStartAndKillPlatformSupportsProcessStartAndKill (positive naming ✓)
  • Moved from Unix-only ProcessUtils.Unix.cs to shared ProcessUtils.cs (now usable from non-Unix-specific code ✓)
  • No orphaned references to the old name remain anywhere in the repo ✓
  • All 4 callers (Process.Start, Process.Kill, SafeProcessHandle.Start, SafeProcessHandle.SignalCore) are updated ✓

✅ Console guard centralization is correct

PlatformSupportsConsole => !(OperatingSystem.IsAndroid() || OperatingSystem.IsMacCatalyst()) correctly replaces the previous inline !OperatingSystem.IsAndroid() checks. All 6 Console.OpenStandard*Handle() call sites (3 in Process.cs, 3 in SafeProcessHandle.cs) are properly guarded with this property.

✅ PlatformNotSupportedException guard coverage is complete

The guard was removed from the internal SafeProcessHandle.StartCore and placed in both public callers (Process.Start at line 1250, SafeProcessHandle.Start at line 101). Since StartCore is internal and only reachable through these two public entry points (verified: Process.StartCoreSafeProcessHandle.StartCore, and SafeProcessHandle.StartStartCore), the coverage is equivalent.

✅ IsTerminal extraction is well-designed

The IsTerminal helper is correctly split across platform files:

  • Unix (ConfigureTerminalForChildProcesses.Unix.cs): standardHandle is not null && Interop.Sys.IsATty(standardHandle) — performs the actual TTY check
  • iOS (ConfigureTerminalForChildProcesses.iOS.cs): => false — terminals are never available on iOS-like platforms (includes MacCatalyst via IsiOSLike csproj condition)

The iOS version uses discard parameter _ correctly, matching the pattern of AreChildrenUsingTerminal => false and the no-op ConfigureTerminalForChildProcesses in the same file.

✅ Test consolidation is appropriate

Renaming ProcessTests.Android.csProcessTests.Mobile.cs and applying [PlatformSpecific(TestPlatforms.Android | TestPlatforms.MacCatalyst)] at the class level is clean. The individual [PlatformSpecific(TestPlatforms.Android)] attributes are correctly removed since the class-level attribute covers them.

✅ csproj configuration is correct

  • $(NetCoreAppCurrent)-maccatalyst added to TargetFrameworks ✓
  • maccatalyst added to the Unix/interop ItemGroup condition ✓
  • Mobile test file included for both android and maccatalyst

💡 ProcessTests.Unix.cs UseShellExecute tests — low-risk gap

Two UseShellExecute tests in ProcessTests.Unix.cs (lines 114, 134: ProcessStart_UseShellExecute_OnUnix_OpenMissingFile_DoesNotThrow and ProcessStart_UseShellExecute_OnUnix_SuccessWhenProgramInstalled) skip iOS/tvOS/Android/Browser but not MacCatalyst. Now that maccatalyst is a test target, these tests will be compiled for MacCatalyst. However, both are [OuterLoop] tests so they won't affect regular CI, and UseShellExecute via the open command likely works on MacCatalyst (it's macOS-based). This is low-risk and could be addressed in a follow-up if needed.

💡 ProcessHandlesTests skip message

The previous version had two [SkipOnPlatform] attributes with distinct reasons ("sh is not available on Android" vs "Not supported on Apple mobile platforms"). These were consolidated into a single attribute with a combined message ("sh is not available in the mobile platform sandbox"). This is a minor readability tradeoff — the combined message is slightly less precise but avoids attribute duplication. Fine as-is.

Generated by Code Review for issue #126306 ·

@adamsitnik
adamsitnik merged commit 1f9be20 into mainApr 2, 2026
94 checks passed
@adamsitnik
adamsitnik deleted the copilot/implement-process-tests-maccatalyst branch April 2, 2026 17:56
radekdoulik pushed a commit to radekdoulik/runtime that referenced this pull request Apr 9, 2026
…#126306)
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: Adam Sitnik <adam.sitnik@gmail.com>
Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@github-actionsgithub-actionsBot locked and limited conversation to collaborators May 3, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement Process tests for MacCatalyst

5 participants

@adamsitnik@jkotas@rolfbjarne
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Add Process tests for MacCatalyst and fix MacCatalyst support - #126306

Merged
adamsitnik merged 17 commits into
mainfrom
copilot/implement-process-tests-maccatalyst
Apr 2, 2026
Merged

Add Process tests for MacCatalyst and fix MacCatalyst support#126306
adamsitnik merged 17 commits into
mainfrom
copilot/implement-process-tests-maccatalyst

Conversation

CopilotAI commented Mar 30, 2026

Copy link
Copy Markdown
Contributor

All existing Process tests rely on RemoteExecutor, which is unsupported on MacCatalyst. This adds mobile-platform Process tests using simple Unix tools (ls, sleep) available in both the Android and MacCatalyst environments, and fixes MacCatalyst-specific bugs in the Process implementation.

Changes

  • ProcessTests.Mobile.cs (new, replaces separate ProcessTests.Android.cs and ProcessTests.MacCatalyst.cs): Six mobile Process tests shared between Android and MacCatalyst via [PlatformSpecific(TestPlatforms.Android | TestPlatforms.MacCatalyst)], covering:

    • Basic process start with inherited I/O
    • Redirected stdout/stderr
    • Process.Kill() termination
    • Non-zero exit code on command failure
    • ProcessName availability for a running process
    • StandardInputHandle/StandardOutputHandle/StandardErrorHandle via anonymous pipes
  • SafeProcessHandle.Unix.cs:

    • Extracted a new private static bool UsesTerminal(SafeFileHandle?, SafeFileHandle?, SafeFileHandle?) helper method (with explanatory comments preserved) called from both StartCore and StartWithShellExecute, eliminating the previous inconsistency between the two call sites. UsesTerminal now delegates to ProcessUtils.IsTerminal (see below), removing the inline Interop.Sys.IsATty calls and the PlatformSupportsConsole guard from this site.
    • Updated comment to mention both Android and MacCatalyst as platforms that don't support Console.Open* methods
  • ProcessUtils.ConfigureTerminalForChildProcesses.iOS.cs: Added internal static bool IsTerminal(SafeFileHandle? _) => false; — on iOS/tvOS/MacCatalyst there is no terminal, so this always returns false without any P/Invoke reference to Interop.Sys.IsATty.

  • ProcessUtils.ConfigureTerminalForChildProcesses.Unix.cs: Added internal static bool IsTerminal(SafeFileHandle? standardHandle) => standardHandle is not null && Interop.Sys.IsATty(standardHandle); — on other Unix platforms, delegates to the native IsATty check.

  • ProcessUtils.cs:

    • Added PlatformSupportsConsole (!(IsAndroid() || IsMacCatalyst())) to gate Console.OpenStandard*Handle() calls
    • Added PlatformSupportsProcessStartAndKill (replaces the previous double-negative PlatformDoesNotSupportProcessStartAndKill) with a positive sense consistent with PlatformSupportsConsole; all call sites updated to if (!ProcessUtils.PlatformSupportsProcessStartAndKill)
    • Moved the platform support check to the shared file (from ProcessUtils.Unix.cs) and added early checks in Process.Start() and SafeProcessHandle.Start()
  • ProcessTests.cs: Added TestPlatforms.MacCatalyst to the [SkipOnPlatform] attributes on ProcessStart_UseShellExecute_Executes and ProcessStart_UseShellExecute_WorkingDirectory, alongside the existing iOS and tvOS skips, since MacCatalyst shares the same shell script execution restrictions

  • ProcessHandlesTests.cs: Extended [SkipOnPlatform] to include iOS, tvOS, and MacCatalyst (in addition to Android); updated skip reason to "sh is not available in the mobile platform sandbox" for greater precision

  • System.Diagnostics.Process.Tests.csproj:

    • Added $(NetCoreAppCurrent)-maccatalyst to TargetFrameworks
    • Included maccatalyst in the Unix compile condition (picks up ProcessTests.Unix.cs, ProcessThreadTests.Unix.cs, and related interop)
    • Consolidated the android and maccatalyst ItemGroup entries into a single group for ProcessTests.Mobile.cs

CopilotAI linked an issue Mar 30, 2026 that may be closed by this pull request
@github-actionsgithub-actionsBot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Mar 30, 2026
- Add $(NetCoreAppCurrent)-maccatalyst to TargetFrameworks
- Include Unix interop files for maccatalyst target
- Create ProcessTests.MacCatalyst.cs with tests for Process.Start,
I/O redirection, process killing, exit codes, and process names
modeled on ProcessTests.Android.cs
Fixes: tests cannot run on MacCatalyst because RemoteExecutor is not supported
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/6cbdf5a5-19f7-46b1-acab-3a4a9f5a6dde
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
CopilotAI changed the title [WIP] Add Process tests for MacCatalyst supportAdd Process tests for MacCatalystMar 30, 2026
CopilotAI requested a review from adamsitnikMarch 30, 2026 11:15
@github-actionsgithub-actionsBot added area-System.Diagnostics.Process and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Mar 30, 2026
@adamsitnik

Copy link
Copy Markdown
Member

/azp list

@azure-pipelines

This comment was marked as resolved.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/area-system-diagnostics-process
See info in area-owners.md if you want to be subscribed.

@adamsitnik

Copy link
Copy Markdown
Member

/azp run runtime-maccatalyst

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@adamsitnik

adamsitnik commented Mar 30, 2026

Copy link
Copy Markdown
Member

@rolfbjarne@akoeplinger@vitek-karas I am going to need some help, as this failure does not tell me anything:

[08:54:54]info:Preparingrun for maccatalyst
[08:54:54]info:Gettingapp bundle information from '/tmp/helix/working/B5070A06/w/A65C0914/e/System.Diagnostics.Process.Tests.app'..
[08:54:54] dbug: Property UIRequiredDeviceCapabilities not present in Info.plist, assuming 32-bitis not supported.Error: Failed to get bundle information: Print: Entry,"UIRequiredDeviceCapabilities",Does Not Exist[08:54:54]dbug:***Executing 'System.Diagnostics.Process.Tests' on MacCatalyst ***

@jkotas

Copy link
Copy Markdown
Member

All existing Process tests rely on RemoteExecutor, which is unsupported on MacCatalyst.

Why is RemoteExecutor not unsupported on MacCatalyst?

If we are able to start child processes on MacCatalyst, RemoteExecutor should work just fine.

@adamsitnik

Copy link
Copy Markdown
Member

Why is RemoteExecutor not unsupported on MacCatalyst?

If we are able to start child processes on MacCatalyst, RemoteExecutor should work just fine.

I don't know, but I agree it would be the best long-term solution.

@rolfbjarne

Copy link
Copy Markdown
Member
dbug:PropertyUIRequiredDeviceCapabilities not present in Info.plist,assuming32-bitis not supported.Error: Failed to get bundle information: Print: Entry,"UIRequiredDeviceCapabilities",DoesNot Exist

This comes from here, and just looks diagnostic.

The problem is that there were test failures:

[08:55:13] info: Application finished the test run successfully with some failed tests
[08:55:13] info: Tests run: 299 Passed: 130 Inconclusive: 0 Failed: 28 Ignored: 141

which can be seen in the Tests tab here: https://dev.azure.com/dnceng-public/public/_build/results?buildId=1357894&view=ms.vss-test-web.build-test-results-tab&runId=37954004&resultId=157642&paneView=debug

@adamsitnikadamsitnik added the NO-REVIEW Experimental/testing PR, do NOT review it label Mar 31, 2026
@adamsitnik

Copy link
Copy Markdown
Member

/azp run runtime-maccatalyst

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Co-authored-by: Adam Sitnik <adam.sitnik@gmail.com>
@adamsitnik

Copy link
Copy Markdown
Member

/azp run runtime-maccatalyst

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

…rocessStartAndKill
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/634abc6a-3308-4bf9-8cbc-0ee291fe1533
Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
CopilotAI requested a review from jkotasApril 2, 2026 14:52
@adamsitnik

Copy link
Copy Markdown
Member

/azp run runtime-maccatalyst

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@adamsitnik
adamsitnik enabled auto-merge (squash) April 2, 2026 15:07
@github-actions

Copy link
Copy Markdown
Contributor

🤖 Copilot Code Review — PR #126306

Note

This review was generated by Copilot.

Holistic Assessment

Motivation: This PR adds MacCatalyst platform support to the System.Diagnostics.Process test suite and refactors platform-specific checks for clarity. The motivation is well-justified — MacCatalyst can run processes (unlike iOS/tvOS) but lacks Console support (like Android), and the existing code/tests didn't account for this.

Approach: The approach is clean and well-structured. It centralizes platform-specific logic into named properties (PlatformSupportsProcessStartAndKill, PlatformSupportsConsole), extracts IsTerminal into platform files (Unix: IsATty, iOS: false), deduplicates the UsesTerminal check, and consolidates Android+MacCatalyst tests into a shared ProcessTests.Mobile.cs.

Summary: ✅ LGTM. The code changes are correct and improve the codebase. The renamed property uses positive naming conventions, all callers are properly updated with no orphaned references, the PlatformNotSupportedException guard covers all public entry points, and no new public API surface is introduced. Minor observations noted below.


Detailed Findings

✅ Platform check refactoring is correct and complete

  • PlatformDoesNotSupportProcessStartAndKillPlatformSupportsProcessStartAndKill (positive naming ✓)
  • Moved from Unix-only ProcessUtils.Unix.cs to shared ProcessUtils.cs (now usable from non-Unix-specific code ✓)
  • No orphaned references to the old name remain anywhere in the repo ✓
  • All 4 callers (Process.Start, Process.Kill, SafeProcessHandle.Start, SafeProcessHandle.SignalCore) are updated ✓

✅ Console guard centralization is correct

PlatformSupportsConsole => !(OperatingSystem.IsAndroid() || OperatingSystem.IsMacCatalyst()) correctly replaces the previous inline !OperatingSystem.IsAndroid() checks. All 6 Console.OpenStandard*Handle() call sites (3 in Process.cs, 3 in SafeProcessHandle.cs) are properly guarded with this property.

✅ PlatformNotSupportedException guard coverage is complete

The guard was removed from the internal SafeProcessHandle.StartCore and placed in both public callers (Process.Start at line 1250, SafeProcessHandle.Start at line 101). Since StartCore is internal and only reachable through these two public entry points (verified: Process.StartCoreSafeProcessHandle.StartCore, and SafeProcessHandle.StartStartCore), the coverage is equivalent.

✅ IsTerminal extraction is well-designed

The IsTerminal helper is correctly split across platform files:

  • Unix (ConfigureTerminalForChildProcesses.Unix.cs): standardHandle is not null && Interop.Sys.IsATty(standardHandle) — performs the actual TTY check
  • iOS (ConfigureTerminalForChildProcesses.iOS.cs): => false — terminals are never available on iOS-like platforms (includes MacCatalyst via IsiOSLike csproj condition)

The iOS version uses discard parameter _ correctly, matching the pattern of AreChildrenUsingTerminal => false and the no-op ConfigureTerminalForChildProcesses in the same file.

✅ Test consolidation is appropriate

Renaming ProcessTests.Android.csProcessTests.Mobile.cs and applying [PlatformSpecific(TestPlatforms.Android | TestPlatforms.MacCatalyst)] at the class level is clean. The individual [PlatformSpecific(TestPlatforms.Android)] attributes are correctly removed since the class-level attribute covers them.

✅ csproj configuration is correct

  • $(NetCoreAppCurrent)-maccatalyst added to TargetFrameworks ✓
  • maccatalyst added to the Unix/interop ItemGroup condition ✓
  • Mobile test file included for both android and maccatalyst

💡 ProcessTests.Unix.cs UseShellExecute tests — low-risk gap

Two UseShellExecute tests in ProcessTests.Unix.cs (lines 114, 134: ProcessStart_UseShellExecute_OnUnix_OpenMissingFile_DoesNotThrow and ProcessStart_UseShellExecute_OnUnix_SuccessWhenProgramInstalled) skip iOS/tvOS/Android/Browser but not MacCatalyst. Now that maccatalyst is a test target, these tests will be compiled for MacCatalyst. However, both are [OuterLoop] tests so they won't affect regular CI, and UseShellExecute via the open command likely works on MacCatalyst (it's macOS-based). This is low-risk and could be addressed in a follow-up if needed.

💡 ProcessHandlesTests skip message

The previous version had two [SkipOnPlatform] attributes with distinct reasons ("sh is not available on Android" vs "Not supported on Apple mobile platforms"). These were consolidated into a single attribute with a combined message ("sh is not available in the mobile platform sandbox"). This is a minor readability tradeoff — the combined message is slightly less precise but avoids attribute duplication. Fine as-is.

Generated by Code Review for issue #126306 ·

@adamsitnik
adamsitnik merged commit 1f9be20 into mainApr 2, 2026
94 checks passed
@adamsitnik
adamsitnik deleted the copilot/implement-process-tests-maccatalyst branch April 2, 2026 17:56
radekdoulik pushed a commit to radekdoulik/runtime that referenced this pull request Apr 9, 2026
…#126306)
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: Adam Sitnik <adam.sitnik@gmail.com>
Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@github-actionsgithub-actionsBot locked and limited conversation to collaborators May 3, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement Process tests for MacCatalyst

5 participants

@adamsitnik@jkotas@rolfbjarne
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Add Process tests for MacCatalyst and fix MacCatalyst support - #126306

Merged
adamsitnik merged 17 commits into
mainfrom
copilot/implement-process-tests-maccatalyst
Apr 2, 2026
Merged

Add Process tests for MacCatalyst and fix MacCatalyst support#126306
adamsitnik merged 17 commits into
mainfrom
copilot/implement-process-tests-maccatalyst

Conversation

CopilotAI commented Mar 30, 2026

Copy link
Copy Markdown
Contributor

All existing Process tests rely on RemoteExecutor, which is unsupported on MacCatalyst. This adds mobile-platform Process tests using simple Unix tools (ls, sleep) available in both the Android and MacCatalyst environments, and fixes MacCatalyst-specific bugs in the Process implementation.

Changes

  • ProcessTests.Mobile.cs (new, replaces separate ProcessTests.Android.cs and ProcessTests.MacCatalyst.cs): Six mobile Process tests shared between Android and MacCatalyst via [PlatformSpecific(TestPlatforms.Android | TestPlatforms.MacCatalyst)], covering:

    • Basic process start with inherited I/O
    • Redirected stdout/stderr
    • Process.Kill() termination
    • Non-zero exit code on command failure
    • ProcessName availability for a running process
    • StandardInputHandle/StandardOutputHandle/StandardErrorHandle via anonymous pipes
  • SafeProcessHandle.Unix.cs:

    • Extracted a new private static bool UsesTerminal(SafeFileHandle?, SafeFileHandle?, SafeFileHandle?) helper method (with explanatory comments preserved) called from both StartCore and StartWithShellExecute, eliminating the previous inconsistency between the two call sites. UsesTerminal now delegates to ProcessUtils.IsTerminal (see below), removing the inline Interop.Sys.IsATty calls and the PlatformSupportsConsole guard from this site.
    • Updated comment to mention both Android and MacCatalyst as platforms that don't support Console.Open* methods
  • ProcessUtils.ConfigureTerminalForChildProcesses.iOS.cs: Added internal static bool IsTerminal(SafeFileHandle? _) => false; — on iOS/tvOS/MacCatalyst there is no terminal, so this always returns false without any P/Invoke reference to Interop.Sys.IsATty.

  • ProcessUtils.ConfigureTerminalForChildProcesses.Unix.cs: Added internal static bool IsTerminal(SafeFileHandle? standardHandle) => standardHandle is not null && Interop.Sys.IsATty(standardHandle); — on other Unix platforms, delegates to the native IsATty check.

  • ProcessUtils.cs:

    • Added PlatformSupportsConsole (!(IsAndroid() || IsMacCatalyst())) to gate Console.OpenStandard*Handle() calls
    • Added PlatformSupportsProcessStartAndKill (replaces the previous double-negative PlatformDoesNotSupportProcessStartAndKill) with a positive sense consistent with PlatformSupportsConsole; all call sites updated to if (!ProcessUtils.PlatformSupportsProcessStartAndKill)
    • Moved the platform support check to the shared file (from ProcessUtils.Unix.cs) and added early checks in Process.Start() and SafeProcessHandle.Start()
  • ProcessTests.cs: Added TestPlatforms.MacCatalyst to the [SkipOnPlatform] attributes on ProcessStart_UseShellExecute_Executes and ProcessStart_UseShellExecute_WorkingDirectory, alongside the existing iOS and tvOS skips, since MacCatalyst shares the same shell script execution restrictions

  • ProcessHandlesTests.cs: Extended [SkipOnPlatform] to include iOS, tvOS, and MacCatalyst (in addition to Android); updated skip reason to "sh is not available in the mobile platform sandbox" for greater precision

  • System.Diagnostics.Process.Tests.csproj:

    • Added $(NetCoreAppCurrent)-maccatalyst to TargetFrameworks
    • Included maccatalyst in the Unix compile condition (picks up ProcessTests.Unix.cs, ProcessThreadTests.Unix.cs, and related interop)
    • Consolidated the android and maccatalyst ItemGroup entries into a single group for ProcessTests.Mobile.cs

CopilotAI linked an issue Mar 30, 2026 that may be closed by this pull request
@github-actionsgithub-actionsBot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Mar 30, 2026
- Add $(NetCoreAppCurrent)-maccatalyst to TargetFrameworks
- Include Unix interop files for maccatalyst target
- Create ProcessTests.MacCatalyst.cs with tests for Process.Start,
I/O redirection, process killing, exit codes, and process names
modeled on ProcessTests.Android.cs
Fixes: tests cannot run on MacCatalyst because RemoteExecutor is not supported
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/6cbdf5a5-19f7-46b1-acab-3a4a9f5a6dde
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
CopilotAI changed the title [WIP] Add Process tests for MacCatalyst supportAdd Process tests for MacCatalystMar 30, 2026
CopilotAI requested a review from adamsitnikMarch 30, 2026 11:15
@github-actionsgithub-actionsBot added area-System.Diagnostics.Process and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Mar 30, 2026
@adamsitnik

Copy link
Copy Markdown
Member

/azp list

@azure-pipelines

This comment was marked as resolved.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/area-system-diagnostics-process
See info in area-owners.md if you want to be subscribed.

@adamsitnik

Copy link
Copy Markdown
Member

/azp run runtime-maccatalyst

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@adamsitnik

adamsitnik commented Mar 30, 2026

Copy link
Copy Markdown
Member

@rolfbjarne@akoeplinger@vitek-karas I am going to need some help, as this failure does not tell me anything:

[08:54:54]info:Preparingrun for maccatalyst
[08:54:54]info:Gettingapp bundle information from '/tmp/helix/working/B5070A06/w/A65C0914/e/System.Diagnostics.Process.Tests.app'..
[08:54:54] dbug: Property UIRequiredDeviceCapabilities not present in Info.plist, assuming 32-bitis not supported.Error: Failed to get bundle information: Print: Entry,"UIRequiredDeviceCapabilities",Does Not Exist[08:54:54]dbug:***Executing 'System.Diagnostics.Process.Tests' on MacCatalyst ***

@jkotas

Copy link
Copy Markdown
Member

All existing Process tests rely on RemoteExecutor, which is unsupported on MacCatalyst.

Why is RemoteExecutor not unsupported on MacCatalyst?

If we are able to start child processes on MacCatalyst, RemoteExecutor should work just fine.

@adamsitnik

Copy link
Copy Markdown
Member

Why is RemoteExecutor not unsupported on MacCatalyst?

If we are able to start child processes on MacCatalyst, RemoteExecutor should work just fine.

I don't know, but I agree it would be the best long-term solution.

@rolfbjarne

Copy link
Copy Markdown
Member
dbug:PropertyUIRequiredDeviceCapabilities not present in Info.plist,assuming32-bitis not supported.Error: Failed to get bundle information: Print: Entry,"UIRequiredDeviceCapabilities",DoesNot Exist

This comes from here, and just looks diagnostic.

The problem is that there were test failures:

[08:55:13] info: Application finished the test run successfully with some failed tests
[08:55:13] info: Tests run: 299 Passed: 130 Inconclusive: 0 Failed: 28 Ignored: 141

which can be seen in the Tests tab here: https://dev.azure.com/dnceng-public/public/_build/results?buildId=1357894&view=ms.vss-test-web.build-test-results-tab&runId=37954004&resultId=157642&paneView=debug

@adamsitnikadamsitnik added the NO-REVIEW Experimental/testing PR, do NOT review it label Mar 31, 2026
@adamsitnik

Copy link
Copy Markdown
Member

/azp run runtime-maccatalyst

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Co-authored-by: Adam Sitnik <adam.sitnik@gmail.com>
@adamsitnik

Copy link
Copy Markdown
Member

/azp run runtime-maccatalyst

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

…rocessStartAndKill
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/634abc6a-3308-4bf9-8cbc-0ee291fe1533
Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
CopilotAI requested a review from jkotasApril 2, 2026 14:52
@adamsitnik

Copy link
Copy Markdown
Member

/azp run runtime-maccatalyst

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@adamsitnik
adamsitnik enabled auto-merge (squash) April 2, 2026 15:07
@github-actions

Copy link
Copy Markdown
Contributor

🤖 Copilot Code Review — PR #126306

Note

This review was generated by Copilot.

Holistic Assessment

Motivation: This PR adds MacCatalyst platform support to the System.Diagnostics.Process test suite and refactors platform-specific checks for clarity. The motivation is well-justified — MacCatalyst can run processes (unlike iOS/tvOS) but lacks Console support (like Android), and the existing code/tests didn't account for this.

Approach: The approach is clean and well-structured. It centralizes platform-specific logic into named properties (PlatformSupportsProcessStartAndKill, PlatformSupportsConsole), extracts IsTerminal into platform files (Unix: IsATty, iOS: false), deduplicates the UsesTerminal check, and consolidates Android+MacCatalyst tests into a shared ProcessTests.Mobile.cs.

Summary: ✅ LGTM. The code changes are correct and improve the codebase. The renamed property uses positive naming conventions, all callers are properly updated with no orphaned references, the PlatformNotSupportedException guard covers all public entry points, and no new public API surface is introduced. Minor observations noted below.


Detailed Findings

✅ Platform check refactoring is correct and complete

  • PlatformDoesNotSupportProcessStartAndKillPlatformSupportsProcessStartAndKill (positive naming ✓)
  • Moved from Unix-only ProcessUtils.Unix.cs to shared ProcessUtils.cs (now usable from non-Unix-specific code ✓)
  • No orphaned references to the old name remain anywhere in the repo ✓
  • All 4 callers (Process.Start, Process.Kill, SafeProcessHandle.Start, SafeProcessHandle.SignalCore) are updated ✓

✅ Console guard centralization is correct

PlatformSupportsConsole => !(OperatingSystem.IsAndroid() || OperatingSystem.IsMacCatalyst()) correctly replaces the previous inline !OperatingSystem.IsAndroid() checks. All 6 Console.OpenStandard*Handle() call sites (3 in Process.cs, 3 in SafeProcessHandle.cs) are properly guarded with this property.

✅ PlatformNotSupportedException guard coverage is complete

The guard was removed from the internal SafeProcessHandle.StartCore and placed in both public callers (Process.Start at line 1250, SafeProcessHandle.Start at line 101). Since StartCore is internal and only reachable through these two public entry points (verified: Process.StartCoreSafeProcessHandle.StartCore, and SafeProcessHandle.StartStartCore), the coverage is equivalent.

✅ IsTerminal extraction is well-designed

The IsTerminal helper is correctly split across platform files:

  • Unix (ConfigureTerminalForChildProcesses.Unix.cs): standardHandle is not null && Interop.Sys.IsATty(standardHandle) — performs the actual TTY check
  • iOS (ConfigureTerminalForChildProcesses.iOS.cs): => false — terminals are never available on iOS-like platforms (includes MacCatalyst via IsiOSLike csproj condition)

The iOS version uses discard parameter _ correctly, matching the pattern of AreChildrenUsingTerminal => false and the no-op ConfigureTerminalForChildProcesses in the same file.

✅ Test consolidation is appropriate

Renaming ProcessTests.Android.csProcessTests.Mobile.cs and applying [PlatformSpecific(TestPlatforms.Android | TestPlatforms.MacCatalyst)] at the class level is clean. The individual [PlatformSpecific(TestPlatforms.Android)] attributes are correctly removed since the class-level attribute covers them.

✅ csproj configuration is correct

  • $(NetCoreAppCurrent)-maccatalyst added to TargetFrameworks ✓
  • maccatalyst added to the Unix/interop ItemGroup condition ✓
  • Mobile test file included for both android and maccatalyst

💡 ProcessTests.Unix.cs UseShellExecute tests — low-risk gap

Two UseShellExecute tests in ProcessTests.Unix.cs (lines 114, 134: ProcessStart_UseShellExecute_OnUnix_OpenMissingFile_DoesNotThrow and ProcessStart_UseShellExecute_OnUnix_SuccessWhenProgramInstalled) skip iOS/tvOS/Android/Browser but not MacCatalyst. Now that maccatalyst is a test target, these tests will be compiled for MacCatalyst. However, both are [OuterLoop] tests so they won't affect regular CI, and UseShellExecute via the open command likely works on MacCatalyst (it's macOS-based). This is low-risk and could be addressed in a follow-up if needed.

💡 ProcessHandlesTests skip message

The previous version had two [SkipOnPlatform] attributes with distinct reasons ("sh is not available on Android" vs "Not supported on Apple mobile platforms"). These were consolidated into a single attribute with a combined message ("sh is not available in the mobile platform sandbox"). This is a minor readability tradeoff — the combined message is slightly less precise but avoids attribute duplication. Fine as-is.

Generated by Code Review for issue #126306 ·

@adamsitnik
adamsitnik merged commit 1f9be20 into mainApr 2, 2026
94 checks passed
@adamsitnik
adamsitnik deleted the copilot/implement-process-tests-maccatalyst branch April 2, 2026 17:56
radekdoulik pushed a commit to radekdoulik/runtime that referenced this pull request Apr 9, 2026
…#126306)
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: Adam Sitnik <adam.sitnik@gmail.com>
Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@github-actionsgithub-actionsBot locked and limited conversation to collaborators May 3, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement Process tests for MacCatalyst

5 participants

@adamsitnik@jkotas@rolfbjarne
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Add Process tests for MacCatalyst and fix MacCatalyst support - #126306

Merged
adamsitnik merged 17 commits into
mainfrom
copilot/implement-process-tests-maccatalyst
Apr 2, 2026
Merged

Add Process tests for MacCatalyst and fix MacCatalyst support#126306
adamsitnik merged 17 commits into
mainfrom
copilot/implement-process-tests-maccatalyst

Conversation

CopilotAI commented Mar 30, 2026

Copy link
Copy Markdown
Contributor

All existing Process tests rely on RemoteExecutor, which is unsupported on MacCatalyst. This adds mobile-platform Process tests using simple Unix tools (ls, sleep) available in both the Android and MacCatalyst environments, and fixes MacCatalyst-specific bugs in the Process implementation.

Changes

  • ProcessTests.Mobile.cs (new, replaces separate ProcessTests.Android.cs and ProcessTests.MacCatalyst.cs): Six mobile Process tests shared between Android and MacCatalyst via [PlatformSpecific(TestPlatforms.Android | TestPlatforms.MacCatalyst)], covering:

    • Basic process start with inherited I/O
    • Redirected stdout/stderr
    • Process.Kill() termination
    • Non-zero exit code on command failure
    • ProcessName availability for a running process
    • StandardInputHandle/StandardOutputHandle/StandardErrorHandle via anonymous pipes
  • SafeProcessHandle.Unix.cs:

    • Extracted a new private static bool UsesTerminal(SafeFileHandle?, SafeFileHandle?, SafeFileHandle?) helper method (with explanatory comments preserved) called from both StartCore and StartWithShellExecute, eliminating the previous inconsistency between the two call sites. UsesTerminal now delegates to ProcessUtils.IsTerminal (see below), removing the inline Interop.Sys.IsATty calls and the PlatformSupportsConsole guard from this site.
    • Updated comment to mention both Android and MacCatalyst as platforms that don't support Console.Open* methods
  • ProcessUtils.ConfigureTerminalForChildProcesses.iOS.cs: Added internal static bool IsTerminal(SafeFileHandle? _) => false; — on iOS/tvOS/MacCatalyst there is no terminal, so this always returns false without any P/Invoke reference to Interop.Sys.IsATty.

  • ProcessUtils.ConfigureTerminalForChildProcesses.Unix.cs: Added internal static bool IsTerminal(SafeFileHandle? standardHandle) => standardHandle is not null && Interop.Sys.IsATty(standardHandle); — on other Unix platforms, delegates to the native IsATty check.

  • ProcessUtils.cs:

    • Added PlatformSupportsConsole (!(IsAndroid() || IsMacCatalyst())) to gate Console.OpenStandard*Handle() calls
    • Added PlatformSupportsProcessStartAndKill (replaces the previous double-negative PlatformDoesNotSupportProcessStartAndKill) with a positive sense consistent with PlatformSupportsConsole; all call sites updated to if (!ProcessUtils.PlatformSupportsProcessStartAndKill)
    • Moved the platform support check to the shared file (from ProcessUtils.Unix.cs) and added early checks in Process.Start() and SafeProcessHandle.Start()
  • ProcessTests.cs: Added TestPlatforms.MacCatalyst to the [SkipOnPlatform] attributes on ProcessStart_UseShellExecute_Executes and ProcessStart_UseShellExecute_WorkingDirectory, alongside the existing iOS and tvOS skips, since MacCatalyst shares the same shell script execution restrictions

  • ProcessHandlesTests.cs: Extended [SkipOnPlatform] to include iOS, tvOS, and MacCatalyst (in addition to Android); updated skip reason to "sh is not available in the mobile platform sandbox" for greater precision

  • System.Diagnostics.Process.Tests.csproj:

    • Added $(NetCoreAppCurrent)-maccatalyst to TargetFrameworks
    • Included maccatalyst in the Unix compile condition (picks up ProcessTests.Unix.cs, ProcessThreadTests.Unix.cs, and related interop)
    • Consolidated the android and maccatalyst ItemGroup entries into a single group for ProcessTests.Mobile.cs

CopilotAI linked an issue Mar 30, 2026 that may be closed by this pull request
@github-actionsgithub-actionsBot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Mar 30, 2026
- Add $(NetCoreAppCurrent)-maccatalyst to TargetFrameworks
- Include Unix interop files for maccatalyst target
- Create ProcessTests.MacCatalyst.cs with tests for Process.Start,
I/O redirection, process killing, exit codes, and process names
modeled on ProcessTests.Android.cs
Fixes: tests cannot run on MacCatalyst because RemoteExecutor is not supported
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/6cbdf5a5-19f7-46b1-acab-3a4a9f5a6dde
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
CopilotAI changed the title [WIP] Add Process tests for MacCatalyst supportAdd Process tests for MacCatalystMar 30, 2026
CopilotAI requested a review from adamsitnikMarch 30, 2026 11:15
@github-actionsgithub-actionsBot added area-System.Diagnostics.Process and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Mar 30, 2026
@adamsitnik

Copy link
Copy Markdown
Member

/azp list

@azure-pipelines

This comment was marked as resolved.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/area-system-diagnostics-process
See info in area-owners.md if you want to be subscribed.

@adamsitnik

Copy link
Copy Markdown
Member

/azp run runtime-maccatalyst

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@adamsitnik

adamsitnik commented Mar 30, 2026

Copy link
Copy Markdown
Member

@rolfbjarne@akoeplinger@vitek-karas I am going to need some help, as this failure does not tell me anything:

[08:54:54]info:Preparingrun for maccatalyst
[08:54:54]info:Gettingapp bundle information from '/tmp/helix/working/B5070A06/w/A65C0914/e/System.Diagnostics.Process.Tests.app'..
[08:54:54] dbug: Property UIRequiredDeviceCapabilities not present in Info.plist, assuming 32-bitis not supported.Error: Failed to get bundle information: Print: Entry,"UIRequiredDeviceCapabilities",Does Not Exist[08:54:54]dbug:***Executing 'System.Diagnostics.Process.Tests' on MacCatalyst ***

@jkotas

Copy link
Copy Markdown
Member

All existing Process tests rely on RemoteExecutor, which is unsupported on MacCatalyst.

Why is RemoteExecutor not unsupported on MacCatalyst?

If we are able to start child processes on MacCatalyst, RemoteExecutor should work just fine.

@adamsitnik

Copy link
Copy Markdown
Member

Why is RemoteExecutor not unsupported on MacCatalyst?

If we are able to start child processes on MacCatalyst, RemoteExecutor should work just fine.

I don't know, but I agree it would be the best long-term solution.

@rolfbjarne

Copy link
Copy Markdown
Member
dbug:PropertyUIRequiredDeviceCapabilities not present in Info.plist,assuming32-bitis not supported.Error: Failed to get bundle information: Print: Entry,"UIRequiredDeviceCapabilities",DoesNot Exist

This comes from here, and just looks diagnostic.

The problem is that there were test failures:

[08:55:13] info: Application finished the test run successfully with some failed tests
[08:55:13] info: Tests run: 299 Passed: 130 Inconclusive: 0 Failed: 28 Ignored: 141

which can be seen in the Tests tab here: https://dev.azure.com/dnceng-public/public/_build/results?buildId=1357894&view=ms.vss-test-web.build-test-results-tab&runId=37954004&resultId=157642&paneView=debug

@adamsitnikadamsitnik added the NO-REVIEW Experimental/testing PR, do NOT review it label Mar 31, 2026
@adamsitnik

Copy link
Copy Markdown
Member

/azp run runtime-maccatalyst

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Co-authored-by: Adam Sitnik <adam.sitnik@gmail.com>
@adamsitnik

Copy link
Copy Markdown
Member

/azp run runtime-maccatalyst

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

…rocessStartAndKill
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/634abc6a-3308-4bf9-8cbc-0ee291fe1533
Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
CopilotAI requested a review from jkotasApril 2, 2026 14:52
@adamsitnik

Copy link
Copy Markdown
Member

/azp run runtime-maccatalyst

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@adamsitnik
adamsitnik enabled auto-merge (squash) April 2, 2026 15:07
@github-actions

Copy link
Copy Markdown
Contributor

🤖 Copilot Code Review — PR #126306

Note

This review was generated by Copilot.

Holistic Assessment

Motivation: This PR adds MacCatalyst platform support to the System.Diagnostics.Process test suite and refactors platform-specific checks for clarity. The motivation is well-justified — MacCatalyst can run processes (unlike iOS/tvOS) but lacks Console support (like Android), and the existing code/tests didn't account for this.

Approach: The approach is clean and well-structured. It centralizes platform-specific logic into named properties (PlatformSupportsProcessStartAndKill, PlatformSupportsConsole), extracts IsTerminal into platform files (Unix: IsATty, iOS: false), deduplicates the UsesTerminal check, and consolidates Android+MacCatalyst tests into a shared ProcessTests.Mobile.cs.

Summary: ✅ LGTM. The code changes are correct and improve the codebase. The renamed property uses positive naming conventions, all callers are properly updated with no orphaned references, the PlatformNotSupportedException guard covers all public entry points, and no new public API surface is introduced. Minor observations noted below.


Detailed Findings

✅ Platform check refactoring is correct and complete

  • PlatformDoesNotSupportProcessStartAndKillPlatformSupportsProcessStartAndKill (positive naming ✓)
  • Moved from Unix-only ProcessUtils.Unix.cs to shared ProcessUtils.cs (now usable from non-Unix-specific code ✓)
  • No orphaned references to the old name remain anywhere in the repo ✓
  • All 4 callers (Process.Start, Process.Kill, SafeProcessHandle.Start, SafeProcessHandle.SignalCore) are updated ✓

✅ Console guard centralization is correct

PlatformSupportsConsole => !(OperatingSystem.IsAndroid() || OperatingSystem.IsMacCatalyst()) correctly replaces the previous inline !OperatingSystem.IsAndroid() checks. All 6 Console.OpenStandard*Handle() call sites (3 in Process.cs, 3 in SafeProcessHandle.cs) are properly guarded with this property.

✅ PlatformNotSupportedException guard coverage is complete

The guard was removed from the internal SafeProcessHandle.StartCore and placed in both public callers (Process.Start at line 1250, SafeProcessHandle.Start at line 101). Since StartCore is internal and only reachable through these two public entry points (verified: Process.StartCoreSafeProcessHandle.StartCore, and SafeProcessHandle.StartStartCore), the coverage is equivalent.

✅ IsTerminal extraction is well-designed

The IsTerminal helper is correctly split across platform files:

  • Unix (ConfigureTerminalForChildProcesses.Unix.cs): standardHandle is not null && Interop.Sys.IsATty(standardHandle) — performs the actual TTY check
  • iOS (ConfigureTerminalForChildProcesses.iOS.cs): => false — terminals are never available on iOS-like platforms (includes MacCatalyst via IsiOSLike csproj condition)

The iOS version uses discard parameter _ correctly, matching the pattern of AreChildrenUsingTerminal => false and the no-op ConfigureTerminalForChildProcesses in the same file.

✅ Test consolidation is appropriate

Renaming ProcessTests.Android.csProcessTests.Mobile.cs and applying [PlatformSpecific(TestPlatforms.Android | TestPlatforms.MacCatalyst)] at the class level is clean. The individual [PlatformSpecific(TestPlatforms.Android)] attributes are correctly removed since the class-level attribute covers them.

✅ csproj configuration is correct

  • $(NetCoreAppCurrent)-maccatalyst added to TargetFrameworks ✓
  • maccatalyst added to the Unix/interop ItemGroup condition ✓
  • Mobile test file included for both android and maccatalyst

💡 ProcessTests.Unix.cs UseShellExecute tests — low-risk gap

Two UseShellExecute tests in ProcessTests.Unix.cs (lines 114, 134: ProcessStart_UseShellExecute_OnUnix_OpenMissingFile_DoesNotThrow and ProcessStart_UseShellExecute_OnUnix_SuccessWhenProgramInstalled) skip iOS/tvOS/Android/Browser but not MacCatalyst. Now that maccatalyst is a test target, these tests will be compiled for MacCatalyst. However, both are [OuterLoop] tests so they won't affect regular CI, and UseShellExecute via the open command likely works on MacCatalyst (it's macOS-based). This is low-risk and could be addressed in a follow-up if needed.

💡 ProcessHandlesTests skip message

The previous version had two [SkipOnPlatform] attributes with distinct reasons ("sh is not available on Android" vs "Not supported on Apple mobile platforms"). These were consolidated into a single attribute with a combined message ("sh is not available in the mobile platform sandbox"). This is a minor readability tradeoff — the combined message is slightly less precise but avoids attribute duplication. Fine as-is.

Generated by Code Review for issue #126306 ·

@adamsitnik
adamsitnik merged commit 1f9be20 into mainApr 2, 2026
94 checks passed
@adamsitnik
adamsitnik deleted the copilot/implement-process-tests-maccatalyst branch April 2, 2026 17:56
radekdoulik pushed a commit to radekdoulik/runtime that referenced this pull request Apr 9, 2026
…#126306)
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: Adam Sitnik <adam.sitnik@gmail.com>
Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@github-actionsgithub-actionsBot locked and limited conversation to collaborators May 3, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement Process tests for MacCatalyst

5 participants

@adamsitnik@jkotas@rolfbjarne
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Add Process tests for MacCatalyst and fix MacCatalyst support - #126306

Merged
adamsitnik merged 17 commits into
mainfrom
copilot/implement-process-tests-maccatalyst
Apr 2, 2026
Merged

Add Process tests for MacCatalyst and fix MacCatalyst support#126306
adamsitnik merged 17 commits into
mainfrom
copilot/implement-process-tests-maccatalyst

Conversation

CopilotAI commented Mar 30, 2026

Copy link
Copy Markdown
Contributor

All existing Process tests rely on RemoteExecutor, which is unsupported on MacCatalyst. This adds mobile-platform Process tests using simple Unix tools (ls, sleep) available in both the Android and MacCatalyst environments, and fixes MacCatalyst-specific bugs in the Process implementation.

Changes

  • ProcessTests.Mobile.cs (new, replaces separate ProcessTests.Android.cs and ProcessTests.MacCatalyst.cs): Six mobile Process tests shared between Android and MacCatalyst via [PlatformSpecific(TestPlatforms.Android | TestPlatforms.MacCatalyst)], covering:

    • Basic process start with inherited I/O
    • Redirected stdout/stderr
    • Process.Kill() termination
    • Non-zero exit code on command failure
    • ProcessName availability for a running process
    • StandardInputHandle/StandardOutputHandle/StandardErrorHandle via anonymous pipes
  • SafeProcessHandle.Unix.cs:

    • Extracted a new private static bool UsesTerminal(SafeFileHandle?, SafeFileHandle?, SafeFileHandle?) helper method (with explanatory comments preserved) called from both StartCore and StartWithShellExecute, eliminating the previous inconsistency between the two call sites. UsesTerminal now delegates to ProcessUtils.IsTerminal (see below), removing the inline Interop.Sys.IsATty calls and the PlatformSupportsConsole guard from this site.
    • Updated comment to mention both Android and MacCatalyst as platforms that don't support Console.Open* methods
  • ProcessUtils.ConfigureTerminalForChildProcesses.iOS.cs: Added internal static bool IsTerminal(SafeFileHandle? _) => false; — on iOS/tvOS/MacCatalyst there is no terminal, so this always returns false without any P/Invoke reference to Interop.Sys.IsATty.

  • ProcessUtils.ConfigureTerminalForChildProcesses.Unix.cs: Added internal static bool IsTerminal(SafeFileHandle? standardHandle) => standardHandle is not null && Interop.Sys.IsATty(standardHandle); — on other Unix platforms, delegates to the native IsATty check.

  • ProcessUtils.cs:

    • Added PlatformSupportsConsole (!(IsAndroid() || IsMacCatalyst())) to gate Console.OpenStandard*Handle() calls
    • Added PlatformSupportsProcessStartAndKill (replaces the previous double-negative PlatformDoesNotSupportProcessStartAndKill) with a positive sense consistent with PlatformSupportsConsole; all call sites updated to if (!ProcessUtils.PlatformSupportsProcessStartAndKill)
    • Moved the platform support check to the shared file (from ProcessUtils.Unix.cs) and added early checks in Process.Start() and SafeProcessHandle.Start()
  • ProcessTests.cs: Added TestPlatforms.MacCatalyst to the [SkipOnPlatform] attributes on ProcessStart_UseShellExecute_Executes and ProcessStart_UseShellExecute_WorkingDirectory, alongside the existing iOS and tvOS skips, since MacCatalyst shares the same shell script execution restrictions

  • ProcessHandlesTests.cs: Extended [SkipOnPlatform] to include iOS, tvOS, and MacCatalyst (in addition to Android); updated skip reason to "sh is not available in the mobile platform sandbox" for greater precision

  • System.Diagnostics.Process.Tests.csproj:

    • Added $(NetCoreAppCurrent)-maccatalyst to TargetFrameworks
    • Included maccatalyst in the Unix compile condition (picks up ProcessTests.Unix.cs, ProcessThreadTests.Unix.cs, and related interop)
    • Consolidated the android and maccatalyst ItemGroup entries into a single group for ProcessTests.Mobile.cs

CopilotAI linked an issue Mar 30, 2026 that may be closed by this pull request
@github-actionsgithub-actionsBot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Mar 30, 2026
- Add $(NetCoreAppCurrent)-maccatalyst to TargetFrameworks
- Include Unix interop files for maccatalyst target
- Create ProcessTests.MacCatalyst.cs with tests for Process.Start,
I/O redirection, process killing, exit codes, and process names
modeled on ProcessTests.Android.cs
Fixes: tests cannot run on MacCatalyst because RemoteExecutor is not supported
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/6cbdf5a5-19f7-46b1-acab-3a4a9f5a6dde
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
CopilotAI changed the title [WIP] Add Process tests for MacCatalyst supportAdd Process tests for MacCatalystMar 30, 2026
CopilotAI requested a review from adamsitnikMarch 30, 2026 11:15
@github-actionsgithub-actionsBot added area-System.Diagnostics.Process and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Mar 30, 2026
@adamsitnik

Copy link
Copy Markdown
Member

/azp list

@azure-pipelines

This comment was marked as resolved.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/area-system-diagnostics-process
See info in area-owners.md if you want to be subscribed.

@adamsitnik

Copy link
Copy Markdown
Member

/azp run runtime-maccatalyst

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@adamsitnik

adamsitnik commented Mar 30, 2026

Copy link
Copy Markdown
Member

@rolfbjarne@akoeplinger@vitek-karas I am going to need some help, as this failure does not tell me anything:

[08:54:54]info:Preparingrun for maccatalyst
[08:54:54]info:Gettingapp bundle information from '/tmp/helix/working/B5070A06/w/A65C0914/e/System.Diagnostics.Process.Tests.app'..
[08:54:54] dbug: Property UIRequiredDeviceCapabilities not present in Info.plist, assuming 32-bitis not supported.Error: Failed to get bundle information: Print: Entry,"UIRequiredDeviceCapabilities",Does Not Exist[08:54:54]dbug:***Executing 'System.Diagnostics.Process.Tests' on MacCatalyst ***

@jkotas

Copy link
Copy Markdown
Member

All existing Process tests rely on RemoteExecutor, which is unsupported on MacCatalyst.

Why is RemoteExecutor not unsupported on MacCatalyst?

If we are able to start child processes on MacCatalyst, RemoteExecutor should work just fine.

@adamsitnik

Copy link
Copy Markdown
Member

Why is RemoteExecutor not unsupported on MacCatalyst?

If we are able to start child processes on MacCatalyst, RemoteExecutor should work just fine.

I don't know, but I agree it would be the best long-term solution.

@rolfbjarne

Copy link
Copy Markdown
Member
dbug:PropertyUIRequiredDeviceCapabilities not present in Info.plist,assuming32-bitis not supported.Error: Failed to get bundle information: Print: Entry,"UIRequiredDeviceCapabilities",DoesNot Exist

This comes from here, and just looks diagnostic.

The problem is that there were test failures:

[08:55:13] info: Application finished the test run successfully with some failed tests
[08:55:13] info: Tests run: 299 Passed: 130 Inconclusive: 0 Failed: 28 Ignored: 141

which can be seen in the Tests tab here: https://dev.azure.com/dnceng-public/public/_build/results?buildId=1357894&view=ms.vss-test-web.build-test-results-tab&runId=37954004&resultId=157642&paneView=debug

@adamsitnikadamsitnik added the NO-REVIEW Experimental/testing PR, do NOT review it label Mar 31, 2026
@adamsitnik

Copy link
Copy Markdown
Member

/azp run runtime-maccatalyst

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Co-authored-by: Adam Sitnik <adam.sitnik@gmail.com>
@adamsitnik

Copy link
Copy Markdown
Member

/azp run runtime-maccatalyst

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

…rocessStartAndKill
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/634abc6a-3308-4bf9-8cbc-0ee291fe1533
Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
CopilotAI requested a review from jkotasApril 2, 2026 14:52
@adamsitnik

Copy link
Copy Markdown
Member

/azp run runtime-maccatalyst

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@adamsitnik
adamsitnik enabled auto-merge (squash) April 2, 2026 15:07
@github-actions

Copy link
Copy Markdown
Contributor

🤖 Copilot Code Review — PR #126306

Note

This review was generated by Copilot.

Holistic Assessment

Motivation: This PR adds MacCatalyst platform support to the System.Diagnostics.Process test suite and refactors platform-specific checks for clarity. The motivation is well-justified — MacCatalyst can run processes (unlike iOS/tvOS) but lacks Console support (like Android), and the existing code/tests didn't account for this.

Approach: The approach is clean and well-structured. It centralizes platform-specific logic into named properties (PlatformSupportsProcessStartAndKill, PlatformSupportsConsole), extracts IsTerminal into platform files (Unix: IsATty, iOS: false), deduplicates the UsesTerminal check, and consolidates Android+MacCatalyst tests into a shared ProcessTests.Mobile.cs.

Summary: ✅ LGTM. The code changes are correct and improve the codebase. The renamed property uses positive naming conventions, all callers are properly updated with no orphaned references, the PlatformNotSupportedException guard covers all public entry points, and no new public API surface is introduced. Minor observations noted below.


Detailed Findings

✅ Platform check refactoring is correct and complete

  • PlatformDoesNotSupportProcessStartAndKillPlatformSupportsProcessStartAndKill (positive naming ✓)
  • Moved from Unix-only ProcessUtils.Unix.cs to shared ProcessUtils.cs (now usable from non-Unix-specific code ✓)
  • No orphaned references to the old name remain anywhere in the repo ✓
  • All 4 callers (Process.Start, Process.Kill, SafeProcessHandle.Start, SafeProcessHandle.SignalCore) are updated ✓

✅ Console guard centralization is correct

PlatformSupportsConsole => !(OperatingSystem.IsAndroid() || OperatingSystem.IsMacCatalyst()) correctly replaces the previous inline !OperatingSystem.IsAndroid() checks. All 6 Console.OpenStandard*Handle() call sites (3 in Process.cs, 3 in SafeProcessHandle.cs) are properly guarded with this property.

✅ PlatformNotSupportedException guard coverage is complete

The guard was removed from the internal SafeProcessHandle.StartCore and placed in both public callers (Process.Start at line 1250, SafeProcessHandle.Start at line 101). Since StartCore is internal and only reachable through these two public entry points (verified: Process.StartCoreSafeProcessHandle.StartCore, and SafeProcessHandle.StartStartCore), the coverage is equivalent.

✅ IsTerminal extraction is well-designed

The IsTerminal helper is correctly split across platform files:

  • Unix (ConfigureTerminalForChildProcesses.Unix.cs): standardHandle is not null && Interop.Sys.IsATty(standardHandle) — performs the actual TTY check
  • iOS (ConfigureTerminalForChildProcesses.iOS.cs): => false — terminals are never available on iOS-like platforms (includes MacCatalyst via IsiOSLike csproj condition)

The iOS version uses discard parameter _ correctly, matching the pattern of AreChildrenUsingTerminal => false and the no-op ConfigureTerminalForChildProcesses in the same file.

✅ Test consolidation is appropriate

Renaming ProcessTests.Android.csProcessTests.Mobile.cs and applying [PlatformSpecific(TestPlatforms.Android | TestPlatforms.MacCatalyst)] at the class level is clean. The individual [PlatformSpecific(TestPlatforms.Android)] attributes are correctly removed since the class-level attribute covers them.

✅ csproj configuration is correct

  • $(NetCoreAppCurrent)-maccatalyst added to TargetFrameworks ✓
  • maccatalyst added to the Unix/interop ItemGroup condition ✓
  • Mobile test file included for both android and maccatalyst

💡 ProcessTests.Unix.cs UseShellExecute tests — low-risk gap

Two UseShellExecute tests in ProcessTests.Unix.cs (lines 114, 134: ProcessStart_UseShellExecute_OnUnix_OpenMissingFile_DoesNotThrow and ProcessStart_UseShellExecute_OnUnix_SuccessWhenProgramInstalled) skip iOS/tvOS/Android/Browser but not MacCatalyst. Now that maccatalyst is a test target, these tests will be compiled for MacCatalyst. However, both are [OuterLoop] tests so they won't affect regular CI, and UseShellExecute via the open command likely works on MacCatalyst (it's macOS-based). This is low-risk and could be addressed in a follow-up if needed.

💡 ProcessHandlesTests skip message

The previous version had two [SkipOnPlatform] attributes with distinct reasons ("sh is not available on Android" vs "Not supported on Apple mobile platforms"). These were consolidated into a single attribute with a combined message ("sh is not available in the mobile platform sandbox"). This is a minor readability tradeoff — the combined message is slightly less precise but avoids attribute duplication. Fine as-is.

Generated by Code Review for issue #126306 ·

@adamsitnik
adamsitnik merged commit 1f9be20 into mainApr 2, 2026
94 checks passed
@adamsitnik
adamsitnik deleted the copilot/implement-process-tests-maccatalyst branch April 2, 2026 17:56
radekdoulik pushed a commit to radekdoulik/runtime that referenced this pull request Apr 9, 2026
…#126306)
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: Adam Sitnik <adam.sitnik@gmail.com>
Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@github-actionsgithub-actionsBot locked and limited conversation to collaborators May 3, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement Process tests for MacCatalyst

5 participants

@adamsitnik@jkotas@rolfbjarne
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Add Process tests for MacCatalyst and fix MacCatalyst support - #126306

Merged
adamsitnik merged 17 commits into
mainfrom
copilot/implement-process-tests-maccatalyst
Apr 2, 2026
Merged

Add Process tests for MacCatalyst and fix MacCatalyst support#126306
adamsitnik merged 17 commits into
mainfrom
copilot/implement-process-tests-maccatalyst

Conversation

CopilotAI commented Mar 30, 2026

Copy link
Copy Markdown
Contributor

All existing Process tests rely on RemoteExecutor, which is unsupported on MacCatalyst. This adds mobile-platform Process tests using simple Unix tools (ls, sleep) available in both the Android and MacCatalyst environments, and fixes MacCatalyst-specific bugs in the Process implementation.

Changes

  • ProcessTests.Mobile.cs (new, replaces separate ProcessTests.Android.cs and ProcessTests.MacCatalyst.cs): Six mobile Process tests shared between Android and MacCatalyst via [PlatformSpecific(TestPlatforms.Android | TestPlatforms.MacCatalyst)], covering:

    • Basic process start with inherited I/O
    • Redirected stdout/stderr
    • Process.Kill() termination
    • Non-zero exit code on command failure
    • ProcessName availability for a running process
    • StandardInputHandle/StandardOutputHandle/StandardErrorHandle via anonymous pipes
  • SafeProcessHandle.Unix.cs:

    • Extracted a new private static bool UsesTerminal(SafeFileHandle?, SafeFileHandle?, SafeFileHandle?) helper method (with explanatory comments preserved) called from both StartCore and StartWithShellExecute, eliminating the previous inconsistency between the two call sites. UsesTerminal now delegates to ProcessUtils.IsTerminal (see below), removing the inline Interop.Sys.IsATty calls and the PlatformSupportsConsole guard from this site.
    • Updated comment to mention both Android and MacCatalyst as platforms that don't support Console.Open* methods
  • ProcessUtils.ConfigureTerminalForChildProcesses.iOS.cs: Added internal static bool IsTerminal(SafeFileHandle? _) => false; — on iOS/tvOS/MacCatalyst there is no terminal, so this always returns false without any P/Invoke reference to Interop.Sys.IsATty.

  • ProcessUtils.ConfigureTerminalForChildProcesses.Unix.cs: Added internal static bool IsTerminal(SafeFileHandle? standardHandle) => standardHandle is not null && Interop.Sys.IsATty(standardHandle); — on other Unix platforms, delegates to the native IsATty check.

  • ProcessUtils.cs:

    • Added PlatformSupportsConsole (!(IsAndroid() || IsMacCatalyst())) to gate Console.OpenStandard*Handle() calls
    • Added PlatformSupportsProcessStartAndKill (replaces the previous double-negative PlatformDoesNotSupportProcessStartAndKill) with a positive sense consistent with PlatformSupportsConsole; all call sites updated to if (!ProcessUtils.PlatformSupportsProcessStartAndKill)
    • Moved the platform support check to the shared file (from ProcessUtils.Unix.cs) and added early checks in Process.Start() and SafeProcessHandle.Start()
  • ProcessTests.cs: Added TestPlatforms.MacCatalyst to the [SkipOnPlatform] attributes on ProcessStart_UseShellExecute_Executes and ProcessStart_UseShellExecute_WorkingDirectory, alongside the existing iOS and tvOS skips, since MacCatalyst shares the same shell script execution restrictions

  • ProcessHandlesTests.cs: Extended [SkipOnPlatform] to include iOS, tvOS, and MacCatalyst (in addition to Android); updated skip reason to "sh is not available in the mobile platform sandbox" for greater precision

  • System.Diagnostics.Process.Tests.csproj:

    • Added $(NetCoreAppCurrent)-maccatalyst to TargetFrameworks
    • Included maccatalyst in the Unix compile condition (picks up ProcessTests.Unix.cs, ProcessThreadTests.Unix.cs, and related interop)
    • Consolidated the android and maccatalyst ItemGroup entries into a single group for ProcessTests.Mobile.cs

CopilotAI linked an issue Mar 30, 2026 that may be closed by this pull request
@github-actionsgithub-actionsBot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Mar 30, 2026
- Add $(NetCoreAppCurrent)-maccatalyst to TargetFrameworks
- Include Unix interop files for maccatalyst target
- Create ProcessTests.MacCatalyst.cs with tests for Process.Start,
I/O redirection, process killing, exit codes, and process names
modeled on ProcessTests.Android.cs
Fixes: tests cannot run on MacCatalyst because RemoteExecutor is not supported
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/6cbdf5a5-19f7-46b1-acab-3a4a9f5a6dde
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
CopilotAI changed the title [WIP] Add Process tests for MacCatalyst supportAdd Process tests for MacCatalystMar 30, 2026
CopilotAI requested a review from adamsitnikMarch 30, 2026 11:15
@github-actionsgithub-actionsBot added area-System.Diagnostics.Process and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Mar 30, 2026
@adamsitnik

Copy link
Copy Markdown
Member

/azp list

@azure-pipelines

This comment was marked as resolved.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/area-system-diagnostics-process
See info in area-owners.md if you want to be subscribed.

@adamsitnik

Copy link
Copy Markdown
Member

/azp run runtime-maccatalyst

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@adamsitnik

adamsitnik commented Mar 30, 2026

Copy link
Copy Markdown
Member

@rolfbjarne@akoeplinger@vitek-karas I am going to need some help, as this failure does not tell me anything:

[08:54:54]info:Preparingrun for maccatalyst
[08:54:54]info:Gettingapp bundle information from '/tmp/helix/working/B5070A06/w/A65C0914/e/System.Diagnostics.Process.Tests.app'..
[08:54:54] dbug: Property UIRequiredDeviceCapabilities not present in Info.plist, assuming 32-bitis not supported.Error: Failed to get bundle information: Print: Entry,"UIRequiredDeviceCapabilities",Does Not Exist[08:54:54]dbug:***Executing 'System.Diagnostics.Process.Tests' on MacCatalyst ***

@jkotas

Copy link
Copy Markdown
Member

All existing Process tests rely on RemoteExecutor, which is unsupported on MacCatalyst.

Why is RemoteExecutor not unsupported on MacCatalyst?

If we are able to start child processes on MacCatalyst, RemoteExecutor should work just fine.

@adamsitnik

Copy link
Copy Markdown
Member

Why is RemoteExecutor not unsupported on MacCatalyst?

If we are able to start child processes on MacCatalyst, RemoteExecutor should work just fine.

I don't know, but I agree it would be the best long-term solution.

@rolfbjarne

Copy link
Copy Markdown
Member
dbug:PropertyUIRequiredDeviceCapabilities not present in Info.plist,assuming32-bitis not supported.Error: Failed to get bundle information: Print: Entry,"UIRequiredDeviceCapabilities",DoesNot Exist

This comes from here, and just looks diagnostic.

The problem is that there were test failures:

[08:55:13] info: Application finished the test run successfully with some failed tests
[08:55:13] info: Tests run: 299 Passed: 130 Inconclusive: 0 Failed: 28 Ignored: 141

which can be seen in the Tests tab here: https://dev.azure.com/dnceng-public/public/_build/results?buildId=1357894&view=ms.vss-test-web.build-test-results-tab&runId=37954004&resultId=157642&paneView=debug

@adamsitnikadamsitnik added the NO-REVIEW Experimental/testing PR, do NOT review it label Mar 31, 2026
@adamsitnik

Copy link
Copy Markdown
Member

/azp run runtime-maccatalyst

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Co-authored-by: Adam Sitnik <adam.sitnik@gmail.com>
@adamsitnik

Copy link
Copy Markdown
Member

/azp run runtime-maccatalyst

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

…rocessStartAndKill
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/634abc6a-3308-4bf9-8cbc-0ee291fe1533
Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
CopilotAI requested a review from jkotasApril 2, 2026 14:52
@adamsitnik

Copy link
Copy Markdown
Member

/azp run runtime-maccatalyst

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@adamsitnik
adamsitnik enabled auto-merge (squash) April 2, 2026 15:07
@github-actions

Copy link
Copy Markdown
Contributor

🤖 Copilot Code Review — PR #126306

Note

This review was generated by Copilot.

Holistic Assessment

Motivation: This PR adds MacCatalyst platform support to the System.Diagnostics.Process test suite and refactors platform-specific checks for clarity. The motivation is well-justified — MacCatalyst can run processes (unlike iOS/tvOS) but lacks Console support (like Android), and the existing code/tests didn't account for this.

Approach: The approach is clean and well-structured. It centralizes platform-specific logic into named properties (PlatformSupportsProcessStartAndKill, PlatformSupportsConsole), extracts IsTerminal into platform files (Unix: IsATty, iOS: false), deduplicates the UsesTerminal check, and consolidates Android+MacCatalyst tests into a shared ProcessTests.Mobile.cs.

Summary: ✅ LGTM. The code changes are correct and improve the codebase. The renamed property uses positive naming conventions, all callers are properly updated with no orphaned references, the PlatformNotSupportedException guard covers all public entry points, and no new public API surface is introduced. Minor observations noted below.


Detailed Findings

✅ Platform check refactoring is correct and complete

  • PlatformDoesNotSupportProcessStartAndKillPlatformSupportsProcessStartAndKill (positive naming ✓)
  • Moved from Unix-only ProcessUtils.Unix.cs to shared ProcessUtils.cs (now usable from non-Unix-specific code ✓)
  • No orphaned references to the old name remain anywhere in the repo ✓
  • All 4 callers (Process.Start, Process.Kill, SafeProcessHandle.Start, SafeProcessHandle.SignalCore) are updated ✓

✅ Console guard centralization is correct

PlatformSupportsConsole => !(OperatingSystem.IsAndroid() || OperatingSystem.IsMacCatalyst()) correctly replaces the previous inline !OperatingSystem.IsAndroid() checks. All 6 Console.OpenStandard*Handle() call sites (3 in Process.cs, 3 in SafeProcessHandle.cs) are properly guarded with this property.

✅ PlatformNotSupportedException guard coverage is complete

The guard was removed from the internal SafeProcessHandle.StartCore and placed in both public callers (Process.Start at line 1250, SafeProcessHandle.Start at line 101). Since StartCore is internal and only reachable through these two public entry points (verified: Process.StartCoreSafeProcessHandle.StartCore, and SafeProcessHandle.StartStartCore), the coverage is equivalent.

✅ IsTerminal extraction is well-designed

The IsTerminal helper is correctly split across platform files:

  • Unix (ConfigureTerminalForChildProcesses.Unix.cs): standardHandle is not null && Interop.Sys.IsATty(standardHandle) — performs the actual TTY check
  • iOS (ConfigureTerminalForChildProcesses.iOS.cs): => false — terminals are never available on iOS-like platforms (includes MacCatalyst via IsiOSLike csproj condition)

The iOS version uses discard parameter _ correctly, matching the pattern of AreChildrenUsingTerminal => false and the no-op ConfigureTerminalForChildProcesses in the same file.

✅ Test consolidation is appropriate

Renaming ProcessTests.Android.csProcessTests.Mobile.cs and applying [PlatformSpecific(TestPlatforms.Android | TestPlatforms.MacCatalyst)] at the class level is clean. The individual [PlatformSpecific(TestPlatforms.Android)] attributes are correctly removed since the class-level attribute covers them.

✅ csproj configuration is correct

  • $(NetCoreAppCurrent)-maccatalyst added to TargetFrameworks ✓
  • maccatalyst added to the Unix/interop ItemGroup condition ✓
  • Mobile test file included for both android and maccatalyst

💡 ProcessTests.Unix.cs UseShellExecute tests — low-risk gap

Two UseShellExecute tests in ProcessTests.Unix.cs (lines 114, 134: ProcessStart_UseShellExecute_OnUnix_OpenMissingFile_DoesNotThrow and ProcessStart_UseShellExecute_OnUnix_SuccessWhenProgramInstalled) skip iOS/tvOS/Android/Browser but not MacCatalyst. Now that maccatalyst is a test target, these tests will be compiled for MacCatalyst. However, both are [OuterLoop] tests so they won't affect regular CI, and UseShellExecute via the open command likely works on MacCatalyst (it's macOS-based). This is low-risk and could be addressed in a follow-up if needed.

💡 ProcessHandlesTests skip message

The previous version had two [SkipOnPlatform] attributes with distinct reasons ("sh is not available on Android" vs "Not supported on Apple mobile platforms"). These were consolidated into a single attribute with a combined message ("sh is not available in the mobile platform sandbox"). This is a minor readability tradeoff — the combined message is slightly less precise but avoids attribute duplication. Fine as-is.

Generated by Code Review for issue #126306 ·

@adamsitnik
adamsitnik merged commit 1f9be20 into mainApr 2, 2026
94 checks passed
@adamsitnik
adamsitnik deleted the copilot/implement-process-tests-maccatalyst branch April 2, 2026 17:56
radekdoulik pushed a commit to radekdoulik/runtime that referenced this pull request Apr 9, 2026
…#126306)
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: Adam Sitnik <adam.sitnik@gmail.com>
Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@github-actionsgithub-actionsBot locked and limited conversation to collaborators May 3, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement Process tests for MacCatalyst

5 participants

@adamsitnik@jkotas@rolfbjarne
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Add Process tests for MacCatalyst and fix MacCatalyst support - #126306

Merged
adamsitnik merged 17 commits into
mainfrom
copilot/implement-process-tests-maccatalyst
Apr 2, 2026
Merged

Add Process tests for MacCatalyst and fix MacCatalyst support#126306
adamsitnik merged 17 commits into
mainfrom
copilot/implement-process-tests-maccatalyst

Conversation

CopilotAI commented Mar 30, 2026

Copy link
Copy Markdown
Contributor

All existing Process tests rely on RemoteExecutor, which is unsupported on MacCatalyst. This adds mobile-platform Process tests using simple Unix tools (ls, sleep) available in both the Android and MacCatalyst environments, and fixes MacCatalyst-specific bugs in the Process implementation.

Changes

  • ProcessTests.Mobile.cs (new, replaces separate ProcessTests.Android.cs and ProcessTests.MacCatalyst.cs): Six mobile Process tests shared between Android and MacCatalyst via [PlatformSpecific(TestPlatforms.Android | TestPlatforms.MacCatalyst)], covering:

    • Basic process start with inherited I/O
    • Redirected stdout/stderr
    • Process.Kill() termination
    • Non-zero exit code on command failure
    • ProcessName availability for a running process
    • StandardInputHandle/StandardOutputHandle/StandardErrorHandle via anonymous pipes
  • SafeProcessHandle.Unix.cs:

    • Extracted a new private static bool UsesTerminal(SafeFileHandle?, SafeFileHandle?, SafeFileHandle?) helper method (with explanatory comments preserved) called from both StartCore and StartWithShellExecute, eliminating the previous inconsistency between the two call sites. UsesTerminal now delegates to ProcessUtils.IsTerminal (see below), removing the inline Interop.Sys.IsATty calls and the PlatformSupportsConsole guard from this site.
    • Updated comment to mention both Android and MacCatalyst as platforms that don't support Console.Open* methods
  • ProcessUtils.ConfigureTerminalForChildProcesses.iOS.cs: Added internal static bool IsTerminal(SafeFileHandle? _) => false; — on iOS/tvOS/MacCatalyst there is no terminal, so this always returns false without any P/Invoke reference to Interop.Sys.IsATty.

  • ProcessUtils.ConfigureTerminalForChildProcesses.Unix.cs: Added internal static bool IsTerminal(SafeFileHandle? standardHandle) => standardHandle is not null && Interop.Sys.IsATty(standardHandle); — on other Unix platforms, delegates to the native IsATty check.

  • ProcessUtils.cs:

    • Added PlatformSupportsConsole (!(IsAndroid() || IsMacCatalyst())) to gate Console.OpenStandard*Handle() calls
    • Added PlatformSupportsProcessStartAndKill (replaces the previous double-negative PlatformDoesNotSupportProcessStartAndKill) with a positive sense consistent with PlatformSupportsConsole; all call sites updated to if (!ProcessUtils.PlatformSupportsProcessStartAndKill)
    • Moved the platform support check to the shared file (from ProcessUtils.Unix.cs) and added early checks in Process.Start() and SafeProcessHandle.Start()
  • ProcessTests.cs: Added TestPlatforms.MacCatalyst to the [SkipOnPlatform] attributes on ProcessStart_UseShellExecute_Executes and ProcessStart_UseShellExecute_WorkingDirectory, alongside the existing iOS and tvOS skips, since MacCatalyst shares the same shell script execution restrictions

  • ProcessHandlesTests.cs: Extended [SkipOnPlatform] to include iOS, tvOS, and MacCatalyst (in addition to Android); updated skip reason to "sh is not available in the mobile platform sandbox" for greater precision

  • System.Diagnostics.Process.Tests.csproj:

    • Added $(NetCoreAppCurrent)-maccatalyst to TargetFrameworks
    • Included maccatalyst in the Unix compile condition (picks up ProcessTests.Unix.cs, ProcessThreadTests.Unix.cs, and related interop)
    • Consolidated the android and maccatalyst ItemGroup entries into a single group for ProcessTests.Mobile.cs

CopilotAI linked an issue Mar 30, 2026 that may be closed by this pull request
@github-actionsgithub-actionsBot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Mar 30, 2026
- Add $(NetCoreAppCurrent)-maccatalyst to TargetFrameworks
- Include Unix interop files for maccatalyst target
- Create ProcessTests.MacCatalyst.cs with tests for Process.Start,
I/O redirection, process killing, exit codes, and process names
modeled on ProcessTests.Android.cs
Fixes: tests cannot run on MacCatalyst because RemoteExecutor is not supported
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/6cbdf5a5-19f7-46b1-acab-3a4a9f5a6dde
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
CopilotAI changed the title [WIP] Add Process tests for MacCatalyst supportAdd Process tests for MacCatalystMar 30, 2026
CopilotAI requested a review from adamsitnikMarch 30, 2026 11:15
@github-actionsgithub-actionsBot added area-System.Diagnostics.Process and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Mar 30, 2026
@adamsitnik

Copy link
Copy Markdown
Member

/azp list

@azure-pipelines

This comment was marked as resolved.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/area-system-diagnostics-process
See info in area-owners.md if you want to be subscribed.

@adamsitnik

Copy link
Copy Markdown
Member

/azp run runtime-maccatalyst

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@adamsitnik

adamsitnik commented Mar 30, 2026

Copy link
Copy Markdown
Member

@rolfbjarne@akoeplinger@vitek-karas I am going to need some help, as this failure does not tell me anything:

[08:54:54]info:Preparingrun for maccatalyst
[08:54:54]info:Gettingapp bundle information from '/tmp/helix/working/B5070A06/w/A65C0914/e/System.Diagnostics.Process.Tests.app'..
[08:54:54] dbug: Property UIRequiredDeviceCapabilities not present in Info.plist, assuming 32-bitis not supported.Error: Failed to get bundle information: Print: Entry,"UIRequiredDeviceCapabilities",Does Not Exist[08:54:54]dbug:***Executing 'System.Diagnostics.Process.Tests' on MacCatalyst ***

@jkotas

Copy link
Copy Markdown
Member

All existing Process tests rely on RemoteExecutor, which is unsupported on MacCatalyst.

Why is RemoteExecutor not unsupported on MacCatalyst?

If we are able to start child processes on MacCatalyst, RemoteExecutor should work just fine.

@adamsitnik

Copy link
Copy Markdown
Member

Why is RemoteExecutor not unsupported on MacCatalyst?

If we are able to start child processes on MacCatalyst, RemoteExecutor should work just fine.

I don't know, but I agree it would be the best long-term solution.

@rolfbjarne

Copy link
Copy Markdown
Member
dbug:PropertyUIRequiredDeviceCapabilities not present in Info.plist,assuming32-bitis not supported.Error: Failed to get bundle information: Print: Entry,"UIRequiredDeviceCapabilities",DoesNot Exist

This comes from here, and just looks diagnostic.

The problem is that there were test failures:

[08:55:13] info: Application finished the test run successfully with some failed tests
[08:55:13] info: Tests run: 299 Passed: 130 Inconclusive: 0 Failed: 28 Ignored: 141

which can be seen in the Tests tab here: https://dev.azure.com/dnceng-public/public/_build/results?buildId=1357894&view=ms.vss-test-web.build-test-results-tab&runId=37954004&resultId=157642&paneView=debug

@adamsitnikadamsitnik added the NO-REVIEW Experimental/testing PR, do NOT review it label Mar 31, 2026
@adamsitnik

Copy link
Copy Markdown
Member

/azp run runtime-maccatalyst

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Co-authored-by: Adam Sitnik <adam.sitnik@gmail.com>
@adamsitnik

Copy link
Copy Markdown
Member

/azp run runtime-maccatalyst

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

…rocessStartAndKill
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/634abc6a-3308-4bf9-8cbc-0ee291fe1533
Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
CopilotAI requested a review from jkotasApril 2, 2026 14:52
@adamsitnik

Copy link
Copy Markdown
Member

/azp run runtime-maccatalyst

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@adamsitnik
adamsitnik enabled auto-merge (squash) April 2, 2026 15:07
@github-actions

Copy link
Copy Markdown
Contributor

🤖 Copilot Code Review — PR #126306

Note

This review was generated by Copilot.

Holistic Assessment

Motivation: This PR adds MacCatalyst platform support to the System.Diagnostics.Process test suite and refactors platform-specific checks for clarity. The motivation is well-justified — MacCatalyst can run processes (unlike iOS/tvOS) but lacks Console support (like Android), and the existing code/tests didn't account for this.

Approach: The approach is clean and well-structured. It centralizes platform-specific logic into named properties (PlatformSupportsProcessStartAndKill, PlatformSupportsConsole), extracts IsTerminal into platform files (Unix: IsATty, iOS: false), deduplicates the UsesTerminal check, and consolidates Android+MacCatalyst tests into a shared ProcessTests.Mobile.cs.

Summary: ✅ LGTM. The code changes are correct and improve the codebase. The renamed property uses positive naming conventions, all callers are properly updated with no orphaned references, the PlatformNotSupportedException guard covers all public entry points, and no new public API surface is introduced. Minor observations noted below.


Detailed Findings

✅ Platform check refactoring is correct and complete

  • PlatformDoesNotSupportProcessStartAndKillPlatformSupportsProcessStartAndKill (positive naming ✓)
  • Moved from Unix-only ProcessUtils.Unix.cs to shared ProcessUtils.cs (now usable from non-Unix-specific code ✓)
  • No orphaned references to the old name remain anywhere in the repo ✓
  • All 4 callers (Process.Start, Process.Kill, SafeProcessHandle.Start, SafeProcessHandle.SignalCore) are updated ✓

✅ Console guard centralization is correct

PlatformSupportsConsole => !(OperatingSystem.IsAndroid() || OperatingSystem.IsMacCatalyst()) correctly replaces the previous inline !OperatingSystem.IsAndroid() checks. All 6 Console.OpenStandard*Handle() call sites (3 in Process.cs, 3 in SafeProcessHandle.cs) are properly guarded with this property.

✅ PlatformNotSupportedException guard coverage is complete

The guard was removed from the internal SafeProcessHandle.StartCore and placed in both public callers (Process.Start at line 1250, SafeProcessHandle.Start at line 101). Since StartCore is internal and only reachable through these two public entry points (verified: Process.StartCoreSafeProcessHandle.StartCore, and SafeProcessHandle.StartStartCore), the coverage is equivalent.

✅ IsTerminal extraction is well-designed

The IsTerminal helper is correctly split across platform files:

  • Unix (ConfigureTerminalForChildProcesses.Unix.cs): standardHandle is not null && Interop.Sys.IsATty(standardHandle) — performs the actual TTY check
  • iOS (ConfigureTerminalForChildProcesses.iOS.cs): => false — terminals are never available on iOS-like platforms (includes MacCatalyst via IsiOSLike csproj condition)

The iOS version uses discard parameter _ correctly, matching the pattern of AreChildrenUsingTerminal => false and the no-op ConfigureTerminalForChildProcesses in the same file.

✅ Test consolidation is appropriate

Renaming ProcessTests.Android.csProcessTests.Mobile.cs and applying [PlatformSpecific(TestPlatforms.Android | TestPlatforms.MacCatalyst)] at the class level is clean. The individual [PlatformSpecific(TestPlatforms.Android)] attributes are correctly removed since the class-level attribute covers them.

✅ csproj configuration is correct

  • $(NetCoreAppCurrent)-maccatalyst added to TargetFrameworks ✓
  • maccatalyst added to the Unix/interop ItemGroup condition ✓
  • Mobile test file included for both android and maccatalyst

💡 ProcessTests.Unix.cs UseShellExecute tests — low-risk gap

Two UseShellExecute tests in ProcessTests.Unix.cs (lines 114, 134: ProcessStart_UseShellExecute_OnUnix_OpenMissingFile_DoesNotThrow and ProcessStart_UseShellExecute_OnUnix_SuccessWhenProgramInstalled) skip iOS/tvOS/Android/Browser but not MacCatalyst. Now that maccatalyst is a test target, these tests will be compiled for MacCatalyst. However, both are [OuterLoop] tests so they won't affect regular CI, and UseShellExecute via the open command likely works on MacCatalyst (it's macOS-based). This is low-risk and could be addressed in a follow-up if needed.

💡 ProcessHandlesTests skip message

The previous version had two [SkipOnPlatform] attributes with distinct reasons ("sh is not available on Android" vs "Not supported on Apple mobile platforms"). These were consolidated into a single attribute with a combined message ("sh is not available in the mobile platform sandbox"). This is a minor readability tradeoff — the combined message is slightly less precise but avoids attribute duplication. Fine as-is.

Generated by Code Review for issue #126306 ·

@adamsitnik
adamsitnik merged commit 1f9be20 into mainApr 2, 2026
94 checks passed
@adamsitnik
adamsitnik deleted the copilot/implement-process-tests-maccatalyst branch April 2, 2026 17:56
radekdoulik pushed a commit to radekdoulik/runtime that referenced this pull request Apr 9, 2026
…#126306)
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: Adam Sitnik <adam.sitnik@gmail.com>
Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@github-actionsgithub-actionsBot locked and limited conversation to collaborators May 3, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement Process tests for MacCatalyst

5 participants

@adamsitnik@jkotas@rolfbjarne
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Add Process tests for MacCatalyst and fix MacCatalyst support - #126306

Merged
adamsitnik merged 17 commits into
mainfrom
copilot/implement-process-tests-maccatalyst
Apr 2, 2026
Merged

Add Process tests for MacCatalyst and fix MacCatalyst support#126306
adamsitnik merged 17 commits into
mainfrom
copilot/implement-process-tests-maccatalyst

Conversation

CopilotAI commented Mar 30, 2026

Copy link
Copy Markdown
Contributor

All existing Process tests rely on RemoteExecutor, which is unsupported on MacCatalyst. This adds mobile-platform Process tests using simple Unix tools (ls, sleep) available in both the Android and MacCatalyst environments, and fixes MacCatalyst-specific bugs in the Process implementation.

Changes

  • ProcessTests.Mobile.cs (new, replaces separate ProcessTests.Android.cs and ProcessTests.MacCatalyst.cs): Six mobile Process tests shared between Android and MacCatalyst via [PlatformSpecific(TestPlatforms.Android | TestPlatforms.MacCatalyst)], covering:

    • Basic process start with inherited I/O
    • Redirected stdout/stderr
    • Process.Kill() termination
    • Non-zero exit code on command failure
    • ProcessName availability for a running process
    • StandardInputHandle/StandardOutputHandle/StandardErrorHandle via anonymous pipes
  • SafeProcessHandle.Unix.cs:

    • Extracted a new private static bool UsesTerminal(SafeFileHandle?, SafeFileHandle?, SafeFileHandle?) helper method (with explanatory comments preserved) called from both StartCore and StartWithShellExecute, eliminating the previous inconsistency between the two call sites. UsesTerminal now delegates to ProcessUtils.IsTerminal (see below), removing the inline Interop.Sys.IsATty calls and the PlatformSupportsConsole guard from this site.
    • Updated comment to mention both Android and MacCatalyst as platforms that don't support Console.Open* methods
  • ProcessUtils.ConfigureTerminalForChildProcesses.iOS.cs: Added internal static bool IsTerminal(SafeFileHandle? _) => false; — on iOS/tvOS/MacCatalyst there is no terminal, so this always returns false without any P/Invoke reference to Interop.Sys.IsATty.

  • ProcessUtils.ConfigureTerminalForChildProcesses.Unix.cs: Added internal static bool IsTerminal(SafeFileHandle? standardHandle) => standardHandle is not null && Interop.Sys.IsATty(standardHandle); — on other Unix platforms, delegates to the native IsATty check.

  • ProcessUtils.cs:

    • Added PlatformSupportsConsole (!(IsAndroid() || IsMacCatalyst())) to gate Console.OpenStandard*Handle() calls
    • Added PlatformSupportsProcessStartAndKill (replaces the previous double-negative PlatformDoesNotSupportProcessStartAndKill) with a positive sense consistent with PlatformSupportsConsole; all call sites updated to if (!ProcessUtils.PlatformSupportsProcessStartAndKill)
    • Moved the platform support check to the shared file (from ProcessUtils.Unix.cs) and added early checks in Process.Start() and SafeProcessHandle.Start()
  • ProcessTests.cs: Added TestPlatforms.MacCatalyst to the [SkipOnPlatform] attributes on ProcessStart_UseShellExecute_Executes and ProcessStart_UseShellExecute_WorkingDirectory, alongside the existing iOS and tvOS skips, since MacCatalyst shares the same shell script execution restrictions

  • ProcessHandlesTests.cs: Extended [SkipOnPlatform] to include iOS, tvOS, and MacCatalyst (in addition to Android); updated skip reason to "sh is not available in the mobile platform sandbox" for greater precision

  • System.Diagnostics.Process.Tests.csproj:

    • Added $(NetCoreAppCurrent)-maccatalyst to TargetFrameworks
    • Included maccatalyst in the Unix compile condition (picks up ProcessTests.Unix.cs, ProcessThreadTests.Unix.cs, and related interop)
    • Consolidated the android and maccatalyst ItemGroup entries into a single group for ProcessTests.Mobile.cs

CopilotAI linked an issue Mar 30, 2026 that may be closed by this pull request
@github-actionsgithub-actionsBot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Mar 30, 2026
- Add $(NetCoreAppCurrent)-maccatalyst to TargetFrameworks
- Include Unix interop files for maccatalyst target
- Create ProcessTests.MacCatalyst.cs with tests for Process.Start,
I/O redirection, process killing, exit codes, and process names
modeled on ProcessTests.Android.cs
Fixes: tests cannot run on MacCatalyst because RemoteExecutor is not supported
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/6cbdf5a5-19f7-46b1-acab-3a4a9f5a6dde
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
CopilotAI changed the title [WIP] Add Process tests for MacCatalyst supportAdd Process tests for MacCatalystMar 30, 2026
CopilotAI requested a review from adamsitnikMarch 30, 2026 11:15
@github-actionsgithub-actionsBot added area-System.Diagnostics.Process and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Mar 30, 2026
@adamsitnik

Copy link
Copy Markdown
Member

/azp list

@azure-pipelines

This comment was marked as resolved.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/area-system-diagnostics-process
See info in area-owners.md if you want to be subscribed.

@adamsitnik

Copy link
Copy Markdown
Member

/azp run runtime-maccatalyst

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@adamsitnik

adamsitnik commented Mar 30, 2026

Copy link
Copy Markdown
Member

@rolfbjarne@akoeplinger@vitek-karas I am going to need some help, as this failure does not tell me anything:

[08:54:54]info:Preparingrun for maccatalyst
[08:54:54]info:Gettingapp bundle information from '/tmp/helix/working/B5070A06/w/A65C0914/e/System.Diagnostics.Process.Tests.app'..
[08:54:54] dbug: Property UIRequiredDeviceCapabilities not present in Info.plist, assuming 32-bitis not supported.Error: Failed to get bundle information: Print: Entry,"UIRequiredDeviceCapabilities",Does Not Exist[08:54:54]dbug:***Executing 'System.Diagnostics.Process.Tests' on MacCatalyst ***

@jkotas

Copy link
Copy Markdown
Member

All existing Process tests rely on RemoteExecutor, which is unsupported on MacCatalyst.

Why is RemoteExecutor not unsupported on MacCatalyst?

If we are able to start child processes on MacCatalyst, RemoteExecutor should work just fine.

@adamsitnik

Copy link
Copy Markdown
Member

Why is RemoteExecutor not unsupported on MacCatalyst?

If we are able to start child processes on MacCatalyst, RemoteExecutor should work just fine.

I don't know, but I agree it would be the best long-term solution.

@rolfbjarne

Copy link
Copy Markdown
Member
dbug:PropertyUIRequiredDeviceCapabilities not present in Info.plist,assuming32-bitis not supported.Error: Failed to get bundle information: Print: Entry,"UIRequiredDeviceCapabilities",DoesNot Exist

This comes from here, and just looks diagnostic.

The problem is that there were test failures:

[08:55:13] info: Application finished the test run successfully with some failed tests
[08:55:13] info: Tests run: 299 Passed: 130 Inconclusive: 0 Failed: 28 Ignored: 141

which can be seen in the Tests tab here: https://dev.azure.com/dnceng-public/public/_build/results?buildId=1357894&view=ms.vss-test-web.build-test-results-tab&runId=37954004&resultId=157642&paneView=debug

@adamsitnikadamsitnik added the NO-REVIEW Experimental/testing PR, do NOT review it label Mar 31, 2026
@adamsitnik

Copy link
Copy Markdown
Member

/azp run runtime-maccatalyst

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Co-authored-by: Adam Sitnik <adam.sitnik@gmail.com>
@adamsitnik

Copy link
Copy Markdown
Member

/azp run runtime-maccatalyst

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

…rocessStartAndKill
Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/634abc6a-3308-4bf9-8cbc-0ee291fe1533
Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
CopilotAI requested a review from jkotasApril 2, 2026 14:52
@adamsitnik

Copy link
Copy Markdown
Member

/azp run runtime-maccatalyst

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@adamsitnik
adamsitnik enabled auto-merge (squash) April 2, 2026 15:07
@github-actions

Copy link
Copy Markdown
Contributor

🤖 Copilot Code Review — PR #126306

Note

This review was generated by Copilot.

Holistic Assessment

Motivation: This PR adds MacCatalyst platform support to the System.Diagnostics.Process test suite and refactors platform-specific checks for clarity. The motivation is well-justified — MacCatalyst can run processes (unlike iOS/tvOS) but lacks Console support (like Android), and the existing code/tests didn't account for this.

Approach: The approach is clean and well-structured. It centralizes platform-specific logic into named properties (PlatformSupportsProcessStartAndKill, PlatformSupportsConsole), extracts IsTerminal into platform files (Unix: IsATty, iOS: false), deduplicates the UsesTerminal check, and consolidates Android+MacCatalyst tests into a shared ProcessTests.Mobile.cs.

Summary: ✅ LGTM. The code changes are correct and improve the codebase. The renamed property uses positive naming conventions, all callers are properly updated with no orphaned references, the PlatformNotSupportedException guard covers all public entry points, and no new public API surface is introduced. Minor observations noted below.


Detailed Findings

✅ Platform check refactoring is correct and complete

  • PlatformDoesNotSupportProcessStartAndKillPlatformSupportsProcessStartAndKill (positive naming ✓)
  • Moved from Unix-only ProcessUtils.Unix.cs to shared ProcessUtils.cs (now usable from non-Unix-specific code ✓)
  • No orphaned references to the old name remain anywhere in the repo ✓
  • All 4 callers (Process.Start, Process.Kill, SafeProcessHandle.Start, SafeProcessHandle.SignalCore) are updated ✓

✅ Console guard centralization is correct

PlatformSupportsConsole => !(OperatingSystem.IsAndroid() || OperatingSystem.IsMacCatalyst()) correctly replaces the previous inline !OperatingSystem.IsAndroid() checks. All 6 Console.OpenStandard*Handle() call sites (3 in Process.cs, 3 in SafeProcessHandle.cs) are properly guarded with this property.

✅ PlatformNotSupportedException guard coverage is complete

The guard was removed from the internal SafeProcessHandle.StartCore and placed in both public callers (Process.Start at line 1250, SafeProcessHandle.Start at line 101). Since StartCore is internal and only reachable through these two public entry points (verified: Process.StartCoreSafeProcessHandle.StartCore, and SafeProcessHandle.StartStartCore), the coverage is equivalent.

✅ IsTerminal extraction is well-designed

The IsTerminal helper is correctly split across platform files:

  • Unix (ConfigureTerminalForChildProcesses.Unix.cs): standardHandle is not null && Interop.Sys.IsATty(standardHandle) — performs the actual TTY check
  • iOS (ConfigureTerminalForChildProcesses.iOS.cs): => false — terminals are never available on iOS-like platforms (includes MacCatalyst via IsiOSLike csproj condition)

The iOS version uses discard parameter _ correctly, matching the pattern of AreChildrenUsingTerminal => false and the no-op ConfigureTerminalForChildProcesses in the same file.

✅ Test consolidation is appropriate

Renaming ProcessTests.Android.csProcessTests.Mobile.cs and applying [PlatformSpecific(TestPlatforms.Android | TestPlatforms.MacCatalyst)] at the class level is clean. The individual [PlatformSpecific(TestPlatforms.Android)] attributes are correctly removed since the class-level attribute covers them.

✅ csproj configuration is correct

  • $(NetCoreAppCurrent)-maccatalyst added to TargetFrameworks ✓
  • maccatalyst added to the Unix/interop ItemGroup condition ✓
  • Mobile test file included for both android and maccatalyst

💡 ProcessTests.Unix.cs UseShellExecute tests — low-risk gap

Two UseShellExecute tests in ProcessTests.Unix.cs (lines 114, 134: ProcessStart_UseShellExecute_OnUnix_OpenMissingFile_DoesNotThrow and ProcessStart_UseShellExecute_OnUnix_SuccessWhenProgramInstalled) skip iOS/tvOS/Android/Browser but not MacCatalyst. Now that maccatalyst is a test target, these tests will be compiled for MacCatalyst. However, both are [OuterLoop] tests so they won't affect regular CI, and UseShellExecute via the open command likely works on MacCatalyst (it's macOS-based). This is low-risk and could be addressed in a follow-up if needed.

💡 ProcessHandlesTests skip message

The previous version had two [SkipOnPlatform] attributes with distinct reasons ("sh is not available on Android" vs "Not supported on Apple mobile platforms"). These were consolidated into a single attribute with a combined message ("sh is not available in the mobile platform sandbox"). This is a minor readability tradeoff — the combined message is slightly less precise but avoids attribute duplication. Fine as-is.

Generated by Code Review for issue #126306 ·

@adamsitnik
adamsitnik merged commit 1f9be20 into mainApr 2, 2026
94 checks passed
@adamsitnik
adamsitnik deleted the copilot/implement-process-tests-maccatalyst branch April 2, 2026 17:56
radekdoulik pushed a commit to radekdoulik/runtime that referenced this pull request Apr 9, 2026
…#126306)
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
Co-authored-by: Adam Sitnik <adam.sitnik@gmail.com>
Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@github-actionsgithub-actionsBot locked and limited conversation to collaborators May 3, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement Process tests for MacCatalyst

5 participants

@adamsitnik@jkotas@rolfbjarne