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.

Sign up for GitHub

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

Jump to bottom

Update FakeFormActivityLauncher and FakeManageActivityLauncher to use turbines #9957

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Update FakeFormActivityLauncher and FakeManageActivityLauncher to use turbines #9957

wants to merge 3 commits into from
+48 −74

Conversation

tjclawson-stripe
Copy link
Collaborator

Summary

Replace class vars with turbines for better testing

Motivation

https://jira.corp.stripe.com/browse/MOBILESDK-3029

Testing

  • Added tests
  • Modified tests
  • Manually verified

@tjclawson-stripe tjclawson-stripe requested review from a team as code owners January 22, 2025 20:51
jaynewstrom-stripe
jaynewstrom-stripe reviewed Jan 22, 2025
View reviewed changes
...ntsheet/src/test/java/com/stripe/android/paymentelement/embedded/FakeFormActivityLauncher.kt Outdated
launchArgs = input
_launchStateTurbine.add(
LaunchState(
didLaunch = true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you can remove this from the launch state. It'll only be emitted if the launch was called.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This should still be possible!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed

jaynewstrom-stripe
jaynewstrom-stripe reviewed Jan 22, 2025
View reviewed changes
...ntsheet/src/test/java/com/stripe/android/paymentelement/embedded/FakeFormActivityLauncher.kt Outdated
private val _launchStateTurbine = Turbine<LaunchState>()
val launchTurbine: ReceiveTurbine<LaunchState> = _launchStateTurbine
private val _unregisterTurbine = Turbine<Boolean>()
val unregisterTurbine: ReceiveTurbine<Boolean> = _unregisterTurbine
Copy link
Collaborator

Choose a reason for hiding this comment

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

You could probably even just replace this with Unit instead of Boolean. We don't really care about the value. Just that a value was emitted.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed to Unit

jaynewstrom-stripe
jaynewstrom-stripe reviewed Jan 22, 2025
View reviewed changes
...rc/test/java/com/stripe/android/paymentelement/embedded/manage/FakeManageActivityLauncher.kt Outdated
private set
var didUnregister = false
private set
private val _launchStateTurbine = Turbine<LaunchState>()
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's probably a not terrible way to make these generic, and not need 2 of basically the same implementations.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Happy to pair on it if you want!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added generic FakeEmbeddedActivityLauncher

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 22, 2025 •
edited
Loading

Diffuse output:

OLD: identity-example-release-base.apk (signature: V1, V2)
NEW: identity-example-release-pr.apk (signature: V1, V2)

          │          compressed          │         uncompressed         
          ├───────────┬───────────┬──────┼───────────┬───────────┬──────
 APK      │ old       │ new       │ diff │ old       │ new       │ diff 
──────────┼───────────┼───────────┼──────┼───────────┼───────────┼──────
      dex │     2 MiB │     2 MiB │  0 B │   4.1 MiB │   4.1 MiB │  0 B 
     arsc │     1 MiB │     1 MiB │  0 B │     1 MiB │     1 MiB │  0 B 
 manifest │   2.3 KiB │   2.3 KiB │  0 B │     8 KiB │     8 KiB │  0 B 
      res │ 302.6 KiB │ 302.6 KiB │  0 B │ 456.7 KiB │ 456.7 KiB │  0 B 
   native │   6.2 MiB │   6.2 MiB │  0 B │  15.8 MiB │  15.8 MiB │  0 B 
    asset │   7.1 KiB │   7.1 KiB │  0 B │   6.9 KiB │   6.9 KiB │  0 B 
    other │  90.4 KiB │  90.4 KiB │ -3 B │ 170.7 KiB │ 170.7 KiB │  0 B 
──────────┼───────────┼───────────┼──────┼───────────┼───────────┼──────
    total │   9.6 MiB │   9.6 MiB │ -3 B │  21.5 MiB │  21.5 MiB │  0 B 

 DEX     │ old   │ new   │ diff      
─────────┼───────┼───────┼───────────
   files │     1 │     1 │ 0         
 strings │ 19975 │ 19975 │ 0 (+0 -0) 
   types │  6193 │  6193 │ 0 (+0 -0) 
 classes │  4985 │  4985 │ 0 (+0 -0) 
 methods │ 29820 │ 29820 │ 0 (+0 -0) 
  fields │ 17538 │ 17538 │ 0 (+0 -0) 

 ARSC    │ old  │ new  │ diff 
─────────┼──────┼──────┼──────
 configs │  164 │  164 │  0   
 entries │ 3624 │ 3624 │  0
APK 
   compressed    │   uncompressed   │                        
──────────┬──────┼───────────┬──────┤                        
 size     │ diff │ size      │ diff │ path                   
──────────┼──────┼───────────┼──────┼────────────────────────
 25.4 KiB │ -4 B │    63 KiB │  0 B │ ∆ META-INF/MANIFEST.MF 
 28.5 KiB │ +2 B │  63.1 KiB │  0 B │ ∆ META-INF/CERT.SF     
  1.2 KiB │ -1 B │   1.2 KiB │  0 B │ ∆ META-INF/CERT.RSA    
──────────┼──────┼───────────┼──────┼────────────────────────
 55.1 KiB │ -3 B │ 127.3 KiB │  0 B │ (total)

jaynewstrom-stripe
jaynewstrom-stripe approved these changes Jan 22, 2025
View reviewed changes
Copy link
Collaborator

@jaynewstrom-stripe jaynewstrom-stripe left a comment

Choose a reason for hiding this comment

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

Thanks!

...eet/src/test/java/com/stripe/android/paymentelement/embedded/FakeEmbeddedActivityLauncher.kt
import app.cash.turbine.ReceiveTurbine
import app.cash.turbine.Turbine

internal class FakeEmbeddedActivityLauncher<T>(
Copy link
Collaborator

Choose a reason for hiding this comment

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

In theory there is nothing embedded specific about this. Are there any other mocks we could remove and replace with this instead (in a follow up)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Created a follow up ticket https://jira.corp.stripe.com/browse/MOBILESDK-3041

@tjclawson-stripe tjclawson-stripe enabled auto-merge (squash) January 22, 2025 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaynewstrom-stripe jaynewstrom-stripe jaynewstrom-stripe approved these changes

@amk-stripe amk-stripe Awaiting requested review from amk-stripe amk-stripe is a code owner automatically assigned from stripe/android-payments-reviewers

Assignees
No one assigned
Labels
embedded
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tjclawson-stripe @jaynewstrom-stripe