test: lock down PaymentSheet wire-format contract for @JsonKey renames#2418
Open
realmeylisdev wants to merge 3 commits into
Open
test: lock down PaymentSheet wire-format contract for @JsonKey renames#2418realmeylisdev wants to merge 3 commits into
realmeylisdev wants to merge 3 commits into
Conversation
Add tests asserting that SetupPaymentSheetParameters.toJson() emits the field names the native iOS and Android handlers actually read. The native folders are 1:1 mirrors of stripe-react-native and get overwritten by periodic Sync commits, so any Dart-side @jsonkey(name: ...) silently drifting from the native key name breaks PaymentSheet config without a build error (see flutter-stripe#2398). Covers two renames not already guarded by the existing link.display test added in flutter-stripe#2407: - billingDetails -> "defaultBillingDetails" - customPaymentMethodConfiguration.customPaymentMethods
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds test coverage for the ChangesWire-Format Contract Tests
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/stripe_platform_interface/test/method_channel_stripe_test.dart (2)
383-392: ⚡ Quick winConsider verifying the serialized value structure.
The test verifies the key rename but doesn't check if the email value is properly serialized. If
defaultBillingDetailsis present but null or ifBillingDetailsserialization is broken, the test would still pass.🔍 Suggested enhancement
test('billingDetails is sent as "defaultBillingDetails"', () async { final params = await serialize( const SetupPaymentSheetParameters( paymentIntentClientSecret: 'pi_test', billingDetails: BillingDetails(email: 'a@b.com'), ), ); expect(params.containsKey('defaultBillingDetails'), isTrue); expect(params.containsKey('billingDetails'), isFalse); + final billing = params['defaultBillingDetails'] as Map; + expect(billing['email'], 'a@b.com'); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/stripe_platform_interface/test/method_channel_stripe_test.dart` around lines 383 - 392, The test 'billingDetails is sent as "defaultBillingDetails"' currently only checks key presence; update it to also verify the serialized value structure by deserializing or inspecting the map returned by serialize(const SetupPaymentSheetParameters(...)) and assert that the 'defaultBillingDetails' entry is a non-null map containing the expected email ('a@b.com'); reference the serialize function, SetupPaymentSheetParameters, BillingDetails, and the 'defaultBillingDetails' key when adding an expectation that params['defaultBillingDetails']['email'] == 'a@b.com' (or equivalent null-safe access) so the test fails if the billing details are missing or improperly serialized.
394-413: ⚡ Quick winConsider verifying the array type and content.
The test verifies the key exists but doesn't check if
customPaymentMethodsis actually a List or if the test payment method is serialized correctly. If the field is present but has the wrong type or empty content, the test would still pass.🔍 Suggested enhancement
test( 'customPaymentMethodConfiguration nests customPaymentMethods array', () async { final params = await serialize( const SetupPaymentSheetParameters( paymentIntentClientSecret: 'pi_test', customPaymentMethodConfiguration: CustomPaymentMethodConfiguration( customPaymentMethods: [ CustomPaymentMethod(id: 'cpmt_test'), ], ), ), ); final config = params['customPaymentMethodConfiguration'] as Map<dynamic, dynamic>; expect(config.containsKey('customPaymentMethods'), isTrue); + final methods = config['customPaymentMethods'] as List; + expect(methods, isNotEmpty); + expect((methods.first as Map)['id'], 'cpmt_test'); }, );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/stripe_platform_interface/test/method_channel_stripe_test.dart` around lines 394 - 413, Update the test "customPaymentMethodConfiguration nests customPaymentMethods array" to not only assert the presence of the key but also validate that params['customPaymentMethodConfiguration'] (config) holds a List under 'customPaymentMethods' and that the serialized list contains the expected serialized CustomPaymentMethod with id 'cpmt_test'; specifically, after obtaining config from serialize(SetupPaymentSheetParameters(...)) assert that config['customPaymentMethods'] is a List (or List<dynamic>) and that at least one element in that list is a Map containing the key/value pair for id: 'cpmt_test' so the serialization for CustomPaymentMethodConfiguration and CustomPaymentMethod is verified.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/stripe_platform_interface/test/method_channel_stripe_test.dart`:
- Around line 383-392: The test 'billingDetails is sent as
"defaultBillingDetails"' currently only checks key presence; update it to also
verify the serialized value structure by deserializing or inspecting the map
returned by serialize(const SetupPaymentSheetParameters(...)) and assert that
the 'defaultBillingDetails' entry is a non-null map containing the expected
email ('a@b.com'); reference the serialize function,
SetupPaymentSheetParameters, BillingDetails, and the 'defaultBillingDetails' key
when adding an expectation that params['defaultBillingDetails']['email'] ==
'a@b.com' (or equivalent null-safe access) so the test fails if the billing
details are missing or improperly serialized.
- Around line 394-413: Update the test "customPaymentMethodConfiguration nests
customPaymentMethods array" to not only assert the presence of the key but also
validate that params['customPaymentMethodConfiguration'] (config) holds a List
under 'customPaymentMethods' and that the serialized list contains the expected
serialized CustomPaymentMethod with id 'cpmt_test'; specifically, after
obtaining config from serialize(SetupPaymentSheetParameters(...)) assert that
config['customPaymentMethods'] is a List (or List<dynamic>) and that at least
one element in that list is a Map containing the key/value pair for id:
'cpmt_test' so the serialization for CustomPaymentMethodConfiguration and
CustomPaymentMethod is verified.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 34c7fd04-a939-4807-bf44-2162970ae45a
📒 Files selected for processing (1)
packages/stripe_platform_interface/test/method_channel_stripe_test.dart
Add value-shape assertions on top of the key-rename checks, so the tests also catch regressions in the nested model toJson() output: - billingDetails: assert defaultBillingDetails.email round-trips - customPaymentMethodConfiguration: assert customPaymentMethods is a non-empty List whose first element's "id" matches
Contributor
Author
|
Heads-up: the failing
This PR only touches |
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add contract tests asserting that
SetupPaymentSheetParameters.toJson()emits the JSON field names the native iOS and Android handlers actually read. Complements the regression test added in #2407.Why
Per
CONTRIBUTING.md, the native folders (packages/stripe_ios/ios/.../Stripe Sdk/,packages/stripe_android/.../reactnativestripesdk/) are kept in sync withstripe-react-nativeand are not editable here. That means the Dart side is the only place to enforce the bridge field-name contract — but there is no type-level link between Dart@JsonKey(name: …)annotations and the RN-derived native readers, so drift goes silent.Issue #2398 (
linkDisplayParamssilently ignored) was the third Link-display-class fix in five months (d76710c, #2313, #2407). #2407 added an inline contract test forlink.display. This PR extends that approach to the other two@JsonKey(name: …)renames inSetupPaymentSheetParameters:billingDetails→defaultBillingDetailsRead by
StripeSdkImpl+PaymentSheet.swift:74,StripeSdkImpl+Embedded.swift:354,PaymentSheetManager.kt:311,EmbeddedPaymentElementViewManager.kt:95.customPaymentMethodConfiguration.customPaymentMethodsRead by
Mappers.kt:1044.The tests assert by field name (not full snapshot), so they won't trip on unrelated additions to the params struct.
Test plan
flutter test packages/stripe_platform_interface/test/method_channel_stripe_test.dart— all 28 tests pass, including the 2 new onesdart formatcleanflutter analyzecleanAI/LLM disclosure
Per
CONTRIBUTING.md: this PR was drafted with Claude Code (Anthropic Claude Opus 4.7). The author reviewed all generated code, the tests were run locally, and the author will own all follow-up review comments.Summary by CodeRabbit