From 0729e8f729cb68d6f6df15471105c89b12e86d12 Mon Sep 17 00:00:00 2001 From: Dennis Weissmann Date: Fri, 12 Jul 2024 17:26:45 -0700 Subject: [PATCH 1/5] [API Proposal] Promote TimeLimitTrait.Duration and associated decls to public API --- ...ranularity-of-test-time-limit-durations.md | 196 ++++++++++++++++++ 1 file changed, 196 insertions(+) create mode 100644 Documentation/Proposals/NNNN-constrain-the-granularity-of-test-time-limit-durations.md diff --git a/Documentation/Proposals/NNNN-constrain-the-granularity-of-test-time-limit-durations.md b/Documentation/Proposals/NNNN-constrain-the-granularity-of-test-time-limit-durations.md new file mode 100644 index 000000000..b98e768c5 --- /dev/null +++ b/Documentation/Proposals/NNNN-constrain-the-granularity-of-test-time-limit-durations.md @@ -0,0 +1,196 @@ +# Constrain the granularity of test time limit durations + +* Proposal: +[SWT-NNNN](NNNN-constrain-the-granularity-of-test-time-limit-durations.md) +* Authors: [Dennis Weissmann](https://github.com/dennisweissmann) +* Status: **Awaiting review** +* Implementation: +[apple/swift-testing#NNNNN](https://github.com/apple/swift-testing/pull/NNNNN) +* Review: +([pitch](https://forums.swift.org/t/pitch-constrain-the-granularity-of-test-time +-limit-durations/73146)) + +## Introduction + +Sometimes tests might get into a state (either due the test code itself or due +to the code they're testing) where they don't make forward progress and hang. +Swift Testing provides a way to handle these issues using the TimeLimit trait: + +``` +@Test(.timeLimit(.minutes(60)) +func testFunction() { ... } +``` + +Currently there exist multiple overloads for the `.timeLimit` trait: one that +takes a `Swift.Duration` which allows for arbitrary `Duration` values to be +passed, and one that takes a `TimeLimitTrait.Duration` which constrains the +minimum time limit as well as the increment to 1 minute. + +## Motivation + +Small time limit values in particular cause more harm than good due to tests +running in environments with drastically differing performance characteristics. +Particularly when running in CI systems or on virtualized hardware tests can +run much slower than at desk. +Swift Testing should help developers use a reasonable time limit value in its +API without developers having to refer to the documentation. + +It is crucial to emphasize that unit tests failing due to exceeding their +timeout should be exceptionally rare. At the same time, a spurious unit test +failure caused by a short timeout can be surprisingly costly, potentially +leading to an entire CI pipeline being rerun. Determining an appropriate +timeout for a specific test can be a challenging task. + +Additionally, when the system intentionally runs multiple tests simultaneously +to optimize resource utilization, the scheduler becomes the arbiter of test +execution. Consequently, the test may take significantly longer than +anticipated, potentially due to external factors beyond the control of the code +under test. + +A unit test should be capable of failing due to hanging, but it should not fail +due to being slow, unless the developer has explicitly indicated that it +should, effectively transforming it into a performance test. + +The time limit feature is *not* intended to be used to apply small timeouts to +tests to ensure test runtime doesn't regress by small amounts. This feature is +intended to be used to guard against hangs and pathologically long running +tests. + +## Proposed Solution + +We propose changing the `.timeLimit` API to accept values of a new `Duration` +type defined in `TimeLimitTrait` which only allows for `.minute` values to be +passed. +These types already exist as SPI and this proposal is seeking to making these +API. + +## Detailed Design + +The `TimeLimitTrait.Duration` struct only has one factory method: `public +static func minutes(_ minutes: some BinaryInteger) -> Self`. + +That ensures 2 things: +1. It's impossible to create short time limits (under a minute). +2. It's impossible to create high-precision increments of time. + +Both of these features are important to ensure the API is self documenting and +conveying the intended purpose. + +For parameterized tests these time limits apply to each individual test case. + +The `TimeLimitTrait.Duration` struct is declared as follows: + +```swift +/// A type that defines a time limit to apply to a test. +/// +/// To add this trait to a test, use one of the following functions: +/// +/// - ``Trait/timeLimit(_:)`` +@available(macOS 13.0, iOS 16.0, tvOS 16.0, watchOS 9.0, *) +public struct TimeLimitTrait: TestTrait, SuiteTrait { + /// A type representing the duration of a time limit applied to a test. + /// + /// This type is intended for use specifically for specifying test timeouts + /// with ``TimeLimitTrait``. It is used instead of Swift's built-in `Duration` + /// type because test timeouts do not support high-precision, arbitrarily + /// short durations. The smallest allowed unit of time is minutes. + public struct Duration: Sendable { + + /// Construct a time limit duration given a number of minutes. + /// + /// - Parameters: + /// - minutes: The number of minutes the resulting duration should + /// represent. + /// + /// - Returns: A duration representing the specified number of minutes. + public static func minutes(_ minutes: some BinaryInteger) -> Self + } + + /// The maximum amount of time a test may run for before timing out. + public var timeLimit: Swift.Duration +} +``` + +The extension on `Trait` that allows for `.timeLimit(...)` to work is defined +like this: + +``` +/// Construct a time limit trait that causes a test to time out if it runs for +/// too long. +/// +/// - Parameters: +/// - timeLimit: The maximum amount of time the test may run for. +/// +/// - Returns: An instance of ``TimeLimitTrait``. +/// +/// Test timeouts do not support high-precision, arbitrarily short durations +/// due to variability in testing environments. The time limit must be at +/// least one minute, and can only be expressed in increments of one minute. +/// +/// When this trait is associated with a test, that test must complete within +/// a time limit of, at most, `timeLimit`. If the test runs longer, an issue +/// of kind ``Issue/Kind/timeLimitExceeded(timeLimitComponents:)`` is +/// recorded. This timeout is treated as a test failure. +/// +/// The time limit amount specified by `timeLimit` may be reduced if the +/// testing library is configured to enforce a maximum per-test limit. When +/// such a maximum is set, the effective time limit of the test this trait is +/// applied to will be the lesser of `timeLimit` and that maximum. This is a +/// policy which may be configured on a global basis by the tool responsible +/// for launching the test process. Refer to that tool's documentation for +/// more details. +/// +/// If a test is parameterized, this time limit is applied to each of its +/// test cases individually. If a test has more than one time limit associated +/// with it, the shortest one is used. A test run may also be configured with +/// a maximum time limit per test case. +public static func timeLimit(_ timeLimit: Self.Duration) -> Self +``` + +And finally, the call site of the API looks like this: + +```swift +@Test(.timeLimit(.minutes(60)) +func serve100CustomersInOneHour() async { + for _ in 0 ..< 100 { + let customer = await Customer.next() + await customer.order() + ... + } +} +``` + +The `TimeLimitTrait.Duration` struct has various `unavailable` overloads that +are included for diagnostic purposes only. They are all documented and +annotated like this: + +``` +/// Construct a time limit duration given a number of . +/// +/// This function is unavailable and is provided for diagnostic purposes only. +@available(*, unavailable, message: "Time limit must be specified in minutes") +``` + +## Source Compatibility + +This is purely additional API and does not impact existing code. + +## Integration with Supporting Tools + +N/A + +## Future Directions + +N/A. + +## Alternatives Considered + +We have considered using `Swift.Duration` as the currency type for this API but +decided against it to avoid common pitfals and misuses of this feature such as +providing very small time limits that lead to flaky tests in different +environments. + +## Acknowledgments + +The authors acknowledge valuable contributions and feedback from the Swift +Testing community during the development of this proposal. From bc84af5ce28428097f461c1cb24e60b8ba1e9fbf Mon Sep 17 00:00:00 2001 From: Dennis Weissmann Date: Thu, 25 Jul 2024 13:11:01 -0700 Subject: [PATCH 2/5] update --- ...anularity-of-test-time-limit-durations.md} | 29 +++++++++++-------- 1 file changed, 17 insertions(+), 12 deletions(-) rename Documentation/Proposals/{NNNN-constrain-the-granularity-of-test-time-limit-durations.md => 0004-constrain-the-granularity-of-test-time-limit-durations.md} (91%) diff --git a/Documentation/Proposals/NNNN-constrain-the-granularity-of-test-time-limit-durations.md b/Documentation/Proposals/0004-constrain-the-granularity-of-test-time-limit-durations.md similarity index 91% rename from Documentation/Proposals/NNNN-constrain-the-granularity-of-test-time-limit-durations.md rename to Documentation/Proposals/0004-constrain-the-granularity-of-test-time-limit-durations.md index b98e768c5..19a3b357c 100644 --- a/Documentation/Proposals/NNNN-constrain-the-granularity-of-test-time-limit-durations.md +++ b/Documentation/Proposals/0004-constrain-the-granularity-of-test-time-limit-durations.md @@ -1,14 +1,13 @@ # Constrain the granularity of test time limit durations * Proposal: -[SWT-NNNN](NNNN-constrain-the-granularity-of-test-time-limit-durations.md) +[SWT-0004](0004-constrain-the-granularity-of-test-time-limit-durations.md) * Authors: [Dennis Weissmann](https://github.com/dennisweissmann) -* Status: **Awaiting review** +* Status: **Awaiting Review** * Implementation: -[apple/swift-testing#NNNNN](https://github.com/apple/swift-testing/pull/NNNNN) +[apple/swift-testing#534](https://github.com/apple/swift-testing/pull/534) * Review: -([pitch](https://forums.swift.org/t/pitch-constrain-the-granularity-of-test-time --limit-durations/73146)) +([pitch](https://forums.swift.org/t/pitch-constrain-the-granularity-of-test-time-limit-durations/73146)) ## Introduction @@ -66,8 +65,10 @@ API. ## Detailed Design -The `TimeLimitTrait.Duration` struct only has one factory method: `public -static func minutes(_ minutes: some BinaryInteger) -> Self`. +The `TimeLimitTrait.Duration` struct only has one factory method: +```swift +public static func minutes(_ minutes: some BinaryInteger) -> Self` +``` That ensures 2 things: 1. It's impossible to create short time limits (under a minute). @@ -114,7 +115,7 @@ public struct TimeLimitTrait: TestTrait, SuiteTrait { The extension on `Trait` that allows for `.timeLimit(...)` to work is defined like this: -``` +```swift /// Construct a time limit trait that causes a test to time out if it runs for /// too long. /// @@ -160,11 +161,11 @@ func serve100CustomersInOneHour() async { } ``` -The `TimeLimitTrait.Duration` struct has various `unavailable` overloads that -are included for diagnostic purposes only. They are all documented and +The `TimeLimitTrait.Duration` struct has various `unavailable` overloads that +are included for diagnostic purposes only. They are all documented and annotated like this: -``` +```swift /// Construct a time limit duration given a number of . /// /// This function is unavailable and is provided for diagnostic purposes only. @@ -181,7 +182,11 @@ N/A ## Future Directions -N/A. +We could reconsider the granularity constraints and allow for more finegrained +time limits if we come to the conclusion that the advantages outweigh the +disadvantages. +Part of that could be the automatic detection of environments (like CI vs local +and providing a way to use different timeouts in different environments. ## Alternatives Considered From 1ec5a8399a799cf233340cf1ac91ee2251781179 Mon Sep 17 00:00:00 2001 From: Dennis Weissmann Date: Thu, 25 Jul 2024 13:37:56 -0700 Subject: [PATCH 3/5] update --- ...ranularity-of-test-time-limit-durations.md | 25 ++++++++++--------- 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/Documentation/Proposals/0004-constrain-the-granularity-of-test-time-limit-durations.md b/Documentation/Proposals/0004-constrain-the-granularity-of-test-time-limit-durations.md index 19a3b357c..3f615b2f4 100644 --- a/Documentation/Proposals/0004-constrain-the-granularity-of-test-time-limit-durations.md +++ b/Documentation/Proposals/0004-constrain-the-granularity-of-test-time-limit-durations.md @@ -15,7 +15,7 @@ Sometimes tests might get into a state (either due the test code itself or due to the code they're testing) where they don't make forward progress and hang. Swift Testing provides a way to handle these issues using the TimeLimit trait: -``` +```swift @Test(.timeLimit(.minutes(60)) func testFunction() { ... } ``` @@ -60,14 +60,13 @@ tests. We propose changing the `.timeLimit` API to accept values of a new `Duration` type defined in `TimeLimitTrait` which only allows for `.minute` values to be passed. -These types already exist as SPI and this proposal is seeking to making these -API. +This type already exists as SPI and this proposal is seeking to making it API. ## Detailed Design The `TimeLimitTrait.Duration` struct only has one factory method: ```swift -public static func minutes(_ minutes: some BinaryInteger) -> Self` +public static func minutes(_ minutes: some BinaryInteger) -> Self ``` That ensures 2 things: @@ -108,7 +107,7 @@ public struct TimeLimitTrait: TestTrait, SuiteTrait { } /// The maximum amount of time a test may run for before timing out. - public var timeLimit: Swift.Duration + public var timeLimit: Swift.Duration { get set } } ``` @@ -174,7 +173,9 @@ annotated like this: ## Source Compatibility -This is purely additional API and does not impact existing code. +This impacts clients that have adopted the `.timeLimit` trait and use overloads +of the trait that accept an arbitrary `Swift.Duration` except if they used the +`minutes` overload. ## Integration with Supporting Tools @@ -182,16 +183,16 @@ N/A ## Future Directions -We could reconsider the granularity constraints and allow for more finegrained -time limits if we come to the conclusion that the advantages outweigh the -disadvantages. -Part of that could be the automatic detection of environments (like CI vs local -and providing a way to use different timeouts in different environments. +We could allow more finegrained time limits in the future that scale with the +performance of the test host device. +Or take a more manual approach where we detect the type of environment +(like CI vs local) and provide a way to use different timeouts depending on the +environment. ## Alternatives Considered We have considered using `Swift.Duration` as the currency type for this API but -decided against it to avoid common pitfals and misuses of this feature such as +decided against it to avoid common pitfalls and misuses of this feature such as providing very small time limits that lead to flaky tests in different environments. From 56bdf3aad63d36850387983616fb6a00cd49870a Mon Sep 17 00:00:00 2001 From: Dennis Weissmann Date: Thu, 25 Jul 2024 13:47:19 -0700 Subject: [PATCH 4/5] impl --- Sources/Testing/Traits/TimeLimitTrait.swift | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Sources/Testing/Traits/TimeLimitTrait.swift b/Sources/Testing/Traits/TimeLimitTrait.swift index 8828a703a..fe1d7f787 100644 --- a/Sources/Testing/Traits/TimeLimitTrait.swift +++ b/Sources/Testing/Traits/TimeLimitTrait.swift @@ -21,7 +21,6 @@ public struct TimeLimitTrait: TestTrait, SuiteTrait { /// with ``TimeLimitTrait``. It is used instead of Swift's built-in `Duration` /// type because test timeouts do not support high-precision, arbitrarily /// short durations. The smallest allowed unit of time is minutes. - @_spi(Experimental) public struct Duration: Sendable { /// The underlying Swift `Duration` which this time limit duration /// represents. @@ -84,6 +83,7 @@ extension Trait where Self == TimeLimitTrait { /// test cases individually. If a test has more than one time limit associated /// with it, the shortest one is used. A test run may also be configured with /// a maximum time limit per test case. + @_spi(Experimental) public static func timeLimit(_ timeLimit: Duration) -> Self { return Self(timeLimit: timeLimit) } @@ -117,7 +117,6 @@ extension Trait where Self == TimeLimitTrait { /// test cases individually. If a test has more than one time limit associated /// with it, the shortest one is used. A test run may also be configured with /// a maximum time limit per test case. - @_spi(Experimental) public static func timeLimit(_ timeLimit: Self.Duration) -> Self { return Self(timeLimit: timeLimit.underlyingDuration) } From cca84c8db058e169104b21a66abf0f6bd56ff104 Mon Sep 17 00:00:00 2001 From: Dennis Weissmann Date: Thu, 25 Jul 2024 16:58:07 -0700 Subject: [PATCH 5/5] acceptance --- ...constrain-the-granularity-of-test-time-limit-durations.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Documentation/Proposals/0004-constrain-the-granularity-of-test-time-limit-durations.md b/Documentation/Proposals/0004-constrain-the-granularity-of-test-time-limit-durations.md index 3f615b2f4..bf91eb84f 100644 --- a/Documentation/Proposals/0004-constrain-the-granularity-of-test-time-limit-durations.md +++ b/Documentation/Proposals/0004-constrain-the-granularity-of-test-time-limit-durations.md @@ -3,11 +3,12 @@ * Proposal: [SWT-0004](0004-constrain-the-granularity-of-test-time-limit-durations.md) * Authors: [Dennis Weissmann](https://github.com/dennisweissmann) -* Status: **Awaiting Review** +* Status: **Accepted** * Implementation: [apple/swift-testing#534](https://github.com/apple/swift-testing/pull/534) * Review: -([pitch](https://forums.swift.org/t/pitch-constrain-the-granularity-of-test-time-limit-durations/73146)) +([pitch](https://forums.swift.org/t/pitch-constrain-the-granularity-of-test-time-limit-durations/73146)), +([acceptance](https://forums.swift.org/t/pitch-constrain-the-granularity-of-test-time-limit-durations/73146/3)) ## Introduction