HTTPClientTests: fixed failing test with missing assertions
#2262
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
NachoSoto
commented
Feb 3, 2023
| } | ||
| } | ||
|
|
||
| expect(result).to(beSuccess()) | ||
| expect(requests.value) == 2 |
There was a problem hiding this comment.
NachoSoto
force-pushed
the
coverage-flaky-results
branch
from
73abde2
to
65bd937
Compare
NachoSoto
changed the title
HTTPClientTests: failing test to expose race conditionHTTPClientTests: fixed failing test with missing assertions
Codecov Report
@@ Coverage Diff @@
## main #2262 +/- ##
==========================================
+ Coverage 85.96% 85.98% +0.02%
==========================================
Files 183 183
Lines 12119 12119
==========================================
+ Hits 10418 10421 +3
+ Misses 1701 1698 -3
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
aboedo
reviewed
Feb 6, 2023
|
great catch! |
|
Updated. |
aboedo
approved these changes
Feb 6, 2023
NachoSoto
added a commit
that referenced
this pull request
Feb 6, 2023
Similarly to #2262, test execution ordering was leading to [flaky code coverage results](https://app.codecov.io/gh/RevenueCat/purchases-ios/pull/2262) with this. Because we didn't have any test coverage, only if some other test changed the `Storefront`, this test was calling the delegate code. This adds explicit test coverage using a `MockAsyncSequence`. - Added `StorefrontType` to `StoreKit2StorefrontListenerDelegate` method - `Storefront` initializers are no longer `Optional` `init?` - Simplified `StoreKit2StorefrontListenerTests` by moving `AvailabilityChecks` to `setUp`
NachoSoto
added a commit
that referenced
this pull request
Feb 8, 2023
Similarly to #2262, test execution ordering was leading to [flaky code coverage results](https://app.codecov.io/gh/RevenueCat/purchases-ios/pull/2262) with this. Because we didn't have any test coverage, only if some other test changed the `Storefront`, this test was calling the delegate code. This adds explicit test coverage using a `MockAsyncSequence`. - Added `StorefrontType` to `StoreKit2StorefrontListenerDelegate` method - `Storefront` initializers are no longer `Optional` `init?` - Simplified `StoreKit2StorefrontListenerTests` by moving `AvailabilityChecks` to `setUp`
NachoSoto
added a commit
that referenced
this pull request
Feb 8, 2023
…#2265) Similarly to #2262, test execution ordering was leading to [flaky code coverage results](https://app.codecov.io/gh/RevenueCat/purchases-ios/pull/2262) with this.  Because we didn't have any test coverage, only if some other test changed the `Storefront`, this test was calling the delegate code. This adds explicit test coverage using a `MockAsyncSequence`. ### Other changes: - Added `StorefrontType` to `StoreKit2StorefrontListenerDelegate` method - `Storefront` initializers are no longer `Optional` `init?` - Simplified `StoreKit2StorefrontListenerTests` by moving `AvailabilityChecks` to `setUp`
NachoSoto
pushed a commit
that referenced
this pull request
Feb 8, 2023
**This is an automatic release.** ### Dependency Updates * Bump fastlane-plugin-revenuecat_internal from `738f255` to `9255366` (#2264) via dependabot[bot] (@dependabot[bot]) * Update `Gemfile.lock` (#2254) via Cesar de la Vega (@vegaro) ### Other Changes * `HTTPClient`: added support for sending `X-Nonce` (#2214) via NachoSoto (@NachoSoto) * `Configuration`: added (`internal` for now) API to load public key (#2215) via NachoSoto (@NachoSoto) * Replaced `Any` uses for workaround with `Box` (#2250) via NachoSoto (@NachoSoto) * `HTTPClientTests`: fixed failing test with missing assertions (#2262) via NachoSoto (@NachoSoto) * `HTTPClientTests`: refactored tests to use `waitUntil` (#2257) via NachoSoto (@NachoSoto) * PurchaseTester: Add Receipt Inspector UI (#2249) via Andy Boedo (@aboedo) * Adds dependabot (#2259) via Cesar de la Vega (@vegaro) * `StoreKit1WrapperTests`: avoid using `Bool.random` to fix flaky code coverage (#2258) via NachoSoto (@NachoSoto) * `IntroEligibilityCalculator`: changed logic to handle products with no subscription group (#2247) via NachoSoto (@NachoSoto)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
HTTPClientTest.testRequestIsRetriedIfResponseFromETagManagerIsNilwas actually failing (the request wasn't successful), but because it had no expectations, we didn't notice until we introduced Code Coverage.As you can see in the Codecov diff:
Most of the time that code didn't have coverage (meaning we weren't testing it). The reason for why is uncovered in this PR.
The random execution order meant that sometimes (most of the time) tests like
testHandlesRealErrorConditionswere running first, which did this:That seemingly innocuous code was modifying
HTTPStubsResponse.emptySuccessResponse, defined in an extension in that file. Being used to value types nobody thought much of that, but unfortunatelyHTTPStubsResponseis a class, therefore it's a mutable type with reference semantics. Because of that, every test executed after any of these modifying the response would receive a failure, but it was going unnoticed.This PR also introduces a couple improvements to how we wait for several concurrent requests (which I thought was the issue at first).