-
Notifications
You must be signed in to change notification settings - Fork 992
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
Deflake testURLParamsWithCardAndBankFundingSources #4549
Conversation
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.
Drive-by review with one non-blocking suggestion
StripePaymentSheet/StripePaymentSheet/Source/Internal/Link/Utils/LinkURLGenerator.swift
Outdated
Show resolved
Hide resolved
4687523
@@ -50,7 +50,7 @@ struct LinkURLParams: Encodable { | |||
var intentMode: IntentMode | |||
var setupFutureUsage: Bool | |||
var cardBrandChoice: CardBrandChoiceInfo? | |||
var linkFundingSources: Set<LinkSettings.FundingSource> | |||
var linkFundingSources: [LinkSettings.FundingSource] |
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.
The idea behind using a set was to not care about the ordering in this test (and in general), but I guess the custom Equatable
implementation doesn’t really allow for that 😔
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.
The issue is that we're serializing it to JSON data, and we sort the keys when doing that, but if we use a Set
then the ordering is non-deterministic during the test.
87ed2ca
Summary
This test has about a 50% chance of failing. To remedy this, I am proposing we continue to deserialize as a Set(), but during encoding, convert to an orderedlist.
Motivation
Deflaking test
Testing
Using existing tests
Changelog