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

Fix Samsung memory leak in screenshot tests #8589

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Fix Samsung memory leak in screenshot tests #8589

wants to merge 1 commit into from

Conversation

tillh-stripe
Copy link
Collaborator

@tillh-stripe tillh-stripe commented Jun 5, 2024 •
edited
Loading

Summary

This pull request fixes a memory leak in TestPaymentSheetScreenshots that’s happening on some Samsung devices.

Motivation

Testing

  • Added tests
  • Modified tests
  • Manually verified

Screenshots

Before After
before screenshot after screenshot

Changelog

@tillh-stripe tillh-stripe force-pushed the tillh/fix-samsung-memory-leak-in-tests branch from b955740 to bf5b162 Compare June 5, 2024 15:35
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jun 5, 2024 •
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.3 MiB │   4.3 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.1 KiB │   8.1 KiB │  0 B 
      res │ 301.5 KiB │ 301.5 KiB │  0 B │   455 KiB │   455 KiB │  0 B 
   native │   7.3 MiB │   7.3 MiB │  0 B │  18.4 MiB │  18.4 MiB │  0 B 
    asset │   1.5 MiB │   1.5 MiB │  0 B │   1.5 MiB │   1.5 MiB │  0 B 
    other │    87 KiB │    87 KiB │ -6 B │ 161.5 KiB │ 161.5 KiB │  0 B 
──────────┼───────────┼───────────┼──────┼───────────┼───────────┼──────
    total │  12.2 MiB │  12.2 MiB │ -6 B │  25.8 MiB │  25.8 MiB │  0 B 

 DEX     │ old   │ new   │ diff      
─────────┼───────┼───────┼───────────
   files │     1 │     1 │ 0         
 strings │ 21689 │ 21689 │ 0 (+0 -0) 
   types │  6869 │  6869 │ 0 (+0 -0) 
 classes │  5634 │  5634 │ 0 (+0 -0) 
 methods │ 31456 │ 31456 │ 0 (+0 -0) 
  fields │ 18319 │ 18319 │ 0 (+0 -0) 

 ARSC    │ old  │ new  │ diff 
─────────┼──────┼──────┼──────
 configs │  164 │  164 │  0   
 entries │ 3404 │ 3404 │  0
APK 
   compressed    │   uncompressed   │                        
──────────┬──────┼───────────┬──────┤                        
 size     │ diff │ size      │ diff │ path                   
──────────┼──────┼───────────┼──────┼────────────────────────
 25.8 KiB │ -6 B │  63.9 KiB │  0 B │ ∆ META-INF/MANIFEST.MF 
 29.1 KiB │ +1 B │    64 KiB │  0 B │ ∆ META-INF/CERT.SF     
  1.2 KiB │ -1 B │   1.2 KiB │  0 B │ ∆ META-INF/CERT.RSA    
──────────┼──────┼───────────┼──────┼────────────────────────
 56.1 KiB │ -6 B │ 129.1 KiB │  0 B │ (total)

@tillh-stripe tillh-stripe force-pushed the tillh/fix-samsung-memory-leak-in-tests branch 6 times, most recently from c102045 to e943d0d Compare June 5, 2024 21:17
tillh-stripe
tillh-stripe commented Jun 5, 2024
View reviewed changes
...src/main/java/com/stripe/android/paymentsheet/example/playground/activity/AppearanceStore.kt
@@ -7,10 +7,8 @@ import com.stripe.android.paymentsheet.PaymentSheet

internal object AppearanceStore {
var state by mutableStateOf(PaymentSheet.Appearance())
var forceDarkMode by mutableStateOf(false)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can remove this workaround since we’re no longer relying on an activity-scoped dark mode.

@tillh-stripe tillh-stripe force-pushed the tillh/fix-samsung-memory-leak-in-tests branch from e943d0d to c85fa1f Compare June 5, 2024 21:25
@tillh-stripe tillh-stripe force-pushed the tillh/fix-samsung-memory-leak-in-tests branch from c85fa1f to 9233686 Compare June 6, 2024 14:20
jaynewstrom-stripe
jaynewstrom-stripe approved these changes Jun 6, 2024
View reviewed changes
paymentsheet-example/src/androidTest/java/com/stripe/android/test/core/PlaygroundTestDriver.kt
// that happen when you switch modes.
pressBack()
waitForPlaygroundActivity()
enableDarkMode(enabled = false)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice!

...n/java/com/stripe/android/paymentsheet/example/playground/PaymentSheetPlaygroundViewModel.kt
"Canceled"
// We don't show the toast in tests so that we don't
// pollute the screenshots with those overlays.
"Canceled".takeUnless { runningInTest }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we log it instead then?

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

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

Successfully merging this pull request may close these issues.

None yet
2 participants
@tillh-stripe @jaynewstrom-stripe