-
Notifications
You must be signed in to change notification settings - Fork 50
Implement Protocol-Based Testing with Sendable Conformance in AppStoreServerAPIClient #91
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
base: main
Are you sure you want to change the base?
Conversation
Instead of initializing a http client pool, expose the httpClient through the initializer or default on .shared that way we can share the http pool with others Deinit is no longer needed since httpclient is exposed Create a configuration struct so we are able to throw when initializing config & have a non throwing client initializer
…it tests, since AppStoreServerAPIClient is a final Sendable class now
| // try! used for example purposes only | ||
| let client = try! AppStoreServerAPIClient(signingKey: encodedKey, keyId: keyId, issuerId: issuerId, bundleId: bundleId, environment: environment) | ||
| let config = try! AppStoreServerAPIConfiguration(signingKeyPem: encodedKey, keyId: keyId, issuerId: issuerId, bundleId: bundleId, environment: environment) | ||
| let client = AppStoreServerAPIClient(config: config) |
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.
Is breaking out the configuration necessary for Sendable conformance? Why was this change made
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.
The main goal was use a mocked http client instead of internal override func executeRequest. Sendable conformance is an addition, because why not :)
| transactionHistoryRequest.sort = TransactionHistoryRequest.Order.ascending | ||
| transactionHistoryRequest.revoked = false | ||
| transactionHistoryRequest.productTypes = [TransactionHistoryRequest.ProductType.autoRenewable] | ||
| let transactionHistoryRequest = TransactionHistoryRequest( |
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.
This seems unrelated to to the proposed changes in this PR
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.
I think this triggered a warning, since var should only be used when mutation is logically required.
| } | ||
|
|
||
| public enum APIResult<T> { | ||
| public enum APIResult<T: Sendable>: Sendable{ |
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.
Is this the only change in this PR necessary to actually make this Sendable?
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.
No, there are more Sendable conformances added in the PR.
Summary
This PR implements a protocol-based testing approach inspired by Soto's AWSHTTPClient pattern, making AppStoreServerAPIClient conform to Sendable, and improves the overall architecture for better testability and concurrency safety.
Protocol-Based HTTP Client Architecture
Configuration Improvements
Testing
Documentation Updates