-
Notifications
You must be signed in to change notification settings - Fork 47
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
Conversation
84050f5 to
1c7f185
Compare
❤️ Resolves: #100 |
1c7f185 to
471a3ab
Compare
| } | ||
|
|
||
|
|
||
| private func registerFileDescriptor( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should registerFileDescriptor precondition that the fd is not already in the _registration map?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✘ 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.
471a3ab to
5c86b88
Compare
|
Introduced |
779dae4 to
fb99f50
Compare
ddb6d23 to
584ae43
Compare
584ae43 to
80aa4c0
Compare
jakepetroules
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
This PR refactors and consolidates tests across all platforms and introduces numerous additional tests.
Now, tests are categorized into four 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 theAsyncIOfunctionality specifically.ProcessMonitoringTests: These tests are dedicated to unit testingfunc monitorProcessTerminationspecificallyThese newly established test categories have revealed a few bugs in the implementation, which are also addressed in this PR:
monitorProcessTerminationfunction inside thewithAsyncTaskCleanupHandler. This adjustment ensures that the teardown sequence can still be executed even if thebodyclosure terminates prematurely, but the child process remains stuck.STARTUPINFOEXWto ensure that only the explicitly provided file descriptors are inheritable.AsyncIO.shutdownis idempotent.Resolves: #97
Resolves: #100