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

Paywalls: add test coverage for locales with different region #1600

Merged
merged 2 commits into from
Feb 6, 2024

Conversation

NachoSoto
Copy link
Contributor

Test coverage to ensure that Android isn't affected by RevenueCat/purchases-ios#3633.

I've also extracted getDefaultLocales and localizedConfiguration(List<Locale>) so we can test those.

Test coverage to ensure that Android isn't affected by RevenueCat/purchases-ios#3633.
I've also extracted `getDefaultLocales` and  `localizedConfiguration(List<Locale>)` so we can test those.
@NachoSoto NachoSoto added the test label Feb 5, 2024
@NachoSoto NachoSoto requested a review from a team February 5, 2024 23:19
Copy link

codecov bot commented Feb 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (14704ff) 83.71% compared to head (f7ed296) 83.76%.
Report is 4 commits behind head on main.

❗ Current head f7ed296 differs from pull request most recent head 14c62d8. Consider uploading reports for the commit 14c62d8 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1600      +/-   ##
==========================================
+ Coverage   83.71%   83.76%   +0.04%     
==========================================
  Files         218      218              
  Lines        7260     7263       +3     
  Branches     1011     1011              
==========================================
+ Hits         6078     6084       +6     
+ Misses        788      787       -1     
+ Partials      394      392       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@tonidero tonidero left a comment

Choose a reason for hiding this comment

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

Just a question but looks good!

fun `getDefaultLocales returns the correct list`() {
assertThat(
getDefaultLocales().map { it.toString() }
).isEqualTo(listOf("en_US"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm if I'm not wrong, this test will only pass on devices where the default locale is en_US?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. Do you have any suggestions that wouldn't lead to false positives (like overriding the locale ourselves).

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm difficult to do without mocking, which would lose the purpose of the test... Honestly the value of the test is limited since the unit tests don't run in an android image but on the JVM virtual machine (in some cases, there are some differences)

Having said that, I think we can either keep it as is, even if it might fail in some devices, or just return early if the locale is not Locale.US so we don't test in those devices. Feels like the first option might be safer but we should add a comment so when it fails, we are aware of this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah ideally we only run this on CI, where I think it's safe to assume it's that locale. But we don't have a way to detect that, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not right now no... We could potentially pass a parameter in CI for that through gradle. That could be an option as well yeah

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me add a comment for now then.

@NachoSoto NachoSoto enabled auto-merge (squash) February 6, 2024 17:25
@NachoSoto NachoSoto merged commit bad03da into main Feb 6, 2024
7 checks passed
@NachoSoto NachoSoto deleted the paywalls-localization-test branch February 6, 2024 17:44
@vegaro vegaro added pr:other and removed pr:test labels Sep 17, 2024
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.

3 participants