-
Notifications
You must be signed in to change notification settings - Fork 317
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
Fighting flakiness: no longer uses beCloseToDate
in CustomerInfoOfflineEntitlementsStoreKitTest.verifyEntitlement
#4399
base: main
Are you sure you want to change the base?
Conversation
…itTest.verifyEntitlement
purchaseDate: Date(), | ||
purchaseDate: transaction.purchaseDate, |
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.
Note that this is an actual implementation change. However, using the transaction date might be more correct? This was introduced in #2358, but there are no details on this particular property. Let me know if this is actually not what we want.
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.
Seems this is only used for offline entitlements and conceptually it makes sense. Also checked on Android and seems this is the logic we use there so I think it's ok...
But I would like someone from @RevenueCat/catforms to double-check.
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.
Thanks Toni! Hi @RevenueCat/catforms, could you have a quick look? 🙏
@RCGitBot please test |
@RCGitBot please test |
beCloseToDate
in CustomerInfoOfflineEntitlementsStoreKitTest.verifyEntitlement
beCloseToDate
in CustomerInfoOfflineEntitlementsStoreKitTest.verifyEntitlement
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 is great! Thanks for starting on this 🙏. Seems ok to me. I would like someone else from @RevenueCat/catforms to also confirm it's ok
purchaseDate: Date(), | ||
purchaseDate: transaction.purchaseDate, |
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.
Seems this is only used for offline entitlements and conceptually it makes sense. Also checked on Android and seems this is the logic we use there so I think it's ok...
But I would like someone from @RevenueCat/catforms to double-check.
I have seen that
beCloseToDate()
/beCloseToNow()
is one of our sources of flakiness. For instance, see the last test here. A hiccup in the CI server, an extra background process, a little bit more heat in the CircleCI data center, and things can take a fraction longer, causing this to fail.This PR makes the tests deterministic, by defining the exact date we expect. It only tackles
CustomerInfoOfflineEntitlementsStoreKitTest.verifyEntitlement()
for now. More to come in later PRs.