Skip to content

SPI & Codecov #10

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.

Sign up for GitHub

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

Merged
NSFatalError merged 10 commits into from
Apr 28, 2025
Merged

SPI & Codecov #10

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
22e35ce
-
NSFatalError Apr 28, 2025
060eb73
-
NSFatalError Apr 28, 2025
8847ee8
-
NSFatalError Apr 28, 2025
714abdc
-
NSFatalError Apr 28, 2025
69cd43d
-
NSFatalError Apr 28, 2025
cc473a6
-
NSFatalError Apr 28, 2025
998af0b
-
NSFatalError Apr 28, 2025
d49c156
-
NSFatalError Apr 28, 2025
e48a57c
-
NSFatalError Apr 28, 2025
d844249
Added SingleUseTransferTests
NSFatalError Apr 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 22 additions & 2 deletions .github/workflows/pull-request.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,12 @@
name: Pull request
on: pull_request

on:
pull_request:
types: [opened, reopened, synchronize, ready_for_review]

concurrency:
group: ${{ github.workflow }}-${{ github.ref }}
cancel-in-progress: true

env:
XCODE_VERSION: "16.3"
Expand Down Expand Up @@ -92,6 +99,7 @@ jobs:
- name: Build (SPM)
if: ${{ matrix.platform == 'macos' }}
run: swift build

- name: Build (Xcode)
if: ${{ matrix.platform != 'macos' }}
run: |
Expand All @@ -105,7 +113,13 @@ jobs:
if: ${{ matrix.platform == 'macos' }}
run: |
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

Comment on lines 115 to +122
Copy link

@coderabbitai coderabbitai bot Apr 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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
fi

Length 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.lcov

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

Suggested change
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

- name: Test (Xcode)
if: ${{ matrix.platform != 'macos' }}
run: |
Expand All @@ -114,3 +128,9 @@ jobs:
-scheme ${{ needs.prepare.outputs.scheme }} \
-destination "${{ steps.destination.outputs.destination }}" | \
xcbeautify --renderer github-actions

- name: Check coverage (SPM)
if: ${{ matrix.platform == 'macos' }}
uses: codecov/codecov-action@v5
with:
fail_ci_if_error: true
1 change: 1 addition & 0 deletions .github/workflows/release.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
name: Release

on:
push:
tags:
Expand Down
7 changes: 7 additions & 0 deletions .spi.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
version: 1
builder:
configs:
- documentation_targets:
- Principle
- PrincipleConcurrency
- PrincipleCollections
1 change: 1 addition & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# Principle

![Swift](https://img.shields.io/badge/Swift-6.0-EF5239?logo=swift&labelColor=white)
[![Codecov](https://codecov.io/gh/NSFatalError/Principle/graph/badge.svg?token=ITK16CK7NL)](https://codecov.io/gh/NSFatalError/Principle)

Essential tools that extend the capabilities of Swift Standard Library.

Expand Down
44 changes: 44 additions & 0 deletions Tests/PrincipleConcurrencyTests/SingleUseTransferTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
//
// SingleUseTransferTests.swift
// Principle
//
// Created by Kamil Strzelecki on 28/04/2025.
// Copyright © 2025 Kamil Strzelecki. All rights reserved.
//

@testable import PrincipleConcurrency
import Synchronization
import Testing

internal struct SingleUseTransferTests {

@Test
func testFinalize() {
let mutex = Mutex(NonSendable())
let instance = NonSendable()
var transfer = SingleUseTransfer(instance)

mutex.withLock { protected in
protected = transfer.finalize()
}
}

@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()
}
}
Comment on lines +26 to +38
Copy link

@coderabbitai coderabbitai bot Apr 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
@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")
}

Copy link
Owner Author

@NSFatalError NSFatalError Apr 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@coderabbitai coderabbitai bot Apr 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

extension SingleUseTransferTests {

private final class NonSendable {}
}
Loading