Skip to content

Conversation

thoven87
Copy link
Contributor

First pass at feature #512

What's the best way to go about writing unit test for the withTransaction func?

Copy link
Collaborator

@fabianfett fabianfett left a comment

Choose a reason for hiding this comment

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

Added some more comments.

@thoven87 thoven87 requested a review from fabianfett December 16, 2024 04:49
@MahdiBM MahdiBM mentioned this pull request Jan 30, 2025
@MahdiBM MahdiBM linked an issue Jan 30, 2025 that may be closed by this pull request
Copy link

codecov bot commented Jan 31, 2025

Codecov Report

Attention: Patch coverage is 0% with 10 lines in your changes missing coverage. Please review.

Project coverage is 61.68%. Comparing base (8d07f20) to head (4641553).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
Sources/PostgresNIO/Pool/PostgresClient.swift 0.00% 10 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@ ## main #519 +/- ## ========================================== - Coverage 61.74% 61.68% -0.06%  ========================================== Files 125 125 Lines 10089 10099 +10 ========================================== + Hits 6229 6230 +1  - Misses 3860 3869 +9 
Files with missing lines Coverage Δ
Sources/PostgresNIO/Pool/PostgresClient.swift 0.00% <0.00%> (ø)

... and 1 file with indirect coverage changes

/// - Parameter closure: A closure that uses the passed `PostgresConnection`. The closure **must not** capture
/// the provided `PostgresConnection`.
/// - Returns: The closure's return value.
public func withTransaction<Result>(_ process: (PostgresConnection) async throws -> Result) async throws -> Result {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if we should capture calling file and line here. Then we could attach that info to the error that is thrown. We would wrap the thrown error in a PostgresTransactionError. We could also attach the Rollback error, if that happens. cc @gwynne WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense to me, yeah.

thoven87 and others added 2 commits February 4, 2025 14:44
@fabianfett
Copy link
Collaborator

Thanks @thoven87! Let's merge this and I'll do the error handling in a follow up!

@fabianfett fabianfett merged commit 712740b into vapor:main Feb 11, 2025
9 checks passed
@fabianfett fabianfett added the semver-minor Adds new public API. label Feb 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

semver-minor Adds new public API.

4 participants