Fix issue with CollectInputs return values on iOS #748
Conversation
ianlin-bbpos
requested review from
billfinn-stripe,
bric-stripe,
nazli-stripe,
ugochukwu-stripe,
ebarrenechea-stripe and
mshafrir-stripe
as code owners
|
Sure, we are working on that. |
ianlin-bbpos
force-pushed
the
bbpos/fix-collectinputs-return-values
branch
from
80406e8 to
85be7bc
Compare
src/__tests__/functions.test.ts
Outdated
| @@ -42,6 +42,38 @@ const mockLocations = [ | |||
| }, | |||
| ]; | |||
|
|
|||
| const mockCollectInputsReturns = [ | |||
There was a problem hiding this comment.
Can we use the return types defined in the public docs (index.ts) here? We want to verify that the return value of collectInputs matches the types we defined, since these don't depend on the type they can be theoretically different.
src/__tests__/functions.test.ts
Outdated
| { | ||
| skipped: false, | ||
| text: '_text', | ||
| toggles:[], |
There was a problem hiding this comment.
Can we include tests where toggles are used as well so we can test the toggle mapping code
src/__tests__/functions.test.ts
Outdated
| @@ -127,6 +159,7 @@ describe('functions.test.ts', () => { | |||
| .mockImplementation(() => ({})), | |||
| cancelCollectSetupIntent: jest.fn().mockImplementation(() => ({})), | |||
| setSimulatedCard: jest.fn(), | |||
| collectInputs: jest.fn().mockImplementation(() => ({ collectInputResults: mockCollectInputsReturns })), | |||
There was a problem hiding this comment.
Correct me if I'm understanding this wrong, but this test seems to be mocking collectInputs to return mockCollectInputsReturns, then it checks that collectInputs returns mockCollectInputsReturns, which seems to just test that the mocked function is mocked correctly.
What method are we mocking here? I'm a bit confused where mapFromCollectInputs from above is being tested.
There was a problem hiding this comment.
@zakh-stripe, The codes submitted for jest testing is indeed unable to achieve the test effect. We should not use mock functions. We have also tried calling the CollectInputs method in function.ts directly, but it seems that we cannot call the code at the native SDK level. And mapFromCollectInputs is not a exposed method which is implemented in native SDKs.
There was a problem hiding this comment.
Ah, thanks for looking into this, it's concerning that all of the other tests are also doing this same mocking pattern.
Are you able to make a new test file that would have access to the inner mapFromCollectInputs method? Maybe you would have to right the test in Swift instead, but that would do the trick
There was a problem hiding this comment.
Thanks for your guidances. emm..I don't have a workaround with this test yet, without modifying the native part codes. I'll do some research on it. And can we revert this modification of test of this PR, let it just for fixing CollectInputs returns then open a new one for unit test?
There was a problem hiding this comment.
Okay, just want to double check that you've manually tested the changes and confirmed that the return values are the ones we are putting in our public docs (index.ts)
And can we revert this modification of test of this PR
I can approve to unblock once this is in, thanks
src/__tests__/functions.test.ts
Outdated
|
|
||
| it('collectInputs returns a proper value', async () => { | ||
| const functions = require('../functions'); | ||
| await expect(functions.collectInputs()).resolves.toEqual({ |
There was a problem hiding this comment.
Can we create a test for the mapping methods separately? I can't clearly see what is the input we are passing into mapFromCollectInputs, and I'm unsure that the test that is written here is actually running the changed code.
There was a problem hiding this comment.
I have some concerns with the current test since it might not be testing what we need, though it might be because I'm unfamiliar with the code base.
@nazli-stripe can you double check this as well? I would like to make sure that our tests are correct to prevent further incidents in the future. Thanks!
|
@zakh-stripe We have also considered using detox for e2e testing, but does the CollectInputs function currently support automatic testing? I mean whether it can be performed on the simulator and the inputs can be automatically filled in, return the results? |
We have some e2e automated testing but it's only for the Java SDK since it's quite tricky to set up correctly. I'm not too familiar with how it can be done on other SDKs |
ianlin-bbpos
force-pushed
the
bbpos/fix-collectinputs-return-values
branch
from
85be7bc to
6bc2f65
Compare
|
Hey @ianlin-bbpos , were you able to manually test this change? If so, if you could include some screenshots that would be perfect |
yup, I did some tests manually last week and updated the PR content with a screenshot. Let me upload some more. |
|
|
Thanks for adding the screenshots!
For unit tests, I think we should aim to check that errors are also handled properly. If we can get these for both iOS and Android that'd be great and will hopefully catch these issues earlier in the future 👍 |
* fix issue with CollectInputs return values on iOS. * fix CollectInputs params errors. * change method name.
Summary
Fix issue with CollectInputs return values on iOS.
Motivation
To fix issue #743
after
Testing
Documentation
Select one: