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 CardDetectTest and SSDOcrTest TODOs #7681

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

Conversation

pablo-guardiola
Copy link

Summary

  • Fixes CardDetectTest and SSDOcrTest TODOs

Motivation

Quick PR fixing CardDetectTest and SSDOcrTest TODOs that I found while exploring stripe-android's repo / learning how Stripe's Android SDK works. This is mostly a quick refactoring that addresses that tech debt replacing runBlocking occurrences by runTest as Kotlin/kotlinx.coroutines#1204 was already fixed.

Also, when checking that the changes made in CardDetectTest were working as expected, noticed that the tests were not formatted to follow the triple A (Arrange / Act / Assert) while tests in SSDOcrTest were, so for consistency, took a quick pass and re-structured them. Reasoning: I think that normally is good to structure the tests following the triple A πŸ‘€ https://speakerdeck.com/guardiola31337/elegant-unit-testing-mobilization-2016?slide=40 http://wiki.c2.com/?ArrangeActAssert

BTW, I'm a big fan of Working Effectively with Unit Tests

when writing tests you should prefer DAMP (Descriptive And Maintainable Procedures)

Using DAMP you increase maintainability and readability of the tests πŸ™Œ

On a side note, I highly recommend you reading that book (if you haven't already). It's πŸ” πŸ’―

Testing

  • Added tests

  • Modified tests

  • Manually verified

  • Run tests locally to confirm there are no regressions and all tests are βœ…

Screenshots

Before After
before screenshot after screenshot

Changelog

@CLAassistant
Copy link

CLAassistant commented Nov 29, 2023

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Pablo Guardiola seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@pablo-guardiola pablo-guardiola force-pushed the pg-fix-stripecardscan-android-test-todos branch from 65bcf3f to 4e30676 Compare November 29, 2023 20:10
Copy link
Collaborator

@awush-stripe awush-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 for doing this! this looks good to me.

Copy link
Collaborator

@awush-stripe awush-stripe left a comment

Choose a reason for hiding this comment

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

looks like you'll need to sign the CLA. I'll approve once that's done.

@pablo-guardiola
Copy link
Author

pablo-guardiola commented Dec 22, 2023

Thanks for taking the time reviewing my contribution @awush-stripe πŸ™‡β€β™‚οΈ

looks like you'll need to sign the CLA.

Originally had some issues signing the CLA but I was able to do it, and apparently everything went well, in fact the license/cla job is βœ… πŸ€”

BTW I noticed that there are some E2E tests failing https://github.com/stripe/stripe-android/actions/runs/7037971542/job/19872041475?pr=7681

com.stripe.android.test.e2e.EndToEndTest > test us_bank_account setup intent flow with amounts FAILED
    java.lang.IllegalArgumentException at EndToEndTest.kt:43

com.stripe.android.test.e2e.EndToEndTest > test us_bank_account payment intent flow with amounts FAILED
    java.lang.IllegalArgumentException at EndToEndTest.kt:43

com.stripe.android.test.e2e.EndToEndTest > test us_bank_account payment intent flow with descriptor code FAILED
    java.lang.IllegalArgumentException at EndToEndTest.kt:43

com.stripe.android.test.e2e.EndToEndTest > test us_bank_account payment intent flow with amounts fails FAILED
    java.lang.IllegalArgumentException at EndToEndTest.kt:43

com.stripe.android.test.e2e.EndToEndTest > test cashapp payment intent flow with setup future usage FAILED
    java.lang.IllegalArgumentException at EndToEndTest.kt:43

com.stripe.android.test.e2e.EndToEndTest > testCreateAndConfirmSetupIntent FAILED
    java.lang.IllegalArgumentException at EndToEndTest.kt:43

com.stripe.android.test.e2e.EndToEndTest > test us_bank_account setup intent flow with descriptor code FAILED
    java.lang.IllegalArgumentException at EndToEndTest.kt:43

com.stripe.android.test.e2e.EndToEndTest > test cashapp payment intent flow FAILED
    java.lang.IllegalArgumentException at EndToEndTest.kt:43

com.stripe.android.test.e2e.EndToEndTest > test us_bank_account setup intent flow with amounts fails FAILED
    java.lang.IllegalArgumentException at EndToEndTest.kt:43

com.stripe.android.test.e2e.EndToEndTest > testCreateAndConfirmPaymentIntent FAILED
    java.lang.IllegalArgumentException at EndToEndTest.kt:43

com.stripe.android.test.e2e.EndToEndTest > testRigCon FAILED
    java.lang.IllegalArgumentException at EndToEndTest.kt:43

com.stripe.android.test.e2e.EndToEndTest > test cashapp setup intent flow FAILED
    java.lang.IllegalArgumentException at EndToEndTest.kt:43


> Task :stripe-test-e2e:testDebugUnitTest FAILED
12 tests completed, 12 failed

I'm going to rebase from master and re-push, wondering if they're failing due to the PR getting outdated. If that's not the reason, I'll dig deeper βš’οΈ

Thanks again!

Copy link
Collaborator

@awush-stripe awush-stripe 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! let's see what bitrise says

@pablo-guardiola
Copy link
Author

@awush-stripe

looks good! let's see what bitrise says

Unfortunately it keeps complaining πŸ˜“

Same E2E tests failing πŸ‘€

> Task :stripe-test-e2e:testDebugUnitTest

com.stripe.android.test.e2e.EndToEndTest > test us_bank_account setup intent flow with amounts FAILED
    java.lang.IllegalArgumentException at EndToEndTest.kt:49

com.stripe.android.test.e2e.EndToEndTest > test us_bank_account payment intent flow with amounts FAILED
    java.lang.IllegalArgumentException at EndToEndTest.kt:49

com.stripe.android.test.e2e.EndToEndTest > test us_bank_account payment intent flow with descriptor code FAILED
    java.lang.IllegalArgumentException at EndToEndTest.kt:49

com.stripe.android.test.e2e.EndToEndTest > test us_bank_account payment intent flow with amounts fails FAILED
    java.lang.IllegalArgumentException at EndToEndTest.kt:49

com.stripe.android.test.e2e.EndToEndTest > Test update payment method FAILED
    java.lang.IllegalArgumentException at EndToEndTest.kt:49

com.stripe.android.test.e2e.EndToEndTest > test cashapp payment intent flow with setup future usage FAILED
    java.lang.IllegalArgumentException at EndToEndTest.kt:49

com.stripe.android.test.e2e.EndToEndTest > testCreateAndConfirmSetupIntent FAILED
    java.lang.IllegalArgumentException at EndToEndTest.kt:49

com.stripe.android.test.e2e.EndToEndTest > test us_bank_account setup intent flow with descriptor code FAILED
    java.lang.IllegalArgumentException at EndToEndTest.kt:49

com.stripe.android.test.e2e.EndToEndTest > test cashapp payment intent flow FAILED
    java.lang.IllegalArgumentException at EndToEndTest.kt:49

com.stripe.android.test.e2e.EndToEndTest > test us_bank_account setup intent flow with amounts fails FAILED
    java.lang.IllegalArgumentException at EndToEndTest.kt:49

com.stripe.android.test.e2e.EndToEndTest > testCreateAndConfirmPaymentIntent FAILED
    java.lang.IllegalArgumentException at EndToEndTest.kt:49

com.stripe.android.test.e2e.EndToEndTest > testRigCon FAILED
    java.lang.IllegalArgumentException at EndToEndTest.kt:49

com.stripe.android.test.e2e.EndToEndTest > test cashapp setup intent flow FAILED
    java.lang.IllegalArgumentException at EndToEndTest.kt:49


13 tests completed, 13 failed
> Task :stripe-test-e2e:testDebugUnitTest FAILED

Will try to reproduce / run these tests locally and report back here πŸ‘

Thanks Adam!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants