stripe-android icon indicating copy to clipboard operation
stripe-android copied to clipboard

Fix `CardDetectTest` and `SSDOcrTest` `TODO`s

Open pablo-guardiola opened this issue 2 years ago • 4 comments

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 https://github.com/Kotlin/kotlinx.coroutines/issues/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

  • [x] 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

pablo-guardiola avatar Nov 29 '23 20:11 pablo-guardiola

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Nov 29 '23 20:11 CLAassistant

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.

CLAassistant avatar Nov 29 '23 20:11 CLAassistant

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!

pablo-guardiola avatar Dec 22 '23 21:12 pablo-guardiola

@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!

pablo-guardiola avatar Jan 19 '24 21:01 pablo-guardiola