Fix flaky SK1 trial eligibility network request test#6956
Conversation
856aa43 to
8db635f
Compare
|
@RCGitBot please test |
| _ = try await self.purchases.checkTrialOrIntroDiscountEligibility(product: product) | ||
|
|
||
| self.logger.verifyMessageWasNotLogged("API request started") | ||
| let introEligibilityPath = HTTPRequest.Path |
There was a problem hiding this comment.
@RevenueCat/coresdk see PR description for background on the issue and solution I went with, happy to hear some thoughts on going with one or the other.
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 5819ac6. Configure here.
| /// - Parameter allowNoMessages: by default, this method requires logs to not be empty | ||
| /// to eliminate the possibility of false positives due to log handler not being installed properly. |
There was a problem hiding this comment.
This is a good guard, but it could make tests that use this depend on external functionality (i.e. a test could theoretically start failing because we remove an unrelated log).
That said, since we can opt-out, I think it's a good compromise, so I'd leave it as well 👍
|
|
||
| private extension StoreKit1IntegrationTests { | ||
|
|
||
| static func verifyIntroEligibilityRequestStartedLogRegexMatchesActualLog( |
There was a problem hiding this comment.
@ajpallares I added this sanity check to make sure the actual http request would be caught if it were to be made. Wdyt?
There was a problem hiding this comment.
Oh nice! So this is mostly to make sure that the logger is registering logs as expected to avoid false positives for the negative verifyMessageWasNotLogged, right?
I think it's a good idea. I'd add a quick documentation for this method:
- Explaining why it exists
- Suggesting calling
logger.clearMessages()afterwards to avoid the logs from this method polluting the result
There was a problem hiding this comment.
Yes mostly, but it's not actually logging it, just checking if our regex (still) matches the description of the HTTP request that's used when actually logging.
I chose not to do actual logging because of the added complexity and the fact that it doesn't prove much more, since the actual implementation could still call the logger with different arguments.
So we also don't have to call logger.clearMessages() :) But thanks for double checking 🙏
There was a problem hiding this comment.
And added the doc comment, good idea!
There was a problem hiding this comment.
Oh true true. I misread the code 🤦♂️ Sorry.
Thanks for adding the comment!
|
@RCGitBot please test |
25dc33c to
cb37ea5
Compare
|
@RCGitBot please test |
Fixes a flaky assertion in
StoreKit2IntegrationTests.testTrialEligibilityMakesNoNetworkRequests.Background
The test asserts that "no network request is made" by checking for the absence of "API request started". However after configuring the SDK other network requests could be made, such as refreshing the customer info.
One solution I initially opted for was forcing the customer info to be refreshed before starting this test. However this didn't feel great as we'd now 'assume' that that request was adding noise and causing the issue. While in the future any other request could potentially cause the same flakiness to happen again.
Fix
Implemented a helper
verifyMessageWasNotLogged(regexPattern:)that asserts that this specific API request was not made by using a regex.Although this also isn't amazing, it at least tightens the assertion a bit. One caveat here (like before) is that we're asserting on the absence of something, which could potentially miss cases in the future once the implementation drifts. Because of this I don't feel super strongly about the one or the other solution direction. Happy to hear any thoughts @RevenueCat/coresdk
In the future we should try to rely less on assertions on log lines and see if we can improve this by using more mocks that record invocations etc.
Tests
Note
Low Risk
Changes are limited to test helpers and integration test assertions; no production SDK behavior is modified.
Overview
Stabilizes the StoreKit integration test that checks trial/intro eligibility does not hit the network by narrowing the log assertion instead of rejecting any
"API request started"line (which could come from unrelated SDK traffic like customer info refresh).TestLogHandlergainsverifyMessageWasNotLogged(regexPattern:), mirroring the existing regex-based positive checks. The eligibility test now uses a pattern targetingintro_eligibilityAPI start logs, and a small regex self-check builds the realPOST /get_intro_eligibilitylog string so the negative assertion is less likely to be a no-op if logging format changes.Reviewed by Cursor Bugbot for commit cb37ea5. Bugbot is set up for automated code reviews on this repo. Configure here.