CacheableNetworkOperation: fixed race condition in new test#1932
Conversation
66a14d6 to
c076800
Compare
|
Huh this is still failing in iOS 14 😶 |
d3ef6d5 to
4c4960e
Compare
|
For some reason debug logs aren't enabled when running the iOS 15 tests, but can't figure out why. We're compiling on debug, and |
edaa387 to
b50f56f
Compare
|
Tests are still failing. I need to figure out why on iOS 15 the |
|
In the mean time it would be nice to merge #1928 so we're running tests in the device we actually want them too. |
033f471 to
f49d789
Compare
b1f9ccc to
e16064f
Compare
There was a problem hiding this comment.
- It's testing a deprecated method
- It's modifying global state, which interferes with every other test. We run tests in parallel, so this is making the new test that requires debug logs to fail randomly, if this test happened to run before (it was changing the level back to
.info).
There was a problem hiding this comment.
is there any way to
- back up the previous value
- run this atomically
- return the previous value?
There was a problem hiding this comment.
like, deprecated or not, if we break this, it's still a problem
There was a problem hiding this comment.
run this atomically
We would have to block the entire Logger from logging anything, and / or block all other tests from running. I can't think of a way to do that.
There was a problem hiding this comment.
like, deprecated or not, if we break this, it's still a problem
For sure. I just can't think of a way to test this global state without messing with every other test.
There was a problem hiding this comment.
Do you have any ideas? :/
There was a problem hiding this comment.
Fastlane's scan documents that Release is the default. However, if we don't pass the value, scan wasn't passing any configuration to xcodebuild, so it was relying on the scheme's default, which is Debug.
Instead of relying on that, which might change in the future, this makes the configuration explicit.
|
Can I get another review of this? |
There was a problem hiding this comment.
is there any way to
- back up the previous value
- run this atomically
- return the previous value?
The test introduced by #1926 passed locally, but sometimes failed in CI due to a race condition. What we're testing is that the second request reuses the callback of the first one, **if called while the first one is still in progress**. However, nothing in the test ensures that's the case. This introduces an optional delay in `MockHTTPClient` so we can ensure that. _Note: because of the nature of these 2 tests, they don't actually wait for the whole response to finish, so they still pass in ~0.006s_
e16064f to
0a02104
Compare
**This is an automatic release.** ### New Features * 🚨 `StoreKit 2` is now enabled by default 🚨 (#1922) via NachoSoto (@NachoSoto) * Extracted `PurchasesType` and `PurchasesSwiftType` (#1912) via NachoSoto (@NachoSoto) ### Bugfixes * `StoreKit 1`: changed result of cancelled purchases to be consistent with `StoreKit 2` (#1910) via NachoSoto (@NachoSoto) * `PaymentQueueWrapper`: handle promotional purchase requests from App Store when SK1 is disabled (#1901) via NachoSoto (@NachoSoto) ### Other Changes * Fixed iOS 12 tests (#1936) via NachoSoto (@NachoSoto) * `CacheableNetworkOperation`: fixed race condition in new test (#1932) via NachoSoto (@NachoSoto) * `BasePurchasesTests`: changed default back to SK1 (#1935) via NachoSoto (@NachoSoto) * `Logger`: refactored default `LogLevel` definition (#1934) via NachoSoto (@NachoSoto) * `AppleReceipt`: refactored declarations into nested types (#1933) via NachoSoto (@NachoSoto) * `Integration Tests`: relaunch tests when retrying failures (#1925) via NachoSoto (@NachoSoto) * `CircleCI`: downgraded release jobs to Xcode 13.x (#1927) via NachoSoto (@NachoSoto) * `ErrorUtils`: added test to verify that `PublicError`s can be `catch`'d as `ErrorCode` (#1924) via NachoSoto (@NachoSoto) * `StoreKitIntegrationTests`: print `AppleReceipt` data whenever `verifyEntitlementWentThrough` fails (#1929) via NachoSoto (@NachoSoto) * `OperationQueue`: log debug message when requests are found in cache and skipped (#1926) via NachoSoto (@NachoSoto) * `GetCustomerInfoAPI`: avoid making a request if there's any `PostReceiptDataOperation` in progress (#1911) via NachoSoto (@NachoSoto) * `PurchaseTester`: allow HTTP requests and enable setting `ProxyURL` (#1917) via NachoSoto (@NachoSoto)
The test introduced by #1926 passed locally, but sometimes failed in CI due to a race condition.
What we're testing is that the second request reuses the callback of the first one, if called while the first one is still in progress. However, nothing in the test ensures that's the case.
This introduces an optional delay in
MockHTTPClientso we can ensure that.Another reason that test was failing randomly is because we had another test modifying global state, which interferes with every other test. We run tests in parallel, so it was making the new test that requires
debuglogs to fail randomly, if this test happened to run before.Note: because of the nature of these 2 tests, they don't actually wait for the whole response to finish, so they still pass in ~0.006s