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 Failing UnitTests #1244

Merged
merged 2 commits into from
Apr 3, 2024
Merged

Fix Failing UnitTests #1244

merged 2 commits into from
Apr 3, 2024

Conversation

jaxdesmarais
Copy link
Contributor

@jaxdesmarais jaxdesmarais commented Mar 29, 2024

Summary of changes

  • BTSEPADirectDebitClient_Tests.testTokenizeWithPresentationContext_handleCreateMandateReturnsInvalidURL_returnsError_andSendsAnalytics()
    • We were returning a cannedErrorResponse here which wasn't needed and was actually returning before the method completed
  • BTJSON_Tests.testLargerMixedJSONWithEmoji()
    • "aString": "Hello, JSON 😍!" and "anInvalidURL": ":™£¢://://://???!!!" were failing to evaluate as nil when calling BTJSON.asURL() - neither of these are URLs so we expect that method to evaluate as nil
    • Adding a check of urlString.utf8.count == urlString.utf16.count allows us to evaluate the string in the same way that we were in Obj-C
    • More details on this test as a whole below...

The Why

I found this super interesting so adding it here if other folks are also interested:

  • NSString - these are UTF-16 (more reading here)
  • String - these are UTF-8 (more reading here)
  • The testLargerMixedJSONWithEmoji test has been failing because "😍" is 2 extra bytes in UTF-8 vs UTF-16 and "™£¢" is 4 extra bytes in UTF-8 vs UTF-16
  • Lots of discussions about eventually adding some logic around this to Foundation directly in the Swift forums

Alternatives

There are some alternatives to the check I added here that we can explore, but the approach I took was the closest to our v5 implementation here which used asString to see if it could be cast to a NSString (UTF-16) and if not returned null. Some alternatives:

  • Cast urlString to NSString - downside, we would then need to cast it back to a string in return URL(string: urlString) because NSString is not implicitly convertible to String
  • Using something like UIApplication.shared.canOpenURL could be an option but we would need to replace our URLs in tests with actual URLs. This also seems like overkill a bit imo, but open to opinions!
  • ... maybe something I haven't thought of yet?

Checklist

  • [ ] Added a changelog entry

Authors

@jaxdesmarais jaxdesmarais requested a review from a team as a code owner March 29, 2024 19:56
Copy link
Contributor

@agedd agedd left a comment

Choose a reason for hiding this comment

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

this is pretty cool ~ it was interesting to learn about how utf-8 and utf-16 are different on a byte level - thanks for linking these articles in your pr! 🎉

@jaxdesmarais jaxdesmarais merged commit a700593 into main Apr 3, 2024
6 of 7 checks passed
@jaxdesmarais jaxdesmarais deleted the unit-test-fixup branch April 3, 2024 16:43
stechiu added a commit that referenced this pull request Apr 3, 2024
scannillo pushed a commit that referenced this pull request Apr 19, 2024
* fix urlString comparison to behave as expected
* remove unneeded cannedErrorResponse causing tokenize to return before expected
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