Add tests for Semaphore type #5

dfed · 2022-12-10T07:14:00Z

This PR adds tests for the Semaphore type used in unit tests.

These tests aren't strictly necessary since the Semaphore type only lives under the Tests directory. But I found writing these tests instructive, so I figured they'd be worthwhile to land in the repo as prior art.

codecov · 2022-12-10T07:20:36Z

Codecov Report

Merging #5 (4efc901) into main (71212a7) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              main        #5    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files            2         7     +5     
  Lines           95       489   +394     
==========================================
+ Hits            95       489   +394

Impacted Files	Coverage Δ
Tests/AsyncQueueTests/SemaphoreTests.swift	`100.00% <100.00%> (ø)`
Tests/AsyncQueueTests/ActorQueueTests.swift	`100.00% <0.00%> (ø)`
Tests/AsyncQueueTests/Utilities/Semaphore.swift	`100.00% <0.00%> (ø)`
Tests/AsyncQueueTests/FIFOQueueTests.swift	`100.00% <0.00%> (ø)`
Tests/AsyncQueueTests/Utilities/Counter.swift	`100.00% <0.00%> (ø)`

dfed · 2023-01-08T23:34:12Z

Given the CI failure, pulling this back into Draft. The failure on sha 8cad925:

/Users/runner/work/swift-async-queue/swift-async-queue/Tests/AsyncQueueTests/SemaphoreTests.swift:113: error: -[AsyncQueueTests.SemaphoreTests test_wait_suspendsUntilEqualNumberOfSignalCalls] : XCTAssertEqual failed: ("194") is not equal to ("0")
[8046](https://github.com/dfed/swift-async-queue/actions/runs/3869442389/jobs/6595538946#step:6:8047)
/Users/runner/work/swift-async-queue/swift-async-queue/Tests/AsyncQueueTests/SemaphoreTests.swift:88: error: -[AsyncQueueTests.SemaphoreTests test_wait_suspendsUntilEqualNumberOfSignalCalls] : XCTAssertTrue failed
[8047](https://github.com/dfed/swift-async-queue/actions/runs/3869442389/jobs/6595538946#step:6:8048)

This is entirely unexpected. Per the XCTAssertTrue failure, we did not suspend when we called let didSuspend = await self.systemUnderTest.wait() on line 87. Since we didn't suspend, the XCTAssertEqual failure makes sense. The question is: why did we not suspend?

It seems like there might be some unexpected interleaving between the executor's isolated context and the semaphore's isolated context. I'm going to try to utilize a single isolated context to improve the reliability of these tests.

dfed · 2023-01-09T02:37:52Z

codecov.yml

-ignore:
-  # This package is not shipped.
-  - "Tests"


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 ignore if maintaining this level of coverage proves problematic. But I like striving for full coverage in a library like this.

dfed · 2023-01-09T02:41:54Z

codecov.yml

    project:
      default:
-        threshold: 0.25%
+        target: 100%


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.

dfed · 2023-01-09T02:46:05Z

Figured it out! 3cdb8b0 solved the issue, and then I went overboard with unit test coverage requirements in the following commits. Marking as ready for review!

bachand

I'm glad we're focused on solid test coverage 💪

Tests/AsyncQueueTests/SemaphoreTests.swift

bachand · 2023-02-08T02:49:49Z

Tests/AsyncQueueTests/SemaphoreTests.swift

+         we utilize an Actor (which has inherently ordered execution) to enqueue ordered `Task`s. We make calls to this actor
+         `await` the beginning of the `Task` to ensure that each `Task` has begun before resuming the test's execution.


I think a word is missing in:

We make calls to this actor await the beginning of the Task to ensure that each Task has begun before resuming the test's execution.

welp. Thanks for catching! I cleaned it up a bit in 4a2848e, but tbh I'm not entirely sure what I was originally trying to say there.

bachand · 2023-02-08T02:55:36Z

Tests/AsyncQueueTests/SemaphoreTests.swift

+         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 before calling `signal()` since that would effectively deadlock the test.


We can't await the wait() call on the main thread right?

I see us awaiting wait() on L64 before we call signal() on L70.

yeah we can't await the wait() on the test's running queue. Updated in 519e1ed

bachand · 2023-02-08T02:58:38Z

Tests/AsyncQueueTests/SemaphoreTests.swift

+    func test_wait_doesNotSuspendIfSignalCalledFirst() async {
+        await systemUnderTest.signal()
+        let didSuspend = await systemUnderTest.wait()
+        XCTAssertFalse(didSuspend)
+    }


bachand · 2023-02-08T03:00:42Z

Tests/AsyncQueueTests/SemaphoreTests.swift

+    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.


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?

Curious to get your thoughts on this ^ when you have a moment @dfed . Tagging you here in case you missed this one.

I did miss this!

the actor context is the semaphore's context

yes!

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

yes!

Thoughts on changing "the actor context" to "the semaphore context"?

Thoughts on changing "the actor context" to "the semaphore context"?

Yeah I think that would increase clarity 👍

bachand · 2023-02-08T03:04:55Z

Tests/AsyncQueueTests/SemaphoreTests.swift

+private final class UnsafeCounter: @unchecked Sendable {
+    var countedTasksCompleted = 0
+}


It's OK that this isn't thread-safe since it's only accessed on the actor's context. Is that right?

Correct! Hence the /// A counter that will only be accessed from within the systemUnderTest's context comment 🙂

Co-authored-by: Michael Bachand <bachand.michael@gmail.com>

dfed requested review from bachand and omv-libs December 10, 2022 07:14

dfed marked this pull request as ready for review December 12, 2022 05:56

dfed mentioned this pull request Dec 12, 2022

Rename AsyncQueue -> FIFOQueue. Create ActorQueue #3

Merged

Base automatically changed from dfed--fifo-vs-actor to main January 8, 2023 23:20

Add tests for Semaphore type

8cad925

dfed force-pushed the dfed--semaphore-tests branch from aa470b6 to 8cad925 Compare January 8, 2023 23:24

dfed marked this pull request as draft January 8, 2023 23:34

Utilize a single actor rather than two actors to increase determinism

3cdb8b0

dfed force-pushed the dfed--semaphore-tests branch from 232dbf2 to 3cdb8b0 Compare January 9, 2023 02:28

Ensure 100% coverage of all code, including test code

8969a8b

dfed commented Jan 9, 2023

View reviewed changes

Be explicit about wanting 100% coverage

1c8031f

dfed commented Jan 9, 2023

View reviewed changes

dfed marked this pull request as ready for review January 9, 2023 02:46

Merge branch 'main' into dfed--semaphore-tests

aeb1709

bachand approved these changes Feb 8, 2023

View reviewed changes

dfed and others added 4 commits February 7, 2023 19:12

Call super.tearDown() last

ed28b2c

Co-authored-by: Michael Bachand <bachand.michael@gmail.com>

Clean up comment

4a2848e

clean up wording

519e1ed

Merge branch 'main' into dfed--semaphore-tests

4efc901

dfed enabled auto-merge (squash) February 8, 2023 03:33

dfed merged commit bc349b2 into main Feb 8, 2023

dfed deleted the dfed--semaphore-tests branch February 8, 2023 03:42

		we utilize an Actor (which has inherently ordered execution) to enqueue ordered `Task`s. We make calls to this actor
		`await` the beginning of the `Task` to ensure that each `Task` has begun before resuming the test's execution.

Uh oh!

Add tests for Semaphore type #5

Add tests for Semaphore type #5

Uh oh!

Conversation

dfed commented Dec 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Dec 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dfed commented Jan 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dfed commented Jan 9, 2023

Uh oh!

bachand left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dfed Feb 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dfed commented Dec 10, 2022 •

edited

Loading

codecov bot commented Dec 10, 2022 •

edited

Loading

dfed commented Jan 8, 2023 •

edited

Loading

dfed Feb 8, 2023 •

edited

Loading