SPI & Codecov #10
SPI & Codecov #10
Conversation
WalkthroughThe changes introduce enhancements to the project's continuous integration and documentation configuration. The GitHub Actions workflow for pull requests is updated to enable code coverage during macOS tests and to upload coverage data using Codecov, with concurrency control added to limit simultaneous runs per branch. Additionally, a new Changes
Possibly related PRs
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/pull-request.yml (2)
117-117: Remove trailing whitespace
Line 117 contains trailing spaces, which trigger a YAML lint error. Please delete the extra spaces on this blank line.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 117-117: trailing spaces
(trailing-spaces)
120-120: Ensure newline at end of file
Add a final newline character to satisfy YAML lint rules (new-line-at-end-of-file).🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 120-120: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/pull-request.yml(2 hunks).spi.yml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/pull-request.yml
[error] 117-117: trailing spaces
(trailing-spaces)
[error] 120-120: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build-and-test (watchos)
- GitHub Check: build-and-test (visionos)
- GitHub Check: build-and-test (tvos)
- GitHub Check: build-and-test (ios)
🔇 Additional comments (3)
.spi.yml (1)
1-7: Configuration Validated: Documentation Targets Defined Correctly
The.spi.ymlfile correctly specifies version 1 and enumerates the intendeddocumentation_targets. No syntax or structure issues detected..github/workflows/pull-request.yml (2)
108-108: Enable code coverage on macOS tests
Adding--enable-code-coveragetoswift testensures coverage data is gathered during macOS runs, which is essential for accurate reporting.
118-120: Upload coverage results to Codecov
The new “Check coverage” step usingcodecov/codecov-action@v5(macOS only) aligns with the coverage collection. This will automate coverage reporting to Codecov.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 120-120: no new line character at the end of file
(new-line-at-end-of-file)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/pull-request.yml (2)
117-117: Remove trailing whitespace
The blank line after the Xcode test step contains trailing spaces, triggering a YAMLLint error. Please remove the trailing spaces on this line to clean up the workflow.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 117-117: trailing spaces
(trailing-spaces)
118-123: Add Codecov upload step for macOS
Great addition of aCheck coverage (SPM)step usingcodecov/codecov-action@v5withfail_ci_if_error: trueto enforce coverage reporting.
Nitpick: Ensure there is a newline at the end of the file to satisfy YAMLLint and POSIX requirements.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 123-123: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/pull-request.yml(2 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/pull-request.yml
[error] 117-117: trailing spaces
(trailing-spaces)
[error] 123-123: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (1)
.github/workflows/pull-request.yml (1)
108-108: Enable code coverage for SPM tests
Adding--enable-code-coverageto the macOS SPM test step ensures that coverage data is generated.
Request Verification: Please confirm that this command produces.profrawfiles under.build/debug/codecov/so the subsequent Codecov action can locate them.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/pull-request.yml (1)
5-6: Remove trailing whitespace and add newline at end-of-file.YAMLlint flags trailing spaces on lines 5, 6, and 124, and the file is missing a final newline (EOF). Please clean up these spaces and append a newline for consistency.
--- a/.github/workflows/pull-request.yml +++ b/.github/workflows/pull-request.yml @@ -5,2 +5,2 @@ - types: [opened, reopened, synchronize, ready_for_review] + types: [opened, reopened, synchronize, ready_for_review] @@ -124 +124 - + @@ -129 +130 - fail_ci_if_error: true + fail_ci_if_error: true +Also applies to: 124-124, 129-129
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 5-5: trailing spaces
(trailing-spaces)
[error] 6-6: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/pull-request.yml(3 hunks).github/workflows/release.yml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/release.yml
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/pull-request.yml
[error] 5-5: trailing spaces
(trailing-spaces)
[error] 6-6: trailing spaces
(trailing-spaces)
[error] 124-124: trailing spaces
(trailing-spaces)
[error] 129-129: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build-and-test (tvos)
- GitHub Check: build-and-test (watchos)
- GitHub Check: build-and-test (ios)
- GitHub Check: build-and-test (visionos)
🔇 Additional comments (4)
.github/workflows/pull-request.yml (4)
3-6: Explicit pull_request event types improve workflow triggers.Specifying
types: [opened, reopened, synchronize, ready_for_review]ensures the job only runs when relevant, reducing unnecessary CI runs. Nicely scoped!🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 5-5: trailing spaces
(trailing-spaces)
[error] 6-6: trailing spaces
(trailing-spaces)
8-9: Prevent overlapping runs with concurrency configuration.The
concurrencygroup using${{ github.workflow }}-${{ github.ref }}andcancel-in-progress: truewill automatically cancel in-flight jobs on new pushes to the same branch, saving CI minutes and avoiding redundant runs.
115-115: Enable code coverage collection for SPM tests on macOS.Adding
--enable-code-coveragetoswift testis essential for gathering coverage reports. It integrates well with the subsequent Codecov upload.
125-129: Upload coverage reports using Codecov.Introducing the
codecov/codecov-action@v5step withfail_ci_if_error: trueensures that coverage is reported and the CI fails on upload errors. This aligns perfectly with your goal of integrating Codecov into the PR workflow.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 129-129: no new line character at the end of file
(new-line-at-end-of-file)
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
.github/workflows/pull-request.yml (1)
16-16:⚠️ Potential issueInvalid runner label
macos-15
GitHub Actions does not support amacos-15runner. This will cause the workflow to fail. Please switch to a valid label such asmacos-latestor a supported version (e.g.,macos-13).Example patch for both jobs:
- runs-on: macos-15 + runs-on: macos-latestAlso applies to: 62-62
🧹 Nitpick comments (2)
.github/workflows/pull-request.yml (2)
5-5: Remove trailing whitespace
The line has trailing spaces, which can cause YAML lint errors (trailing-spaces). Please remove the extra spaces at the end of this line.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 5-5: trailing spaces
(trailing-spaces)
139-139: Add newline at end of file
A missing newline at EOF can lead to lint errors (new-line-at-end-of-file). Please ensure there is a final blank line.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 139-139: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/pull-request.yml(4 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/pull-request.yml
[error] 5-5: trailing spaces
(trailing-spaces)
[error] 139-139: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-and-test (watchos)
- GitHub Check: build-and-test (visionos)
- GitHub Check: build-and-test (ios)
🔇 Additional comments (4)
.github/workflows/pull-request.yml (4)
3-4: Explicitly specifying pull_request event types
Defining the exact PR events (opened, reopened, synchronize, ready_for_review) helps avoid unexpected triggers. This looks good and follows best practices for GitHub Actions event filtering.
7-9: Leverage concurrency to cancel redundant runs
Usingconcurrencywithcancel-in-progress: trueensures that only one workflow runs per branch/ref, improving efficiency and reducing CI costs.
102-102: Skip trivial build step comment
No changes needed; theswift buildstep for the macOS platform is correct.
135-139: Verify Codecov token configuration
Uploading coverage to Codecov requires a valid token (CODECOV_TOKEN) for private repositories. Ensure that you’ve setsecrets.CODECOV_TOKEN, or explicitly pass it in the action inputs.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 139-139: no new line character at the end of file
(new-line-at-end-of-file)
.github/workflows/pull-request.yml
Outdated
| swift test --enable-code-coverage | xcbeautify --renderer github-actions | ||
| TEST_BUNDLE=$(find .build/debug -name "*.xctest" | head -n 1) | ||
| TEST_EXECUTABLE="$TEST_BUNDLE/Contents/MacOS/$(basename "$TEST_BUNDLE" .xctest)" | ||
| llvm-profdata merge -sparse .build/debug/codecov/*.profraw \ | ||
| -o coverage.profdata | ||
| llvm-cov export \ | ||
| "$TEST_EXECUTABLE" \ | ||
| -instr-profile=coverage.profdata \ | ||
| -format=lcov > coverage/lcov.info |
There was a problem hiding this comment.
Ensure coverage directory exists before exporting
The coverage export writes to coverage/lcov.info but there’s no step to create the coverage directory, which will cause an error if the folder doesn’t exist. Please add a directory creation step before llvm-cov export.
Suggested diff:
- llvm-profdata merge -sparse .build/debug/codecov/*.profraw \
+ llvm-profdata merge -sparse .build/debug/codecov/*.profraw \
-o coverage.profdata
+ mkdir -p coverage
llvm-cov export \📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| swift test --enable-code-coverage | xcbeautify --renderer github-actions | |
| TEST_BUNDLE=$(find .build/debug -name "*.xctest" | head -n 1) | |
| TEST_EXECUTABLE="$TEST_BUNDLE/Contents/MacOS/$(basename "$TEST_BUNDLE" .xctest)" | |
| llvm-profdata merge -sparse .build/debug/codecov/*.profraw \ | |
| -o coverage.profdata | |
| llvm-cov export \ | |
| "$TEST_EXECUTABLE" \ | |
| -instr-profile=coverage.profdata \ | |
| -format=lcov > coverage/lcov.info | |
| swift test --enable-code-coverage | xcbeautify --renderer github-actions | |
| TEST_BUNDLE=$(find .build/debug -name "*.xctest" | head -n 1) | |
| TEST_EXECUTABLE="$TEST_BUNDLE/Contents/MacOS/$(basename "$TEST_BUNDLE" .xctest)" | |
| llvm-profdata merge -sparse .build/debug/codecov/*.profraw \ | |
| -o coverage.profdata | |
| mkdir -p coverage | |
| llvm-cov export \ | |
| "$TEST_EXECUTABLE" \ | |
| -instr-profile=coverage.profdata \ | |
| -format=lcov > coverage/lcov.info |
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.github/workflows/pull-request.yml (3)
5-5: Remove trailing whitespace on pull_request types line
YAML lint flagged trailing spaces at the end of this line. Removing them prevents lint failures and maintains consistency.Apply this diff:
- types: [opened, reopened, synchronize, ready_for_review] + types: [opened, reopened, synchronize, ready_for_review]🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 5-5: trailing spaces
(trailing-spaces)
132-136: Specify coverage report file and token for Codecov action
By default, the Codecov action will search for coverage files, but explicitly setting thefileparameter (e.g.,info.lcov) avoids ambiguity. Also verify thatCODECOV_TOKENis configured in repository secrets if this is a private repo.- uses: codecov/codecov-action@v5 - with: - fail_ci_if_error: true + uses: codecov/codecov-action@v5 + with: + file: info.lcov + fail_ci_if_error: true🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 136-136: no new line character at the end of file
(new-line-at-end-of-file)
136-136: Add newline at end of file
YAMLlint flagged a missing newline at the end of this file. Please ensure there's a blank line after the last line to comply with POSIX and avoid tooling issues.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 136-136: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/pull-request.yml(4 hunks)README.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- README.md
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/pull-request.yml
[error] 5-5: trailing spaces
(trailing-spaces)
[error] 136-136: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-and-test (visionos)
- GitHub Check: build-and-test (watchos)
- GitHub Check: build-and-test (ios)
🔇 Additional comments (1)
.github/workflows/pull-request.yml (1)
7-9: Concurrency configuration looks good
Introducing a concurrency group here ensures that only one workflow run per branch/ref is active, preventing race conditions and redundant runs. Thecancel-in-progress: truesetting is a solid choice.
| set -o pipefail | ||
| swift test | xcbeautify --renderer github-actions | ||
| swift test --enable-code-coverage | xcbeautify --renderer github-actions | ||
| TEST_BUNDLE=$(find .build/debug/ -name "*.xctest" | head -n 1) | ||
| TEST_EXECUTABLE="$TEST_BUNDLE/Contents/MacOS/$(basename "$TEST_BUNDLE" .xctest)" | ||
| xcrun llvm-cov export -format="lcov" \ | ||
| -instr-profile .build/debug/codecov/default.profdata \ | ||
| "$TEST_EXECUTABLE" > info.lcov | ||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify generation of .profdata before export
The llvm-cov export step references .build/debug/codecov/default.profdata, but it’s unclear if swift test --enable-code-coverage automatically merges raw profiles into a .profdata file. Confirm that the .profdata exists or insert an llvm-profdata merge step if necessary.
Run this script to check and merge profiles if needed:
🏁 Script executed:
#!/bin/bash
# Verify or merge Swift coverage profiles
profraws=(.build/debug/codecov/*.profraw)
if [ ${#profraws[@]} -gt 0 ]; then
echo "Merging raw profiles into default.profdata"
llvm-profdata merge -sparse "${profraws[@]}" -o .build/debug/codecov/default.profdata
else
echo "No .profraw files found; ensure swift test generates coverage data"
exit 1
fiLength of output: 364
Add an explicit llvm-profdata merge step before exporting coverage
swift test --enable-code-coverage produces .profraw files but does not automatically merge them into a .profdata. Without that merge, the xcrun llvm-cov export … -instr-profile .build/debug/codecov/default.profdata step will fail.
Suggested patch in .github/workflows/pull-request.yml (after the swift test … line and before finding TEST_BUNDLE):
set -o pipefail
swift test --enable-code-coverage | xcbeautify --renderer github-actions
+ # Merge raw Swift coverage profiles into default.profdata
+ profraws=(.build/debug/codecov/*.profraw)
+ if [ ${#profraws[@]} -gt 0 ]; then
+ llvm-profdata merge -sparse "${profraws[@]}" \
+ -o .build/debug/codecov/default.profdata
+ else
+ echo "⚠️ No .profraw files found; ensure coverage is enabled" >&2
+ exit 1
+ fi
TEST_BUNDLE=$(find .build/debug/ -name "*.xctest" | head -n 1)
TEST_EXECUTABLE="$TEST_BUNDLE/Contents/MacOS/$(basename "$TEST_BUNDLE" .xctest)"
xcrun llvm-cov export -format="lcov" \
-instr-profile .build/debug/codecov/default.profdata \
"$TEST_EXECUTABLE" > info.lcovAlso ensure the runner has llvm-profdata available (via brew install llvm or the Apple toolchain).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| set -o pipefail | |
| swift test | xcbeautify --renderer github-actions | |
| swift test --enable-code-coverage | xcbeautify --renderer github-actions | |
| TEST_BUNDLE=$(find .build/debug/ -name "*.xctest" | head -n 1) | |
| TEST_EXECUTABLE="$TEST_BUNDLE/Contents/MacOS/$(basename "$TEST_BUNDLE" .xctest)" | |
| xcrun llvm-cov export -format="lcov" \ | |
| -instr-profile .build/debug/codecov/default.profdata \ | |
| "$TEST_EXECUTABLE" > info.lcov | |
| set -o pipefail | |
| swift test --enable-code-coverage | xcbeautify --renderer github-actions | |
| # Merge raw Swift coverage profiles into default.profdata | |
| profraws=(.build/debug/codecov/*.profraw) | |
| if [ ${#profraws[@]} -gt 0 ]; then | |
| llvm-profdata merge -sparse "${profraws[@]}" \ | |
| -o .build/debug/codecov/default.profdata | |
| else | |
| echo "⚠️ No .profraw files found; ensure coverage is enabled" >&2 | |
| exit 1 | |
| fi | |
| TEST_BUNDLE=$(find .build/debug/ -name "*.xctest" | head -n 1) | |
| TEST_EXECUTABLE="$TEST_BUNDLE/Contents/MacOS/$(basename "$TEST_BUNDLE" .xctest)" | |
| xcrun llvm-cov export -format="lcov" \ | |
| -instr-profile .build/debug/codecov/default.profdata \ | |
| "$TEST_EXECUTABLE" > info.lcov |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Tests/PrincipleConcurrencyTests/SingleUseTransferTests.swift (2)
15-24: Add assertions to verify the test behavior.The test demonstrates the usage of
finalize()but doesn't verify any expected behaviors. Consider adding assertions to validate that:
- The value received from
finalize()is the same instance that was originally wrapped- Calling
finalize()again would trigger a precondition failure@Test func testFinalize() { let mutex = Mutex(NonSendable()) let instance = NonSendable() var transfer = SingleUseTransfer(instance) mutex.withLock { protected in - protected = transfer.finalize() + let finalized = transfer.finalize() + protected = finalized + XCTAssertTrue(instance === finalized, "Finalized value should be the same instance that was wrapped") + + // Verify the transfer is consumed and can't be used again + #if ENABLE_PRECONDITION_TESTS + expectPreconditionFailure { + _ = transfer.finalize() + } + #endif } }
41-44: Consider adding identity verification to the NonSendable class.To better test that the same instance is being transferred, add a unique identifier to the
NonSendableclass.extension SingleUseTransferTests { - private final class NonSendable {} + private final class NonSendable { + let id = UUID() + } }This would allow you to verify that the exact same instance is being transferred and finalized, rather than just checking reference equality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Tests/PrincipleConcurrencyTests/SingleUseTransferTests.swift(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
Tests/PrincipleConcurrencyTests/SingleUseTransferTests.swift (1)
Sources/PrincipleConcurrency/SingleUseTransfer.swift (2)
finalize(79-87)take(69-71)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build-and-test (watchos)
- GitHub Check: build-and-test (tvos)
- GitHub Check: build-and-test (visionos)
- GitHub Check: build-and-test (ios)
🔇 Additional comments (1)
Tests/PrincipleConcurrencyTests/SingleUseTransferTests.swift (1)
9-12:Details
✅ Verification successful
Check required import dependencies.
The test imports both
PrincipleConcurrency(with@testable) andSynchronization. Verify that both imports are necessary and thatSynchronizationis actually used outside of theMuteximplementation.
🏁 Script executed:
#!/bin/bash # Check if the Synchronization module is used directly in other tests grep -r "import Synchronization" Tests/PrincipleConcurrencyTests/ # Check if Mutex is defined in Synchronization ast-grep --pattern $'struct Mutex { $$$' Sources/Synchronization/Length of output: 290
🏁 Script executed:
#!/bin/bash # List top-level directories echo "Root directories:" ls -1 # Locate any directories matching 'Synchronization' echo echo "Directories containing 'Synchronization':" find . -maxdepth 3 -type d | grep -i Synchronization || echo "None found" # Search for Mutex type definition echo echo "Searching for 'struct Mutex' definitions in repo:" rg "struct Mutex" -n . || echo "No 'struct Mutex' definitions found" # Check for Mutex usage in the SingleUseTransferTests.swift echo echo "Mutex references in SingleUseTransferTests.swift:" rg "Mutex" -n Tests/PrincipleConcurrencyTests/SingleUseTransferTests.swift || echo "No occurrences of 'Mutex'"Length of output: 762
No action required:
Synchronizationimport is neededThe test instantiates
Mutex(…), which comes from the externalSynchronizationmodule, so theimport Synchronizationline is indeed necessary.
| @Test | ||
| func testTake() async { | ||
| let instance = NonSendable() | ||
| var transfer = SingleUseTransfer(instance) | ||
|
|
||
| await withTaskGroup { group in | ||
| var transfer = transfer.take() | ||
| group.addTask { | ||
| _ = transfer.finalize() | ||
| } | ||
| await group.waitForAll() | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance concurrent testing scenario.
The current test doesn't effectively test concurrent behavior with just one task. Consider adding multiple concurrent tasks that attempt to finalize the same transfer and verify that only one succeeds.
@Test
func testTake() async {
let instance = NonSendable()
var transfer = SingleUseTransfer(instance)
+ // Use an actor to safely track successful finalizations
+ actor FinalizationTracker {
+ var successCount = 0
+ func incrementSuccess() { successCount += 1 }
+ func getSuccessCount() -> Int { successCount }
+ }
+
+ let tracker = FinalizationTracker()
+
await withTaskGroup { group in
var transfer = transfer.take()
- group.addTask {
- _ = transfer.finalize()
- }
+
+ // Add multiple tasks attempting to finalize the same transfer
+ for _ in 1...10 {
+ group.addTask {
+ do {
+ _ = transfer.finalize()
+ await tracker.incrementSuccess()
+ } catch {
+ // Precondition failure would terminate the process in real code
+ // In a test environment, we'd catch the resulting error
+ }
+ }
+ }
await group.waitForAll()
}
+
+ // Verify only one task successfully finalized the transfer
+ let successCount = await tracker.getSuccessCount()
+ XCTAssertEqual(successCount, 1, "Only one task should successfully finalize the transfer")
}Note: This implementation assumes precondition failures can be caught as errors for testing purposes, which may not be the case in your environment. You might need to adapt this approach based on your testing infrastructure.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Test | |
| func testTake() async { | |
| let instance = NonSendable() | |
| var transfer = SingleUseTransfer(instance) | |
| await withTaskGroup { group in | |
| var transfer = transfer.take() | |
| group.addTask { | |
| _ = transfer.finalize() | |
| } | |
| await group.waitForAll() | |
| } | |
| } | |
| @Test | |
| func testTake() async { | |
| let instance = NonSendable() | |
| var transfer = SingleUseTransfer(instance) | |
| // Use an actor to safely track successful finalizations | |
| actor FinalizationTracker { | |
| var successCount = 0 | |
| func incrementSuccess() { successCount += 1 } | |
| func getSuccessCount() -> Int { successCount } | |
| } | |
| let tracker = FinalizationTracker() | |
| await withTaskGroup { group in | |
| var transfer = transfer.take() | |
| // Add multiple tasks attempting to finalize the same transfer | |
| for _ in 1...10 { | |
| group.addTask { | |
| do { | |
| _ = transfer.finalize() | |
| await tracker.incrementSuccess() | |
| } catch { | |
| // Precondition failure would terminate the process in real code | |
| // In a test environment, we'd catch the resulting error | |
| } | |
| } | |
| } | |
| await group.waitForAll() | |
| } | |
| // Verify only one task successfully finalized the transfer | |
| let successCount = await tracker.getSuccessCount() | |
| XCTAssertEqual(successCount, 1, "Only one task should successfully finalize the transfer") | |
| } |
There was a problem hiding this comment.
This is intentional - SingleUseTransfer is for single use only. The purpose of this test is just to check if take method in the outer closure compiles at all.
There was a problem hiding this comment.
@NSFatalError I understand now, thank you for the clarification. You're right that the test is specifically checking if the take method compiles correctly in the async context, not testing concurrent finalization behavior. Since SingleUseTransfer is designed for single use only, my suggested test wouldn't align with its intended usage pattern.
(_/)
(^.^)
(> <) thinking
✏️ Learnings added
Learnt from: NSFatalError
PR: NSFatalError/Principle#10
File: Tests/PrincipleConcurrencyTests/SingleUseTransferTests.swift:26-38
Timestamp: 2025-04-28T15:06:57.204Z
Learning: `SingleUseTransfer` in PrincipleConcurrency is designed for single use only. The `testTake()` test is specifically checking if the `take` method compiles correctly in an async context, not testing concurrent finalization behavior.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Summary by CodeRabbit