Record issues generated within exit tests. #697
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
grynspan
added
enhancement
grynspan
requested review from
stmontgomery,
dennisweissmann,
briancroom,
SeanROlszewski and
suzannaratcliff
as code owners
|
@swift-ci test |
6 similar comments
|
@swift-ci test |
|
@swift-ci test |
|
@swift-ci test |
|
@swift-ci test |
|
@swift-ci test |
|
@swift-ci test |
grynspan
force-pushed
the
jgrynspan/exit-test-back-channel
branch
from
4b4ec43 to
32b58ed
Compare
|
@swift-ci test |
1 similar comment
|
@swift-ci test |
grynspan
force-pushed
the
jgrynspan/exit-test-back-channel
branch
from
c783c17 to
ff61fc6
Compare
|
@swift-ci test |
|
@swift-ci test Linux |
grynspan
force-pushed
the
jgrynspan/exit-test-back-channel
branch
from
b291707 to
324f6b8
Compare
|
@swift-ci test |
grynspan
force-pushed
the
jgrynspan/exit-test-back-channel
branch
2 times, most recently
from
6845910 to
c635ea3
Compare
grynspan
added a commit
that referenced
this pull request
Sep 17, 2024
This PR moves the logic that implements JSON Lines support (i.e. the code that strips newlines from JSON generated by Foundation) out of EntryPoint.swift so that it can be used by other callers. This change was originally part of #697. I'm splitting it out into its own PR to make that one (both really) easier to read and understand.
compnerd
approved these changes
Sep 17, 2024
|
@swift-ci test |
1 similar comment
|
@swift-ci test |
|
@swift-ci test |
This PR introduces a "back channel" file handle to exit tests, allowing us to
record issues that occur within exit test bodies. For example:
```swift
await #expect(exitsWith: .failure) {
let context = try #require(possiblyMissingContext)
...
}
```
In this example, if the call to `try #require()` finds `nil`, it will record an
issue, but that issue today will be lost because there's no mechanism to forward
the issue back to the parent process hosting the exit test. This PR fixes that!
Issues are converted to JSON using the same schema we use for event handling,
then written over a pipe back to the parent process where they are decoded. This
decoding is lossy, so there will be further refinement needed here to try to
preserve more information about the recorded issues. That said, "it's got good
bones" right?
On Darwin, Linux, and FreeBSD, the pipe's write end is allowed to survive into
the child process (i.e. no `FD_CLOEXEC`). On Windows, the equivalent is to tell
`CreateProcessW()` to explicitly inherit a `HANDLE`. The identity of this file
descriptor or handle is passed to the child process via environment variable.
The child process then parses the file descriptor or handle out of the
environment and converts it back to a `FileHandle` that is then connected to an
instance of `Configuration` with an event handler set, and off we go.
Because we can now report these issues back to the parent process, I've removed
the compile-time diagnostic in the `#expect(exitsWith:)` macro implementation
that we emit when we see a nested `#expect()` or `#require()` call.
…nction so you can't break things by calling it twice in the same process
…y applies to our own entry point and third parties would need to supply their own configuration/etc. anyway
grynspan
force-pushed
the
jgrynspan/exit-test-back-channel
branch
from
a03037b to
ec89f02
Compare
|
@swift-ci test |
1 similar comment
|
@swift-ci test |
|
@swift-ci test |
|
@swift-ci test |
| message.symbol == .details ? Comment(rawValue: message.text) : nil | ||
| } | ||
| let sourceContext = SourceContext( | ||
| backtrace: nil, // `issue._backtrace` will have the wrong address space. |
There was a problem hiding this comment.
Can we file an issue to follow up here and eventually propagate and preserve some of the symbolicated address of the backtrace? I know the specific numeric addresses will not be useful, but if the backtrace was symbolicated, it would be great to propagate symbol names. But I assume that will require a bit more effort and isn't in scope here
There was a problem hiding this comment.
It's something we can do once we have a proper symbolication mechanism rather than the POC that's in the repo now.
2 tasks
grynspan
added a commit
that referenced
this pull request
Sep 26, 2024
#718) Hot on the heels of #697… If an exit test records an issue of kind `.errorCaught()`, we currently treat it the same as any other issue and translate it to a flat `.unconditional` issue in the parent process: ```swift await #expect(exitsWith: .failure) { Issue.record(NoSuchWeaselError()) // unconditional issue recorded } ``` This PR distinguishes the `.errorCaught` issue kind and records an `.errorCaught` issue in the parent process. Fidelity of errors across a process boundary is weak, but we encode a minimal representation of the error that should be good enough for most uses. If you need `withKnownIssue()`, or `#expect(throws:)`, you can call them _inside_ the exit test. ### Checklist: - [x] Code and documentation should follow the style of the [Style Guide](https://github.com/apple/swift-testing/blob/main/Documentation/StyleGuide.md). - [x] If public symbols are renamed or modified, DocC references should be updated.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR introduces a "back channel" file handle to exit tests, allowing us to record issues that occur within exit test bodies. For example:
In this example, if the call to
try #require()findsnil, it will record an issue, but that issue today will be lost because there's no mechanism to forward the issue back to the parent process hosting the exit test. This PR fixes that!Issues are converted to JSON using the same schema we use for event handling, then written over a pipe back to the parent process where they are decoded. This decoding is lossy, so there will be further refinement needed here to try to preserve more information about the recorded issues. That said, "it's got good bones" right?
On Darwin, Linux, and FreeBSD, the pipe's write end is allowed to survive into the child process (i.e. no
FD_CLOEXEC). On Windows, the equivalent is to tellCreateProcessW()to explicitly inherit aHANDLE. The identity of this file descriptor or handle is passed to the child process via environment variable. The child process then parses the file descriptor or handle out of the environment and converts it back to aFileHandlethat is then connected to an instance ofConfigurationwith an event handler set, and off we go.Because we can now report these issues back to the parent process, I've removed the compile-time diagnostic in the
#expect(exitsWith:)macro implementation that we emit when we see a nested#expect()or#require()call.Checklist: