Skip to content
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

[SWT-NNNN] Exit tests #324

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

Conversation

grynspan
Copy link
Contributor

@grynspan grynspan commented Apr 2, 2024

One of the first enhancement requests we received for swift-testing was the ability to test for precondition failures and other critical failures that terminate the current process when they occur. This feature is also frequently requested for XCTest. With swift-testing, we have the opportunity to build such a feature in an ergonomic way.

Read the full proposal here.

@grynspan grynspan added tools integration Integration of swift-testing into tools/IDEs windows 🪟 Windows support linux 🐧 Linux support (all distros) darwin 🍎 macOS, iOS, watchOS, tvOS, and visionOS support public-api Affects public API swift-6 labels Apr 2, 2024
@grynspan grynspan self-assigned this Apr 2, 2024
@grynspan grynspan force-pushed the jgrynspan/exit-tests-proposal branch from fe21f14 to 988ddfe Compare April 2, 2024 17:57
@grynspan grynspan added the enhancement New feature or request label Apr 2, 2024
@grynspan grynspan marked this pull request as draft April 2, 2024 22:10
@grynspan grynspan force-pushed the jgrynspan/exit-tests-proposal branch from 988ddfe to 697123a Compare April 10, 2024 17:40
@grynspan grynspan changed the title [SWT-0001] Exit tests [SWT-NNNN] Exit tests Apr 10, 2024
@grynspan grynspan force-pushed the jgrynspan/exit-tests-proposal branch 3 times, most recently from 19f37c3 to 4825821 Compare April 12, 2024 13:30
@grynspan grynspan force-pushed the jgrynspan/exit-tests-proposal branch 2 times, most recently from feb425b to e84b014 Compare April 30, 2024 14:23
@grynspan grynspan added the api-proposal API proposal PRs (documentation only) label May 1, 2024
@grynspan grynspan force-pushed the jgrynspan/exit-tests-proposal branch 2 times, most recently from 6f0e7f0 to 875b870 Compare May 16, 2024 14:41
@grynspan grynspan force-pushed the jgrynspan/exit-tests-proposal branch from 875b870 to 4365611 Compare May 24, 2024 20:47
@grynspan grynspan force-pushed the jgrynspan/exit-tests-proposal branch from 4365611 to 7d25e07 Compare July 12, 2024 15:43
@grynspan
Copy link
Contributor Author

Rebased.

@grynspan grynspan removed the swift-6 label Jul 25, 2024
@grynspan grynspan force-pushed the jgrynspan/exit-tests-proposal branch from cff8e01 to fb4eb29 Compare August 23, 2024 19:50
@grynspan grynspan force-pushed the jgrynspan/exit-tests-proposal branch from fb4eb29 to 4bb8a69 Compare September 4, 2024 15:20
@grynspan grynspan added this to the Swift 6.1 milestone Sep 10, 2024
@grynspan grynspan force-pushed the jgrynspan/exit-tests-proposal branch 2 times, most recently from 93f700c to 71f5f82 Compare September 20, 2024 20:28
@dabrahams
Copy link

Nice. Three notes:

  1. You can probably support exit tests on platforms that don't let you spawn, because there is always(?) a test driver program running on a host system like a Mac. I outlined how this could work for googletest and CMake here: instead of looping over all the tests in a process on the target platform, you loop over the expected-to-fail ones on the host and launch a separate target process for each one.
  2. I'm not familiar enough with what you're doing to know whether you've covered this case already, but it's sometimes important to be able to verify stdout/stderr contents in addition to detecting abnormal exit. I hope your design allows that.
  3. With GTest I found it very inconvenient that platforms weren't uniform w.r.t. what I could detect in an exit test. I had to make uniformity macros that allowed me to write one test for all platforms and verify as much as possible. They are described (and linked) here. A better library would provide these facilities OOTB.

@grynspan
Copy link
Contributor Author

@dabrahams Please read the draft pitch. :) 1. and 2. are covered in it. It sounds like 3. is sufficiently covered I think? .signal() is not supported on Windows because Windows itself only has emulated signal support. I know how we can fix that (by replacing the default signal handlers on Windows that call exit(3) with something that communicates across the "back channel") but I've punted that from the initial proposal for the sake of complexity.

@dabrahams
Copy link

I skimmed the pitch; I didn't see coverage for 1; what I saw was

The need for exit tests on other platforms is just as strong as it is on the supported platforms (macOS, Linux, and Windows). These platforms do not support spawning new processes, so a different mechanism for running exit tests would be needed.

emphasis mine. I read that as meaning “we don't have an answer for these platforms.”

What I see about 2) is an explicit statement that it isn't supported.

It sounds like 3 is not sufficiently covered, since the Windows signal detection case is one of the ones for which I had to build a workaround manually, and you say you're punting it 'till later.

Maybe by “covered” you just mean “discussed.” In that case I don't see much discussion of strategies for handling these things, so I thought my comment would be useful. If it wasn't, feel free to ignore it.

@grynspan
Copy link
Contributor Author

I skimmed the pitch; I didn't see coverage for 1; what I saw was

The need for exit tests on other platforms is just as strong as it is on the supported platforms (macOS, Linux, and Windows). These platforms do not support spawning new processes, so a different mechanism for running exit tests would be needed.

emphasis mine. I read that as meaning “we don't have an answer for these platforms.”

I'm sure that, as a former Apple employee, you can appreciate that my ability to discuss iOS et al. is limited. 🙂

What I see about 2) is an explicit statement that it isn't supported.

It's listed as a future direction. That doesn't mean it can't be done, just that it's not part of this pitch.

It sounds like 3 is not sufficiently covered, since the Windows signal detection case is one of the ones for which I had to build a workaround manually, and you say you're punting it 'till later.

If you want to detect specifically that a Windows process terminated with a specific signal, that's something that needs additional work beyond the scope of this pitch. If you want to detect that a Windows process terminated abnormally, which would include termination due to an unhandled signal, that's supported and covered by the general .failure exit condition.

@grynspan
Copy link
Contributor Author

grynspan commented Sep 24, 2024

To be clear (I'm now reading my initial reply as snarky, which wasn't intentional), I appreciate the feedback @dabrahams. I can certainly amend the pitch to make it clearer we're thinking about these things.

Edit: I added a "future directions" section about Windows signals.

@grynspan grynspan force-pushed the jgrynspan/exit-tests-proposal branch from 71f5f82 to d4800a1 Compare September 24, 2024 18:55
@grynspan
Copy link
Contributor Author

Windows signals are now handled! #766

@grynspan
Copy link
Contributor Author

And stdout/stderr will be soon too. #773

@dabrahams
Copy link

Windows signals are now handled! #766

Nice; That puts you at least one step ahead of googletest, IIRC.

@grynspan grynspan force-pushed the jgrynspan/exit-tests-proposal branch 4 times, most recently from 38b0650 to 0d48878 Compare October 23, 2024 18:27
@grynspan
Copy link
Contributor Author

@swift-ci test

@grynspan grynspan requested a review from stmontgomery October 23, 2024 18:28
@ikesyo ikesyo marked this pull request as ready for review November 15, 2024 15:12
@ikesyo ikesyo marked this pull request as draft November 15, 2024 15:13
@grynspan grynspan modified the milestones: Swift 6.1, Swift 6.2 Nov 19, 2024
One of the first enhancement requests we received for swift-testing was the
ability to test for precondition failures and other critical failures that
terminate the current process when they occur. This feature is also frequently
requested for XCTest. With swift-testing, we have the opportunity to build such
a feature in an ergonomic way.

Read the full proposal [here](https://github.com/apple/swift-testing/blob/jgrynspan/exit-tests-proposal/Documentation/Proposals/NNNN-exit-tests.md).
@grynspan grynspan force-pushed the jgrynspan/exit-tests-proposal branch from 0d48878 to 39551cf Compare December 5, 2024 22:44
@maartene
Copy link

maartene commented Jan 6, 2025

Is there any news on this PR?

@grynspan
Copy link
Contributor Author

grynspan commented Jan 6, 2025

I'm still actively maintaining this branch/PR. We are not planning to formally propose this feature until the testing workgroup is set up, at the earliest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-proposal API proposal PRs (documentation only) darwin 🍎 macOS, iOS, watchOS, tvOS, and visionOS support enhancement New feature or request exit-tests ☠️ Work related to exit tests linux 🐧 Linux support (all distros) public-api Affects public API tools integration Integration of swift-testing into tools/IDEs windows 🪟 Windows support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants