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

Add unit test on iOS #764

Merged
merged 5 commits into from
Aug 1, 2024
Merged

Conversation

ianlin-bbpos
Copy link
Collaborator

Summary

Add unit test on iOS for function automated testing.

Motivation

Add automated testing requested by https://bugs.bbpos.com/projects/SDK/issues/SDK-174

Testing

  • I tested this manually
  • I added automated tests

Documentation

Select one:

  • I have added relevant documentation for my changes.
  • This PR does not result in any developer-facing changes.

@@ -19,4 +19,14 @@ final class MappersTests: XCTestCase {
XCTAssertEqual(Mappers.mapFromLocationStatus(.notSet), "notSet")
}

func testCollectInputsReturnsMapper() {
let output: NSDictionary = Mappers.mapFromCollectInputsResults([])
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hey @bric-stripe, I'm trying to add a test for checking the return data of method Mappers.mapFromCollectInputsResults, I'm block with a issue that how to mock its input param which is a CollectInputsResult array, and the class CollectInputsResult and all its subclass can't be directly instantiated. I have no idea how to create or hardcode the input params, could you give me some guidance, thanks.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah, this is because all the public classes are annotated as init and new being unavailable because we want to ensure the instances the SDK receives are generated from the SDK. But this means you can't simply instantiate them for tests either 😞

for result classes like this we should probably remove that restriction from the native SDK (its only important classes that are also inputs, like PaymentIntent)

the only option I can think of for now is to create mirror protocols that the Mapper class, and any others being tested, use and then the RN swift layer will declare that the SDK's class conforms to that protocol. I'll check out this branch and see if I can add an example commit to help unblock. It's going to be noisy though as we have to declare each param in the protocol 😞 I'll also bring this up with others to see if there are alternatives to consider.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Understood! Thanks a lot for your detailed teaching. I'm also going to do some research on that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

sorry I didn't get to this yesterday but I just pushed up a commit that does what I was thinking: 5260769

the downside is we'd have to keep the protocols in sync. But since the public classes from the native SDK are fairly stable I don't think that should be too bad. But there may be better alternatives that I'm not aware of.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for your instructive sample codes, but there seems to be some errors😂, could you take a look at it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

silly error, I just missed adding a new file. Pushed a new commit that includes that and should fix CI

@@ -32,6 +32,7 @@
"get:testbutler": "curl -f -o ./test-butler-app.apk https://repo1.maven.org/maven2/com/linkedin/testbutler/test-butler-app/2.2.1/test-butler-app-2.2.1.apk",
"docs": "npx typedoc ./src/index.tsx --out ./docs/api-reference --tsconfig ./tsconfig.json --readme none",
"unit-test:android": "cd android && ./gradlew testDebugUnitTest",
"unit-test:ios": "xcodebuild test -workspace dev-app/ios/StripeTerminalReactNativeDevApp.xcworkspace -destination 'platform=iOS Simulator,name=iPhone 15' -scheme UnitTests",
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice, thanks for adding

Copy link

@zakh-stripe zakh-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 adding these tests, they look great!

Comment on lines +46 to +47
extension StripeTerminal.TextResult : TextResult {
}

Choose a reason for hiding this comment

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

Do you know why we have this for TextResult only?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops, I think I missed the others, but the compilation and testing still passed. I know that CollectInputsResult is essential. As for the other missing ones, I also wonder if it is because they already have definitions inherited from CollectInputsResult in the native sdk? let me add the missing ones.

Comment on lines 87 to 91
XCTAssertTrue(results.contains(where: { $0[testNumericString] != nil }))
XCTAssertTrue(results.contains(where: { $0[testPhone] != nil }))
XCTAssertTrue(results.contains(where: { $0[testEmail] != nil }))
XCTAssertTrue(results.contains(where: { $0[testSelection] != nil }))
XCTAssertTrue(results.contains(where: { $0[testSignatureSvg] != nil }))

Choose a reason for hiding this comment

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

Can we update these checks to assert the order of each result is kept? I think we can loop through the array naively or even hardcode (eg, results[0][testNumericString] != nil)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok, let me improve that.

Copy link

@zakh-stripe zakh-stripe left a comment

Choose a reason for hiding this comment

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

Just a small change

Copy link

@zakh-stripe zakh-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 adding these tests!

@bric-stripe bric-stripe merged commit f2750b4 into main Aug 1, 2024
3 checks passed
@nazli-stripe nazli-stripe deleted the bbpos/add-tests-for-collect-inputs branch September 7, 2024 17:15
nazli-stripe pushed a commit that referenced this pull request Sep 11, 2024
* add test func.

* Example of using a mirror protocol for testing allowing a mock struct to
be used in the tests

* derp; I forgot to git add the Protocols.swift file

* implement mapper test & add test command in bitrise.yml.

* improve test method.

---------

Co-authored-by: Brian Cooke <bric@stripe.com>
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.

3 participants