Fix flaky metadata-sync & consent-status unit tests#6876
Conversation
4075f21 to
5e7132a
Compare
|
@RCGitBot please test |
5e7132a to
e584d5a
Compare
|
@RCGitBot please test |
b2cd340 to
b628db9
Compare
b628db9 to
ef10830
Compare
|
@RCGitBot please test |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ef10830. Configure here.
| private let transactionPoster: TransactionPosterType | ||
|
|
||
| private let isSyncing: Atomic<Bool> = .init(false) | ||
| let isSyncing: Atomic<Bool> = .init(false) |
There was a problem hiding this comment.
Production isSyncing visibility widened solely for tests
Low Severity
The isSyncing property on TransactionMetadataSyncHelper was changed from private to internal access purely so a test can observe it (PurchasesDelegateTests.waitForInitialMetadataSyncToComplete). External access only occurs in tests, meaning this is a test-only visibility change in production code that ships in release builds without a #if DEBUG guard. This violates the rule requiring test-only hooks in production code to be wrapped in #if DEBUG.
Triggered by learned rule: Test-only hooks in production code must be wrapped in #if DEBUG
Reviewed by Cursor Bugbot for commit ef10830. Configure here.
There was a problem hiding this comment.
Technically true, but I'm not sure if it's worth it. We have a bunch of precedents for this
There was a problem hiding this comment.
Alternatively we could do something like this
private let isSyncing: Atomic<Bool> = .init(false) // back to private
// in a `// MARK: - Exposed for testing only`-style accessor:
var isCurrentlySyncing: Bool { self.isSyncing.value } // read-only
| private let transactionPoster: TransactionPosterType | ||
|
|
||
| private let isSyncing: Atomic<Bool> = .init(false) | ||
| let isSyncing: Atomic<Bool> = .init(false) |
Summary
Fixes a few cases of flaky unit tests:
PurchasesDelegateTests.testApplicationDidBecomeActiveSyncsCachedTransactionMetadata/…SyncsMultipleCachedTransactions/…DoesNotPostWhenNoCachedMetadataBackendSubscriberAttributesTests.testPostReceiptCachesRequestsWhenOnlyConsentStatus…TimestampsFixes
.configuremetadata sync data race; on SDK configuration any locally stored transaction metadata (on disk) is synced (using a fire-and-forget Task{}). In the tests we instantly trigger thedidBecomeActivenotification, which also triggers a sync. At that pointisSyncingis still true (sometimes), since the background Task hasn’t finished yet, causing the sync triggered by the foreground notification to get skipped. We’re now observing theisSyncingin the test in order to wait for the initial sync before testing our triggered sync.Some tests (e.g.
testPostReceiptCachesRequestsWhenOnlyConsentStatusTimestampDiffers) concurrently callPOST /receiptthat use theMockDateProviderto increment thecurrentIndex. However this was done in a non-atomicway, causing data races. Fixed by making the index useAtomic`Validation
Failing run (17/20 failed) link
Successful run after fix (20/20 succeeded) link
Note
Low Risk
Changes are limited to test harness timing, mock thread-safety, and internal visibility of a sync flag; no purchase or networking logic changes.
Overview
Stabilizes flaky unit tests around cached transaction metadata sync and concurrent receipt posting without changing product behavior.
Metadata sync tests now wait for the configure-time metadata sync to finish (
isSyncingcleared and the local store read at least once) before firingapplicationDidBecomeActive, so foreground-triggered sync is not skipped by the in-progress guard. The empty-cache case waits for a second store read instead of a fixed sleep.MockDateProvideradvances its stubbed date index withAtomicso parallelpost(receipt:)tests do not race onnow().TransactionMetadataSyncHelper.isSyncingis exposed (no longerprivate) so tests can observe sync completion; the test base holds a reference to the same helper instance used byPurchases.Reviewed by Cursor Bugbot for commit ef10830. Bugbot is set up for automated code reviews on this repo. Configure here.