Unify tests on all supported platforms to ensure consistent behavior and add more tests. #133

iCharlesHu · 2025-08-08T20:24:44Z

This PR refactors and consolidates tests across all platforms and introduces numerous additional tests.

Now, tests are categorized into three main types:

IntegrationTests: This serves as the primary test file that evaluates the end-to-end functionality of Subprocess by executing child processes.
{Unix|Darwin|Linux|Windows}Tests: These tests are platform-specific and cater to the unique characteristics of each operating system.
AsyncIOTests: These tests are dedicated to unit testing the AsyncIO functionality specifically.

These newly established test categories have revealed a few bugs in the implementation, which are also addressed in this PR:

Moved the monitorProcessTermination function inside the withAsyncTaskCleanupHandler. This adjustment ensures that the teardown sequence can still be executed even if the body closure terminates prematurely, but the child process remains stuck.
On Windows, we’ve modified the code to use STARTUPINFOEXW to ensure that only the explicitly provided file descriptors are inheritable.
Make sure AsyncIO.shutdown is idempotent.

Resolves: #97

jakepetroules · 2025-08-08T21:16:44Z

On Windows, we’ve modified the code to use STARTUPINFOEXW to ensure that only the explicitly provided file descriptors are inheritable.

❤️

Resolves: #100

…and add more tests.

jakepetroules · 2025-08-09T03:59:48Z

Sources/Subprocess/IO/AsyncIO+Linux.swift

        guard case .success(let currentState) = self.state else {
            return
        }

+        guard self.shutdownFlag.add(1, ordering: .sequentiallyConsistent).newValue == 1 else {


Should this be >=1 instead of ==1? I think as written this will run the shutdown procedure n-1 times for n calls.

jakepetroules · 2025-08-09T04:02:42Z

Sources/Subprocess/IO/AsyncIO+Windows.swift

        guard case .success(let ioPort) = ioCompletionPort,
              case .success(let monitorThreadHandle) = monitorThread else {
            return
        }
+        // Make sure we don't shutdown the same instance twice
+        guard self.shutdownFlag.add(1, ordering: .relaxed).newValue == 1 else {


needs to be >=1?

jakepetroules · 2025-08-09T04:43:29Z

Sources/Subprocess/Platforms/Subprocess+Windows.swift

+        _ body: (UnsafeMutablePointer<STARTUPINFOEXW>) throws -> Result
+    ) rethrows -> Result {
+        var info: STARTUPINFOEXW = STARTUPINFOEXW()
+        info.StartupInfo.cb = DWORD(MemoryLayout<STARTUPINFOEXW>.size)


nit: can use MemoryLayout.size(ofValue: info) to avoid repeating the type name (seems safer)

jakepetroules · 2025-08-09T04:50:47Z

Sources/Subprocess/Teardown.swift

+        GetExitCodeProcess(self.processInformation.hProcess, &exitCode)
+        return exitCode == STILL_ACTIVE
+        #else
+        return kill(self.processIdentifier.value, 0) == 0


Any particular reason to use kill 0 instead of waitpid with WNOHANG+WNOWAIT?

jakepetroules · 2025-08-09T04:51:42Z

Sources/Subprocess/Teardown.swift

@@ -194,6 +198,19 @@ extension Execution {
            }
        }
    }
+
+    private func isStillAlive() -> Bool {


Could already have exited by the time you return, perhaps a better name is "isPotentiallyStillAlive"? The only definitive case we can know is that it did exit.

jakepetroules · 2025-08-09T06:27:25Z

Tests/SubprocessTests/IntegrationTests.swift

+            arguments: ["/c", "set"],
+            environment: .custom([
+                "Path": "C:\\Windows\\system32;C:\\Windows",
+                "ComSpec": "C:\\Windows\\System32\\cmd.exe",


Can use this to compute the system32 directory:

private func _url(for id: KNOWNFOLDERID) -> URL? { var pszPath: PWSTR? let hr: HRESULT = withUnsafePointer(to: id) { id in SHGetKnownFolderPath(id, DWORD(WinSDK.KF_FLAG_DEFAULT.rawValue), nil, &pszPath) } guard hr >= 0 else { return nil } defer { CoTaskMemFree(pszPath) } return URL(filePath: String(decodingCString: pszPath!, as: UTF16.self), directoryHint: .isDirectory) } _url(for: FOLDERID_System) // C:\\Windows\\System32

jakepetroules · 2025-08-09T06:41:44Z

Tests/SubprocessTests/IntegrationTests.swift

+                var buffer: Data!
+                while let result = try await group.next() {
+                    if let result: Data = result {
+                        buffer = result


Suggested change

buffer = result

return result

jakepetroules · 2025-08-09T06:42:03Z

Tests/SubprocessTests/IntegrationTests.swift

+                        buffer = result
+                    }
+                }
+                return buffer


Suggested change

return buffer

preconditionFailure("unreachable")

jakepetroules · 2025-08-09T06:47:14Z

Tests/SubprocessTests/IntegrationTests.swift

+                output: .string(limit: 16),
+                error: .discarded
+            )
+            Issue.record("Expected to throw")


For this you can use:

#expect(throws: Error.self) { // code... }

jakepetroules · 2025-08-09T07:46:00Z

Tests/SubprocessTests/UnixTests.swift

+                    let grandChildPid = try #require(pid_t(output))
+                    // Make sure the grand child `/usr/bin/yes` actually exited
+                    // This is unfortunately racy because the pid isn't immediately invalided
+                    // once the exit exits. Allow a few failures and delay to counter this


Suggested change

// once the exit exits. Allow a few failures and delay to counter this

// once `kill` returns. Allow a few failures and delay to counter this

?

jakepetroules · 2025-08-09T08:03:40Z

Tests/SubprocessTests/IntegrationTests.swift

+        try await withThrowingTaskGroup(of: Void.self) { group in
+            for _ in 0 ..< 8 {
+                group.addTask {
+                    // This invocation specifically requires bash semantics; sh (on FreeBSD at least) does not consistently support -s in this way


Leftover comment? nothing here is using bash

jakepetroules · 2025-08-09T08:14:50Z

Sources/Subprocess/IO/AsyncIO+Linux.swift

@@ -226,7 +231,6 @@ final class AsyncIO: Sendable {
        }
    }

-
    private func registerFileDescriptor(


Should registerFileDescriptor precondition that the fd is not already in the _registration map?

jakepetroules · 2025-08-09T08:35:18Z

Tests/SubprocessTests/IntegrationTests.swift

+        #expect(result.value == expected)
+    }
+
+    @Test func stressTestWithLittleOutput() async throws {


✘ Test stressTestWithLittleOutput() recorded an issue at IntegrationTests.swift:1460:6: Caught error: An error occurred within the AsyncIO subsystem: failed to add 27 to epoll list. Underlying error: UnderlyingError(rawValue: 17)

Could this be because _safelyClose does not call AsyncIO.shared.removeRegistration for the fd before closing it? Reading I/O fails because of low limit, fails, calls _safelyClose, fd is closed, fd # gets reused, tries to re-add, already in the epoll list, boom.

iCharlesHu requested review from cthielen, parkera, jakepetroules and itingliu August 8, 2025 20:24

iCharlesHu force-pushed the charles/testing branch from 84050f5 to 1c7f185 Compare August 8, 2025 20:30

Unify tests on all supported platforms to ensure consistent behavior …

471a3ab

…and add more tests.

iCharlesHu force-pushed the charles/testing branch from 1c7f185 to 471a3ab Compare August 9, 2025 02:23

jakepetroules reviewed Aug 9, 2025

View reviewed changes

jakepetroules linked an issue Aug 11, 2025 that may be closed by this pull request

Handle inheritance should be constrained to specific handles on Windows? #100

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Unify tests on all supported platforms to ensure consistent behavior and add more tests. #133

Unify tests on all supported platforms to ensure consistent behavior and add more tests. #133

Uh oh!

iCharlesHu commented Aug 8, 2025

Uh oh!

jakepetroules commented Aug 8, 2025 •

edited

Loading

Uh oh!

jakepetroules Aug 9, 2025

Uh oh!

jakepetroules Aug 9, 2025

Uh oh!

jakepetroules Aug 9, 2025

Uh oh!

jakepetroules Aug 9, 2025

Uh oh!

jakepetroules Aug 9, 2025

Uh oh!

jakepetroules Aug 9, 2025

Uh oh!

jakepetroules Aug 9, 2025

Uh oh!

jakepetroules Aug 9, 2025

Uh oh!

jakepetroules Aug 9, 2025

Uh oh!

jakepetroules Aug 9, 2025

Uh oh!

jakepetroules Aug 9, 2025

Uh oh!

jakepetroules Aug 9, 2025

Uh oh!

jakepetroules Aug 9, 2025

Uh oh!

Uh oh!

	// once the exit exits. Allow a few failures and delay to counter this
	// once `kill` returns. Allow a few failures and delay to counter this

Unify tests on all supported platforms to ensure consistent behavior and add more tests. #133

Are you sure you want to change the base?

Unify tests on all supported platforms to ensure consistent behavior and add more tests. #133

Uh oh!

Conversation

iCharlesHu commented Aug 8, 2025

Uh oh!

jakepetroules commented Aug 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jakepetroules commented Aug 8, 2025 •

edited

Loading