Support all platforms in the test targets #717

Juanpe · 2021-08-06T15:32:30Z

Resolves #652

The goal of this PR is to enable all platforms in the test targets.

Juanpe · 2021-08-06T15:33:34Z

Purchases/Attribution/RCAttributionFetcher.m

@@ -70,14 +70,16 @@ - (nullable NSString *)identifierForVendor {
 - (void)adClientAttributionDetailsWithCompletionBlock:(RCAttributionDetailsBlock)completionHandler {
    // Should match available platforms in
    // https://developer.apple.com/documentation/iad/adclient?language=objc
-    #if TARGET_OS_IOS
+    #if TARGET_OS_IOS || TARGET_OS_MACCATALYST


I've added Mac Catalyst, but I didn't remove the comment. I think is useful until this class will be migrated to Swift in order to check again the available platforms.

I saw this after the class had already been merged in #711 😬
I think we had it like this originally because if TARGET_OS_MACCATALYST, then TARGET_OS_IOS also evaluates to true? I'd have to double check.
I think being explicit is probably a good idea in any case

hehe, no problem, I'll update my branch 👍🏼 Swift code is always welcome 🙂

Well, you're right, it's a double check. IMO, since Mac Catalyst is not so popular it wouldn't make sense to be explicit. So, it's ok for me to use only iOS

Juanpe · 2021-08-06T15:37:38Z

Purchases/Attribution/RCAttributionFetcher.m

    RCAdClientProxy * _Nullable adClientProxy = [self.attributionFactory adClientProxy];
    if (!adClientProxy) {
        [RCLog warn:[NSString stringWithFormat:@"%@",
                     RCStrings.attribution.search_ads_attribution_cancelled_missing_iad_framework]];
        return;
    }
    [adClientProxy requestAttributionDetailsWithBlock:completionHandler];
+    #else
+    completionHandler([NSDictionary new], nil);
    #endif


Well, I added this else block because I think if the code is not executed in iOS, this closure never will be released. Maybe I'm wrong... 🤔

you're not wrong, I noticed that too when migrating AttributionFetcher 😬

Juanpe · 2021-08-06T15:38:13Z

PurchasesCoreSwiftTests/Mocks/MockSKDiscount.swift

@@ -23,10 +23,8 @@ class MockDiscount: SKProductDiscount {
        return mockIdentifier ?? "identifier"
    }

-    @available(iOS 11.2, *)


redundant availability information

Juanpe · 2021-08-06T15:41:44Z

PurchasesCoreSwiftTests/Purchasing/ProductInfoTests.swift

-            expect(receivedOffers[2]["price"] as? NSDecimalNumber) == discount3.price
-            expect((receivedOffers[2]["payment_mode"] as? NSNumber)?.intValue) == discount3.paymentMode.rawValue
-        }
+        let discount1 = PromotionalOffer(offerIdentifier: "offerid1",


the test is the same, I've only changed how the discounts are initialized. The constructor with a product discount as param is not available for all OS versions, but there is another compatible one. Since the objective of this test case is not to test if the constructor works fine, I think we can use the most compatible constructor. WDYT?

agreed, I think this was probably an oversight.

Juanpe · 2021-08-06T15:42:01Z

PurchasesCoreSwiftTests/Purchasing/ProductInfoTests.swift

    func testCacheKey() {
-        if #available(macOS 10.14.4, *) {


same here. ⬆️

Juanpe · 2021-08-06T15:42:27Z

PurchasesCoreSwiftTests/Purchasing/StoreKitWrapperTests.swift

@@ -8,7 +8,6 @@

 import Foundation
 import XCTest
-import OHHTTPStubs


useless 🗑️

Juanpe · 2021-08-06T15:42:37Z

PurchasesCoreSwiftTests/SubscriberAttributes/SubscriberAttributeTests.swift

@@ -4,7 +4,6 @@
 //

 import XCTest
-import OHHTTPStubs


useless 🗑️

Juanpe · 2021-08-06T15:43:40Z

PurchasesTests/Attribution/AttributionFetcherTests.swift

@@ -161,7 +164,8 @@ class AttributionFetcherTests: XCTestCase {
    }

    func testPostAppleSearchAdsAttributionIfNeededSkipsIfATTFrameworkNotIncludedOnNewOS() {
-        if #available(iOS 14, macOS 11, tvOS 14, *) {
+        #if os(iOS)


Since the AdClient class is only available for iOS, it doesn't make sense to launch these tests

Juanpe · 2021-08-06T15:43:51Z

PurchasesTests/FoundationExtensions/NSError+RCExtensionsTests.swift

@@ -4,7 +4,6 @@
 //

 import XCTest
-import OHHTTPStubs


useless 🗑️

Juanpe · 2021-08-06T15:50:37Z

PurchasesTests/Networking/HTTPClientTests.swift

@@ -6,6 +6,8 @@
 //  Copyright © 2019 RevenueCat. All rights reserved.
 //

+#if !os(watchOS)


Ok, here there is an x-files 😅 I've disabled all these test cases for watchOS. I'm not sure what it's happening, but all the tests with POST requests fail. It seems that the stub matcher doesn't recognize the request when is sent. But the same code works fine for the rest of the platforms 🕺🏼

TBH, I'm not sure that the OHHTTPStubs supports correctly watchOS. As you can see, there is an issue with the same behavior and there is a WIP PR to support watchOS. Maybe I'm missing something 🤔

I've asked to OHHTTPStubs' maintainers the status of that task ...

let's add a comment to explain that

Perfect! Anyway, I'm going to investigate a little bit more in order to know what it's happening 🤔

aboedo

@Juanpe thanks so much for doing this!! I'm currently getting a crash when running on watchOS simulators, on throwAssertion. Does it work correctly for you?

aboedo · 2021-08-06T18:49:03Z

Purchases/Attribution/RCAttributionFetcher.m

@@ -70,14 +70,16 @@ - (nullable NSString *)identifierForVendor {
 - (void)adClientAttributionDetailsWithCompletionBlock:(RCAttributionDetailsBlock)completionHandler {
    // Should match available platforms in
    // https://developer.apple.com/documentation/iad/adclient?language=objc
-    #if TARGET_OS_IOS
+    #if TARGET_OS_IOS || TARGET_OS_MACCATALYST


I saw this after the class had already been merged in #711 😬
I think we had it like this originally because if TARGET_OS_MACCATALYST, then TARGET_OS_IOS also evaluates to true? I'd have to double check.
I think being explicit is probably a good idea in any case

aboedo · 2021-08-06T18:49:20Z

Purchases/Attribution/RCAttributionFetcher.m

    RCAdClientProxy * _Nullable adClientProxy = [self.attributionFactory adClientProxy];
    if (!adClientProxy) {
        [RCLog warn:[NSString stringWithFormat:@"%@",
                     RCStrings.attribution.search_ads_attribution_cancelled_missing_iad_framework]];
        return;
    }
    [adClientProxy requestAttributionDetailsWithBlock:completionHandler];
+    #else
+    completionHandler([NSDictionary new], nil);
    #endif


you're not wrong, I noticed that too when migrating AttributionFetcher 😬

aboedo · 2021-08-06T18:50:37Z

PurchasesCoreSwiftTests/Mocks/MockProductsManager.swift

-                if #available(iOS 12.2, *) {
+                if #available(iOS 12.2, tvOS 11.2, macOS 10.13.2, *) {


I think we might even wanna update the check for iOS - SKProductDiscount is available starting on 11.2

good catch! 👌🏼

aboedo · 2021-08-06T18:51:17Z

PurchasesCoreSwiftTests/Mocks/MockSKProduct.swift

+    @available(iOS 12.2, tvOS 11.2, macOS 10.13.2, *)
    lazy var mockDiscount: SKProductDiscount? = nil

-    @available(iOS 12.2, *)
+    @available(iOS 12.2, tvOS 11.2, macOS 10.13.2, *)


same note here, I think we missed the mark with the availability checks for iOS 11.2

aboedo · 2021-08-06T18:52:11Z

PurchasesCoreSwiftTests/Purchasing/ProductInfoTests.swift

-            expect(receivedOffers[2]["price"] as? NSDecimalNumber) == discount3.price
-            expect((receivedOffers[2]["payment_mode"] as? NSNumber)?.intValue) == discount3.paymentMode.rawValue
-        }
+        let discount1 = PromotionalOffer(offerIdentifier: "offerid1",


agreed, I think this was probably an oversight.

aboedo · 2021-08-06T19:24:07Z

PurchasesTests/Mocks/MockAttributionTypeFactory.swift

+#if canImport(AppTrackingTransparency)
+  import AppTrackingTransparency
+#endif


aboedo · 2021-08-06T19:25:10Z

PurchasesTests/Networking/HTTPClientTests.swift

@@ -6,6 +6,8 @@
 //  Copyright © 2019 RevenueCat. All rights reserved.
 //

+#if !os(watchOS)


let's add a comment to explain that

…gets # Conflicts: # Purchases/Attribution/RCAttributionFetcher.m

Juanpe · 2021-08-06T20:32:57Z

@Juanpe thanks so much for doing this!! I'm currently getting a crash when running on watchOS simulators, on throwAssertion. Does it work correctly for you?

I've checked it and if I run the tests by fastlane I get this crash but if I run the tests by Xcode, I got a green. Let me check it, maybe it's related to the tests config.

Juanpe · 2021-08-09T10:35:37Z

Thanks for the code review and your feedback 🤙 I've updated the PR with the suggested changes.

I investigated the problem related to the HTTPClient, and the problem is that they are using the URLProtocol to intercept the requests, and all the POST requests don't call one of the protocol methods and the request is not sent. I couldn't found a reason yet 🧐

But the main problem is that Nimble doesn't support watchOS yet 😕

Nimble.podspec
.
I've opened an issue (Build error for watchOS target. BadInstructionException) because they had the same problem with tvOS and they finally supported it.

That said, I suggest that the tests shouldn't support watchOS for now. However, I would create a blocked issue to run tests in watchOS when Nimble supports it.

WDYT?

…gets

aboedo · 2021-08-09T21:28:42Z

I suggest that the tests shouldn't support watchOS for now. However, I would create a blocked issue to run tests in watchOS when Nimble supports it.
WDYT?

@Juanpe 💯💯💯

…gets

Juanpe · 2022-04-15T09:15:15Z

That said, I suggest that the tests shouldn't support watchOS for now. However, I would create a blocked issue to run tests in watchOS when Nimble supports it.

Hi guys 👋🏼 ! Just to inform you, Nimble looks like it will add watchOS support in the next release. 👏🏼

NachoSoto · 2022-04-15T10:15:39Z

Awesome!

This is a follow up to #717. In order to be able to reproduce #2338, which seems to be an issue only on `watchOS`, it's important that we now run tests on `watchOS` as well. ## Changes: - Added CircleCI job - Added lane - Simplified `UnitTestsHostApp` definition using `SwiftUI` - Increased _test only_ watchOS deployment target to 7.0 (this was necessary to be able to have a comm on `@main` for the host app - Fixed test compilation on `watchOS` - Disabled `HTTPClient` tests on `watchOS` (see AliSoftware/OHHTTPStubs#287) - `StoreKitConfigTestCase` is only available from `watchOS 7.0` since that's when `SKTestSession` was introduced - Added `watchOS` support to `UnitTests` target - Added `watchOS` support to `ReceiptParserTests` target - Fixed `isFamilySharable` `watchOS` version checks for version 8.0 - Created new `CI-RevenueCat` Test Plan to only run unit tests and not StoreKit tests on `watchOS`

This is a follow up to #717. In order to be able to reproduce #2338, which seems to be an issue only on `watchOS`, it's important that we now run tests on `watchOS` as well. We probably won't be able to fully replicate that issue since we don't run tests on actual devices, so they'll still run on 64 bits. But this way we might be able to prevent issues that are only reproducible on `watchOS` in the future. ## Changes: - Added CircleCI job - Added lane - Simplified `UnitTestsHostApp` definition using `SwiftUI` - Increased _test only_ `watchOS` deployment target to 7.0 (this was necessary to be able to have a common `@main` for the host app) - Increased _test only_ `tvOS` deployment target to `14.0` (same as above, plus we only run tvOS tests on the latest version anyway. We test backwards compatibility through iOS already) - Fixed test compilation on `watchOS` - Disabled `HTTPClient` tests on `watchOS` (see AliSoftware/OHHTTPStubs#287) - `StoreKitConfigTestCase` is only available from `watchOS 7.0` since that's when `SKTestSession` was introduced - Added `watchOS` support to `UnitTests` target - Added `watchOS` support to `ReceiptParserTests` target - Fixed `isFamilySharable` `watchOS` version checks for version 8.0 - Created new `CI-RevenueCat` Test Plan to only run unit tests and not StoreKit tests on `watchOS`

This is a follow up to #717. In order to be able to reproduce #2338, which seems to be an issue only on `watchOS`, it's important that we now run tests on `watchOS` as well. We probably won't be able to fully replicate that issue since we don't run tests on actual devices, so they'll still run on 64 bits. But this way we might be able to prevent issues that are only reproducible on `watchOS` in the future. - Added CircleCI job - Added lane - Simplified `UnitTestsHostApp` definition using `SwiftUI` - Increased _test only_ `watchOS` deployment target to 7.0 (this was necessary to be able to have a common `@main` for the host app) - Increased _test only_ `tvOS` deployment target to `14.0` (same as above, plus we only run tvOS tests on the latest version anyway. We test backwards compatibility through iOS already) - Fixed test compilation on `watchOS` - Disabled `HTTPClient` tests on `watchOS` (see AliSoftware/OHHTTPStubs#287) - `StoreKitConfigTestCase` is only available from `watchOS 7.0` since that's when `SKTestSession` was introduced - Added `watchOS` support to `UnitTests` target - Added `watchOS` support to `ReceiptParserTests` target - Fixed `isFamilySharable` `watchOS` version checks for version 8.0 - Created new `CI-RevenueCat` Test Plan to only run unit tests and not StoreKit tests on `watchOS`

enable all platforms in test targets

3252a91

Juanpe commented Aug 6, 2021

View reviewed changes

aboedo reviewed Aug 6, 2021

View reviewed changes

Merge branch 'swift_migration' into support_all_platforms_in_test_tar…

203666e

…gets # Conflicts: # Purchases/Attribution/RCAttributionFetcher.m

update after comments

9830d77

Merge branch 'swift_migration' into support_all_platforms_in_test_tar…

35f1534

…gets

Juanpe added 2 commits August 10, 2021 07:40

Merge branch 'swift_migration' into support_all_platforms_in_test_tar…

0ea634f

…gets

revert support watchOS in test targets

6b48e14

Juanpe requested a review from aboedo August 10, 2021 06:16

aboedo approved these changes Aug 10, 2021

View reviewed changes

aboedo merged commit 5b16e79 into RevenueCat:swift_migration Aug 10, 2021

Juanpe deleted the support_all_platforms_in_test_targets branch August 10, 2021 18:14

NachoSoto mentioned this pull request Mar 16, 2023

CI: also run tests on watchOS #2340

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support all platforms in the test targets #717

Support all platforms in the test targets #717

Juanpe commented Aug 6, 2021

Juanpe Aug 6, 2021

aboedo Aug 6, 2021

Juanpe Aug 6, 2021

Juanpe Aug 6, 2021

aboedo Aug 6, 2021

Juanpe Aug 6, 2021

Juanpe Aug 6, 2021

aboedo Aug 6, 2021

Juanpe Aug 6, 2021

Juanpe Aug 6, 2021

Juanpe Aug 6, 2021

Juanpe Aug 6, 2021

Juanpe Aug 6, 2021

Juanpe Aug 6, 2021 •

edited

Loading

aboedo Aug 6, 2021

Juanpe Aug 6, 2021

aboedo left a comment

aboedo Aug 6, 2021

aboedo Aug 6, 2021

aboedo Aug 6, 2021

Juanpe Aug 6, 2021

aboedo Aug 6, 2021

aboedo Aug 6, 2021

aboedo Aug 6, 2021

aboedo Aug 6, 2021

Juanpe commented Aug 6, 2021

Juanpe commented Aug 9, 2021 •

edited

Loading

aboedo commented Aug 9, 2021 •

edited

Loading

Juanpe commented Apr 15, 2022

NachoSoto commented Apr 15, 2022

		if #available(iOS 12.2, *) {
		if #available(iOS 12.2, tvOS 11.2, macOS 10.13.2, *) {

Support all platforms in the test targets #717

Support all platforms in the test targets #717

Conversation

Juanpe commented Aug 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Juanpe Aug 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aboedo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Juanpe commented Aug 6, 2021

Juanpe commented Aug 9, 2021 • edited Loading

aboedo commented Aug 9, 2021 • edited Loading

Juanpe commented Apr 15, 2022

NachoSoto commented Apr 15, 2022

Juanpe Aug 6, 2021 •

edited

Loading

Juanpe commented Aug 9, 2021 •

edited

Loading

aboedo commented Aug 9, 2021 •

edited

Loading