Skip to content
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

Fix failing all-tests and retry more flaky tests #4188

Merged
merged 39 commits into from
Aug 20, 2024

Conversation

joshdholtz
Copy link
Member

@joshdholtz joshdholtz commented Aug 15, 2024

Motivation

  • Compile errors in Xcode 15 with iOS 16
  • Flaky tests

Description

iOS 16 fixes

The following things aren't available:

  • @available(visionOS)
  • .topBarTrailing (need to use .navigationBarTrailing)
  • Fix issues with Locale(identifier: "") now returning en but ONLY on iOS 16 in Xcode 15
    • iOS 17 returns the expected nil in Xcode 15
  • There was a test using #if swift that was now being run by iOS 16 tests that needed an additional AvailabilityChecks.iOS17APIAvailableOrSkipTest()

Flaky tests fixes

Using new scan_with_flaky_test_retries action from in fastlane-plugin-revenuecat_internal

  • Replaces the retry_failed_tests lane that did a similar thing
  • Removed a bunch of duplicated steps in the .circle/config.yml that needed to run the retry_failed_tests
  • scan_with_flaky_test_retries is now used in:
    • test_ios
    • test_tvos
    • test_watchos
    • backend_integration_tests

Carthage tests timing out

The carthage installation tests timed out pretty regularly. They will now use the large resource class (where everything else uses the medium)

@joshdholtz
Copy link
Member Author

@RCGitBot please test

@joshdholtz
Copy link
Member Author

@RCGitBot please test

2 similar comments
@joshdholtz
Copy link
Member Author

@RCGitBot please test

@joshdholtz
Copy link
Member Author

@RCGitBot please test

@joshdholtz joshdholtz requested a review from a team August 15, 2024 17:31
@joshdholtz joshdholtz force-pushed the fix-failing-tests-that-rcbot-does branch from 7c8b25a to 4a4d7d3 Compare August 15, 2024 17:44
@joshdholtz
Copy link
Member Author

@RCGitBot please test

6 similar comments
@joshdholtz
Copy link
Member Author

@RCGitBot please test

@joshdholtz
Copy link
Member Author

@RCGitBot please test

@joshdholtz
Copy link
Member Author

@RCGitBot please test

@joshdholtz
Copy link
Member Author

@RCGitBot please test

@joshdholtz
Copy link
Member Author

@RCGitBot please test

@joshdholtz
Copy link
Member Author

@RCGitBot please test

@joshdholtz joshdholtz force-pushed the fix-failing-tests-that-rcbot-does branch from 2f57fa9 to 0ebfd4b Compare August 16, 2024 16:31
@joshdholtz
Copy link
Member Author

@RCGitBot please test

3 similar comments
@joshdholtz
Copy link
Member Author

@RCGitBot please test

@joshdholtz
Copy link
Member Author

@RCGitBot please test

@joshdholtz
Copy link
Member Author

@RCGitBot please test

@joshdholtz joshdholtz changed the title Fix failing tests Fix failing all-tests Aug 16, 2024
@joshdholtz
Copy link
Member Author

@RCGitBot please test

@aboedo
Copy link
Member

aboedo commented Aug 16, 2024

looks great so far!

@joshdholtz
Copy link
Member Author

@RCGitBot please test

@joshdholtz joshdholtz changed the title Fix failing all-tests Fix failing all-tests and retry more flaky tests Aug 18, 2024
@joshdholtz joshdholtz marked this pull request as ready for review August 18, 2024 19:43
@joshdholtz
Copy link
Member Author

@RCGitBot please test

@joshdholtz
Copy link
Member Author

@RCGitBot please test

@joshdholtz
Copy link
Member Author

@RCGitBot please test

@joshdholtz
Copy link
Member Author

@RCGitBot please test

Copy link
Contributor

@jamesrb1 jamesrb1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Just two questions.

@@ -1058,7 +1047,7 @@ jobs:

installation-tests-carthage:
executor:
name: macos-executor
name: macos-executor-large
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this still needed after #4184? I checked that it was included in this PR, but these were so close together, maybe it's not needed now?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was already included! I rebased pretty often 😁 It was definitely faster but I think that medium executor that was being used just sometimes wasn't powerful/large enough and carthage will timeout when trying to find/use the xcode project 🫠

// and iOS 15 tests with Xcode 14
#if swift(>=5.9)
if #available(iOS 17.0, macOS 14.0, tvOS 17.0, watchOS 10.0, *) {
// Fixed in iOS 16 on Xcode 15
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this comment supposed to be iOS 17? (Ditto in testRemovingRegion() below.)

Suggested change
// Fixed in iOS 16 on Xcode 15
// Fixed in iOS 17 on Xcode 15

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It definitely is 😅

@jamesrb1
Copy link
Contributor

Looking forward to not clicking these buttons :)

image

@joshdholtz
Copy link
Member Author

@RCGitBot please test

@joshdholtz joshdholtz merged commit 26eed6c into main Aug 20, 2024
33 checks passed
@joshdholtz joshdholtz deleted the fix-failing-tests-that-rcbot-does branch August 20, 2024 12:44
joshdholtz added a commit that referenced this pull request Aug 20, 2024
⚠️ Waiting for #4188 to be merged first (because needs all the tests to
pass)

## Motivation

Fix cases where paywall was showing multiple languages at once and
random languages.

## Description

👉 For a demo of the problem and solution, look in Slack in the paywalls
or SDK channel for the latest Loom that I posted

### Flow of paywall localization logic (since it can be confusing)

1.
[PaywallView](https://github.com/RevenueCat/purchases-ios/pull/4158/files#diff-ea75fe7280803e15b975d702a16c6851ffc2bbed01b7b196bf0fb80d75b4b332)
starts with `Locale.current` but then calls
`offering.validatedPaywall()` which will return the new
`displayedLocale: Locale` variable
2.
[PaywallData+Validation](https://github.com/RevenueCat/purchases-ios/pull/4158/files#diff-452790cf5287a44f75a4b2260254db2281a066faeea2b3e441a007ec60522b6b)
returns `displayedLocale` if its a valid paywall, otherwise it return
`Locale.current` for fallback paywall
3.
[PaywallData+Localization](https://github.com/RevenueCat/purchases-ios/pull/4158/files#diff-e620baead94f7acad56d931c485d7691c4f909be3fe625e104528a2702cca2a0)
has a lot of the new logic
- New `locale: Locale` computer property that it determines the locale
from the `localizedConfiguration` or `localizedConfigurationsByTier`
- New ` defaultLocalizedConfiguration` that uses the developer set
`defaultLocale` (from `PaywallData`) to find the default localization
- Updated `localizedConfiguration<Value>()` to take a new
`defaultLocalization` to put in its priority (and log it if its used).
If for some reason that's not found, it will use the legacy
`fallbackLocalization` (which is the first localization it finds or
english)
4. After all of that, the `displayedLocale` is passed into the actually
SwiftUI `LoadedOfferingPaywallView` to show this locale everywhere

### Problem 1 - Inconsistent languages displayed at once

The user defined localizations and the pre-localized content in the SDK
(like links in the footer and package duration names) could should
different localizations. The choice of which locale was happening in two
different spots.

Since the SDK has all possible localizations that the dashboard offers,
the locale SDK displays from the dashboard will be chosen for the
pre-localized content.

### Problem 2 - Showing random languages if not supported

We decided early on to not use a `default_locale` but that has hurt us.
The solution to use was use `localizations.first` to show the first
locale in the list and the intention that this was the first locale the
user did in the paywall. That was not the case, however. The order of
the localizations list was indeterministic so it should should a random
language (even to the point where different random languages would show
the the same user).

We are now introducing a `default_locale` that the developer can set in
the paywall. If the locale the user is requesting is not translated by
the developer, it will use the `default_locale`

### Problem 3 - Footer not using correct localization from the bundle

There were times where some parts of the footer itself (mainly the
restore button) would be a different language than the terms of service
and privacy policy buttons. This was due to a code issue where we were
using `Bundle.module`. Instead, we needed to specify a localization
bundle specifically that we wanted to use.
joshdholtz added a commit that referenced this pull request Aug 22, 2024
**This is an automatic release.**

### New Features
* Price rounding logic (#4132) via James Borthwick (@jamesrb1)
### Bugfixes
* [Customer Center] Migrate to List style (#4190) via Cody Kerns
(@codykerns)
* [Paywalls] Improve locale consistency (#4158) via Josh Holtz
(@joshdholtz)
* Set Paywalls Tester deployment target to iOS 15 (#4196) via James
Borthwick (@jamesrb1)
* [Customer Center] Hide Contact Support button if URL can't be created
(#4192) via Cesar de la Vega (@vegaro)
* Fix the setting for SKIP_INSTALL in Xcode project (#4195) via Andy
Boedo (@aboedo)
* [Customer Center] Improving customer center buttons (#4165) via Cody
Kerns (@codykerns)
* Revert workaround for iOS 18 beta 5 SwiftUI crash (#4173) via Mark
Villacampa (@MarkVillacampa)
* [Paywalls] Make iOS version calculation lazy (#4163) via Mark
Villacampa (@MarkVillacampa)
* [Paywalls] Observe `PurchaseHandler` when owned externally (#4097) via
James Borthwick (@jamesrb1)
### Other Changes
* [Customer Center] Clean up colors in WrongPlatformView and
NoSubscriptionsView (#4204) via Cesar de la Vega (@vegaro)
* Fix failing `all-tests` and retry more flaky tests (#4188) via Josh
Holtz (@joshdholtz)
* Compatibility content unavailable improvements (#4197) via James
Borthwick (@jamesrb1)
* Create lane to enable customer center (#4191) via Cesar de la Vega
(@vegaro)
* XCFramework artifacts in CircleCI (#4189) via Andy Boedo (@aboedo)
* [Customer Center] CustomerCenterViewModel checks whether the app is
the latest version (#4169) via JayShortway (@JayShortway)
* export RevenueCatUI xcframework (#4172) via Andy Boedo (@aboedo)
* Corrects references from ManageSubscriptionsButtonStyle to
ButtonsStyle. (#4186) via JayShortway (@JayShortway)
* Speed up carthage installation tests (#4184) via Andy Boedo (@aboedo)
* Customer center improvements (#4166) via James Borthwick (@jamesrb1)
* replace `color(from colorInformation:)` global with extension (#4183)
via Andy Boedo (@aboedo)
* Fix tests in main (#4174) via Andy Boedo (@aboedo)
* Enable customer center tests (#4171) via James Borthwick (@jamesrb1)
* [Customer Center] Initial implementation (#3967) via Cesar de la Vega
(@vegaro)

---------

Co-authored-by: Josh Holtz <me@joshholtz.com>
nyeu pushed a commit that referenced this pull request Oct 2, 2024
## Motivation

- Compile errors in Xcode 15 with iOS 16
- Flaky tests

## Description

### iOS 16 fixes

The following things aren't available:

- `@available(visionOS)` 
- `.topBarTrailing` (need to use `.navigationBarTrailing`)
- Fix issues with `Locale(identifier: "")` now returning `en` but ONLY
on iOS 16 in Xcode 15
  - iOS 17 returns the expected `nil` in Xcode 15
- There was a test using `#if swift` that was now being run by iOS 16
tests that needed an additional
`AvailabilityChecks.iOS17APIAvailableOrSkipTest()`

### Flaky tests fixes

Using new `scan_with_flaky_test_retries` action from in
`fastlane-plugin-revenuecat_internal`

- Replaces the `retry_failed_tests` lane that did a similar thing
- Removed a bunch of duplicated steps in the `.circle/config.yml` that
needed to run the `retry_failed_tests`
- `scan_with_flaky_test_retries` is now used in:
  - `test_ios`
  - `test_tvos`
  - `test_watchos`
  - `backend_integration_tests`

### Carthage tests timing out

The carthage installation tests timed out pretty regularly. They will
now use the large resource class (where everything else uses the medium)
nyeu pushed a commit that referenced this pull request Oct 2, 2024
⚠️ Waiting for #4188 to be merged first (because needs all the tests to
pass)

## Motivation

Fix cases where paywall was showing multiple languages at once and
random languages.

## Description

👉 For a demo of the problem and solution, look in Slack in the paywalls
or SDK channel for the latest Loom that I posted

### Flow of paywall localization logic (since it can be confusing)

1.
[PaywallView](https://github.com/RevenueCat/purchases-ios/pull/4158/files#diff-ea75fe7280803e15b975d702a16c6851ffc2bbed01b7b196bf0fb80d75b4b332)
starts with `Locale.current` but then calls
`offering.validatedPaywall()` which will return the new
`displayedLocale: Locale` variable
2.
[PaywallData+Validation](https://github.com/RevenueCat/purchases-ios/pull/4158/files#diff-452790cf5287a44f75a4b2260254db2281a066faeea2b3e441a007ec60522b6b)
returns `displayedLocale` if its a valid paywall, otherwise it return
`Locale.current` for fallback paywall
3.
[PaywallData+Localization](https://github.com/RevenueCat/purchases-ios/pull/4158/files#diff-e620baead94f7acad56d931c485d7691c4f909be3fe625e104528a2702cca2a0)
has a lot of the new logic
- New `locale: Locale` computer property that it determines the locale
from the `localizedConfiguration` or `localizedConfigurationsByTier`
- New ` defaultLocalizedConfiguration` that uses the developer set
`defaultLocale` (from `PaywallData`) to find the default localization
- Updated `localizedConfiguration<Value>()` to take a new
`defaultLocalization` to put in its priority (and log it if its used).
If for some reason that's not found, it will use the legacy
`fallbackLocalization` (which is the first localization it finds or
english)
4. After all of that, the `displayedLocale` is passed into the actually
SwiftUI `LoadedOfferingPaywallView` to show this locale everywhere

### Problem 1 - Inconsistent languages displayed at once

The user defined localizations and the pre-localized content in the SDK
(like links in the footer and package duration names) could should
different localizations. The choice of which locale was happening in two
different spots.

Since the SDK has all possible localizations that the dashboard offers,
the locale SDK displays from the dashboard will be chosen for the
pre-localized content.

### Problem 2 - Showing random languages if not supported

We decided early on to not use a `default_locale` but that has hurt us.
The solution to use was use `localizations.first` to show the first
locale in the list and the intention that this was the first locale the
user did in the paywall. That was not the case, however. The order of
the localizations list was indeterministic so it should should a random
language (even to the point where different random languages would show
the the same user).

We are now introducing a `default_locale` that the developer can set in
the paywall. If the locale the user is requesting is not translated by
the developer, it will use the `default_locale`

### Problem 3 - Footer not using correct localization from the bundle

There were times where some parts of the footer itself (mainly the
restore button) would be a different language than the terms of service
and privacy policy buttons. This was due to a code issue where we were
using `Bundle.module`. Instead, we needed to specify a localization
bundle specifically that we wanted to use.
nyeu pushed a commit that referenced this pull request Oct 2, 2024
**This is an automatic release.**

### New Features
* Price rounding logic (#4132) via James Borthwick (@jamesrb1)
### Bugfixes
* [Customer Center] Migrate to List style (#4190) via Cody Kerns
(@codykerns)
* [Paywalls] Improve locale consistency (#4158) via Josh Holtz
(@joshdholtz)
* Set Paywalls Tester deployment target to iOS 15 (#4196) via James
Borthwick (@jamesrb1)
* [Customer Center] Hide Contact Support button if URL can't be created
(#4192) via Cesar de la Vega (@vegaro)
* Fix the setting for SKIP_INSTALL in Xcode project (#4195) via Andy
Boedo (@aboedo)
* [Customer Center] Improving customer center buttons (#4165) via Cody
Kerns (@codykerns)
* Revert workaround for iOS 18 beta 5 SwiftUI crash (#4173) via Mark
Villacampa (@MarkVillacampa)
* [Paywalls] Make iOS version calculation lazy (#4163) via Mark
Villacampa (@MarkVillacampa)
* [Paywalls] Observe `PurchaseHandler` when owned externally (#4097) via
James Borthwick (@jamesrb1)
### Other Changes
* [Customer Center] Clean up colors in WrongPlatformView and
NoSubscriptionsView (#4204) via Cesar de la Vega (@vegaro)
* Fix failing `all-tests` and retry more flaky tests (#4188) via Josh
Holtz (@joshdholtz)
* Compatibility content unavailable improvements (#4197) via James
Borthwick (@jamesrb1)
* Create lane to enable customer center (#4191) via Cesar de la Vega
(@vegaro)
* XCFramework artifacts in CircleCI (#4189) via Andy Boedo (@aboedo)
* [Customer Center] CustomerCenterViewModel checks whether the app is
the latest version (#4169) via JayShortway (@JayShortway)
* export RevenueCatUI xcframework (#4172) via Andy Boedo (@aboedo)
* Corrects references from ManageSubscriptionsButtonStyle to
ButtonsStyle. (#4186) via JayShortway (@JayShortway)
* Speed up carthage installation tests (#4184) via Andy Boedo (@aboedo)
* Customer center improvements (#4166) via James Borthwick (@jamesrb1)
* replace `color(from colorInformation:)` global with extension (#4183)
via Andy Boedo (@aboedo)
* Fix tests in main (#4174) via Andy Boedo (@aboedo)
* Enable customer center tests (#4171) via James Borthwick (@jamesrb1)
* [Customer Center] Initial implementation (#3967) via Cesar de la Vega
(@vegaro)

---------

Co-authored-by: Josh Holtz <me@joshholtz.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants