-
-
Notifications
You must be signed in to change notification settings - Fork 14
Add tests for Semaphore type #5
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
Changes from all commits
8cad925
3cdb8b0
8969a8b
1c8031f
aeb1709
ed28b2c
4a2848e
519e1ed
4efc901
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,143 @@ | ||
| // MIT License | ||
| // | ||
| // Copyright (c) 2022 Dan Federman | ||
| // | ||
| // Permission is hereby granted, free of charge, to any person obtaining a copy | ||
| // of this software and associated documentation files (the "Software"), to deal | ||
| // in the Software without restriction, including without limitation the rights | ||
| // to use, copy, modify, merge, publish, distribute, sublicense, and/or sell | ||
| // copies of the Software, and to permit persons to whom the Software is | ||
| // furnished to do so, subject to the following conditions: | ||
| // | ||
| // The above copyright notice and this permission notice shall be included in all | ||
| // copies or substantial portions of the Software. | ||
| // | ||
| // THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR | ||
| // IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, | ||
| // FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE | ||
| // AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER | ||
| // LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, | ||
| // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE | ||
| // SOFTWARE. | ||
|
|
||
| import XCTest | ||
|
|
||
| final class SemaphoreTests: XCTestCase { | ||
|
|
||
| // MARK: XCTestCase | ||
|
|
||
| override func setUp() async throws { | ||
| try await super.setUp() | ||
|
|
||
| systemUnderTest = Semaphore() | ||
| } | ||
|
|
||
| override func tearDown() async throws { | ||
| let isWaiting = await systemUnderTest.isWaiting | ||
| XCTAssertFalse(isWaiting) | ||
|
|
||
| try await super.tearDown() | ||
| } | ||
|
|
||
| // MARK: Behavior Tests | ||
|
|
||
| func test_wait_suspendsUntilEqualNumberOfSignalCalls() async { | ||
| /* | ||
| This test is tricky to pull off! | ||
| Our requirements: | ||
| 1. We need to call `wait()` before `signal()` | ||
| 2. We need to ensure that the `wait()` call suspends _before_ we call `signal()` | ||
| 3. We can't `await` the `wait()` call on the test's queue before calling `signal()` since that would deadlock the test. | ||
| 4. We must utilize a single actor's isolated context to avoid accidental interleaving when suspending to communicate across actor contexts. | ||
|
|
||
| In order to ensure that we are executing the `wait()` calls before we call `signal()` _without awaiting a `wait()` call_, | ||
| we utilize the Sempahore's ordered execution context to enqueue ordered `Task`s similar to how an ActorQueue works. | ||
| */ | ||
|
|
||
| let iterationCount = 1_000 | ||
| /// A counter that will only be accessed from within the `systemUnderTest`'s context | ||
| let unsafeCounter = UnsafeCounter() | ||
|
|
||
| for _ in 1...iterationCount { | ||
| await systemUnderTest.enqueueAndCount(using: unsafeCounter) { systemUnderTest in | ||
| let didSuspend = await systemUnderTest.wait() | ||
| XCTAssertTrue(didSuspend) | ||
|
|
||
| return { systemUnderTest in | ||
| // Signal that the suspended wait call above has resumed. | ||
| // This signal allows us to `wait()` for all of these enqueued `wait()` tasks to have completed later in this test. | ||
| systemUnderTest.signal() | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Loop one fewer than iterationCount. | ||
| for _ in 1..<iterationCount { | ||
| await systemUnderTest.execute { systemUnderTest in | ||
| systemUnderTest.signal() | ||
| } | ||
| } | ||
|
|
||
| await systemUnderTest.execute { systemUnderTest in | ||
| // Give each suspended `wait` task an opportunity to resume (if they were to resume, which they won't) before we check the count. | ||
| for _ in 1...iterationCount { | ||
| await Task.yield() | ||
| } | ||
|
|
||
| // The count will still be zero each time because we have executed one more `wait` than `signal` calls. | ||
| let completedCountedTasks = unsafeCounter.countedTasksCompleted | ||
| XCTAssertEqual(completedCountedTasks, 0) | ||
|
|
||
| // Signal one last time, enabling all of the original `wait` calls to resume. | ||
| systemUnderTest.signal() | ||
|
|
||
| for _ in 1...iterationCount { | ||
| // Wait for all enqueued `wait`s to have completed and signaled their completion. | ||
| await systemUnderTest.wait() | ||
| } | ||
|
|
||
| let tasksCompleted = unsafeCounter.countedTasksCompleted | ||
| XCTAssertEqual(iterationCount, tasksCompleted) | ||
| } | ||
| } | ||
|
|
||
| func test_wait_doesNotSuspendIfSignalCalledFirst() async { | ||
| await systemUnderTest.signal() | ||
| let didSuspend = await systemUnderTest.wait() | ||
| XCTAssertFalse(didSuspend) | ||
| } | ||
|
|
||
| // MARK: Private | ||
|
|
||
| private var systemUnderTest = Semaphore() | ||
| } | ||
|
|
||
| // MARK: - Semaphore Extension | ||
|
|
||
| private extension Semaphore { | ||
| /// Enqueues an asynchronous task. This method suspends the caller until the asynchronous task has begun, ensuring ordered execution of enqueued tasks. | ||
| /// - Parameter task: A unit of work that returns work to execute after the task completes and the count is incremented. | ||
| func enqueueAndCount(using counter: UnsafeCounter, _ task: @escaping @Sendable (isolated Semaphore) async -> ((isolated Semaphore) -> Void)?) async { | ||
| // Await the start of the soon-to-be-enqueued `Task` with a continuation. | ||
| await withCheckedContinuation { continuation in | ||
| // Re-enter the actor context but don't wait for the result. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To make sure I follow... the actor context is the semaphore's context? And the reason that this task is on that context is because this method is isolated to the semaphore by the nature of it being an extension on an actor?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Curious to get your thoughts on this ^ when you have a moment @dfed . Tagging you here in case you missed this one.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did miss this!
yes!
yes! Thoughts on changing "the actor context" to "the semaphore context"?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yeah I think that would increase clarity 👍
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| Task { | ||
| // Now that we're back in the actor context, resume the calling code. | ||
| continuation.resume() | ||
| let executeAfterIncrement = await task(self) | ||
| counter.countedTasksCompleted += 1 | ||
| executeAfterIncrement?(self) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| func execute(_ task: @Sendable (isolated Semaphore) async throws -> Void) async rethrows { | ||
| try await task(self) | ||
| } | ||
| } | ||
|
|
||
| // MARK: - UnsafeCounter | ||
|
|
||
| private final class UnsafeCounter: @unchecked Sendable { | ||
| var countedTasksCompleted = 0 | ||
| } | ||
|
Comment on lines
+141
to
+143
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's OK that this isn't thread-safe since it's only accessed on the actor's context. Is that right?
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correct! Hence the |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,9 +10,5 @@ coverage: | |
| status: | ||
| project: | ||
| default: | ||
| threshold: 0.25% | ||
| target: 100% | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similarly, it's a little silly to set a target of 100% coverage. But again, this library is complex, and (thus far) can be written without any runtime assertions that would prove difficult to test. Given how fundamental this repo will be for consumers, let's start out by holding ourselves to a high standard. |
||
| patch: off | ||
|
|
||
| ignore: | ||
| # This package is not shipped. | ||
| - "Tests" | ||
|
Comment on lines
-16
to
-18
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a little silly to require 100% coverage in unit tests but given the complexity of this library I see some benefit. We can always re-add this |
||
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.
👍