Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Conversation

iCharlesHu
Copy link
Contributor

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
Copy link
Contributor

jakepetroules commented Aug 8, 2025

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

❤️

Resolves: #100

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

Choose a reason for hiding this comment

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

Suggested change
buffer = result
return result

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is intentional. We want to wait for both tasks to finish and let result = result only check for the one that actually returns result. We also need to wait for the writer task which doesn't return anything.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, right. In that case, how about using async tasks instead of a task group? That would give you a cleaner way to wait for both with no force unwraps. Both things continue to run in parallel and guaranteed to wait for everything to complete.

async let buffer = {
    var buffer = Data()
    for try await chunk in standardOutput {
    let currentChunk = chunk.withUnsafeBytes { Data($0) }
        buffer += currentChunk
    }
    return buffer
}()

_ = try await standardInputWriter.write(Array(expected))
try await standardInputWriter.finish()

return try await buffer

buffer = result
}
}
return buffer
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return buffer
preconditionFailure("unreachable")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ditto. This is intentionally to wait for both tasks.

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


private func registerFileDescriptor(
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can, but I don't think it's necessary because epoll_ctl will fail if it's already registered so you get the same error.

#expect(result.value == expected)
}

@Test func stressTestWithLittleOutput() async throws {
Copy link
Contributor

Choose a reason for hiding this comment

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

✘ 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle inheritance should be constrained to specific handles on Windows? Unify unit tests on all platforms and add more
2 participants