-
Notifications
You must be signed in to change notification settings - Fork 120
Rename ExitTestArtifacts
and split ExitCondition
in twain.
#975
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
This PR renames `ExitTestArtifacts` to `ExitTest.Result` and splits `ExitCondition` into two types: `ExitTest.Condition` which can be passed to `#expect(exitsWith:)` and `StatusAtExit` which represents the raw, possibly platform-specific status reported by the kernel when a child process terminates. The latter type is not nested in `ExitTest` because it can be used independently of exit tests and we may want to use it in the future for things like multi-process parallelization, but if a platform supports spawning processes but not exit tests, nesting it in `ExitTest` would make it unavailable. I considered several names for `StatusAtExit`: - `ExitStatus`: too easily confusable with exit _codes_ such as `EXIT_SUCCESS`; - `ProcessStatus`: we don't say "process" in our API surface elsewhere; - `Status`: too generic - `ExitReason`: "status" is a more widely-used term of art for this concept. Foundation uses `terminationStatus` to represent the raw integer value and `Process.TerminationReason` to represent whether it's an exit code or signal. We don't use "termination" in Swift Testing's API anywhere. I settled on `StatusAtExit` because it was distinct and makes it clear that it represents the status of a process _at exit time_ (as opposed to while running, e.g. `enum ProcessStatus { case running; case suspended; case terminated }`. Updates to the exit tests proposal document will follow in a separate PR.
@swift-ci test |
@swift-ci test |
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.
Overall I like these refinements! Several specific comments
ExitTests/SpawnProcess.swift | ||
ExitTests/StatusAtExit.swift |
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.
You mentioned in the PR description this type may be used independently of exit tests, so I'm wondering if the file ought to be placed in a different directory to emphasize that.
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.
It can move, but the code is already in this directory so for now I was planning to leave it there. Do you feel strongly?
@swift-ci test |
This PR renames
ExitTestArtifacts
toExitTest.Result
and splitsExitCondition
into two types:ExitTest.Condition
which can be passed to#expect(exitsWith:)
andStatusAtExit
which represents the raw, possibly platform-specific status reported by the kernel when a child process terminates.The latter type is not nested in
ExitTest
because it can be used independently of exit tests and we may want to use it in the future for things like multi-process parallelization, but if a platform supports spawning processes but not exit tests, nesting it inExitTest
would make it unavailable.I considered several names for
StatusAtExit
:ExitStatus
: too easily confusable with exit codes such asEXIT_SUCCESS
;ProcessStatus
: we don't say "process" in our API surface elsewhere;Status
: too genericExitReason
: "status" is a more widely-used term of art for this concept.Foundation uses
terminationStatus
to represent the raw integer value andProcess.TerminationReason
to represent whether it's an exit code or signal. We don't use "termination" in Swift Testing's API anywhere.I settled on
StatusAtExit
because it was distinct and makes it clear that it represents the status of a process at exit time (as opposed to while running, e.g.enum ProcessStatus { case running; case suspended; case terminated }
.Updates to the exit tests proposal document will follow in a separate PR.
Checklist: