-
Notifications
You must be signed in to change notification settings - Fork 236
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
SEPA Launcher API [Tests] #814
Conversation
…DebitLauncher.java
…EPADirectDebitClient away from Listener
…e in SEPADemo Fragment
…ds from SEPAClient
…d; update demo fragment to use lambdas
…nt in demo fragment for clarity
…nt.tokenize(), add docstring
verify(listener).onSEPADirectDebitSuccess(nonce); | ||
verify(braintreeClient).sendAnalyticsEvent("sepa-direct-debit.tokenize.success"); | ||
sut.tokenize(sepaDirectDebitRequest, (sepaDirectDebitResponse, error) -> { | ||
assertEquals(sepaDirectDebitResponse.getNonce(), nonce); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just confirming - are these callback assertions being invoked? I haven't seen this done without a countdown latch before
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, these are! (Confirmed with breakpoints & making sure the tests failed when modified)
However I pushed a commit (a1b0434) to move these tests away from the callback syntax and to instead use ArgumentCaptor, to match with the standard way in this test file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh cool we maybe should move to that pattern eventually - seems cleaner. But good to go with the current pattern for now for consistency 👍
} | ||
|
||
// Save for SEPABrowserSwitchResult_Tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be in separate file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary of changes
Checklist
Added a changelog entryAuthors
@scannillo