[in_app_purchase_storekit] backfill native tests for more complete test coverage#6209
[in_app_purchase_storekit] backfill native tests for more complete test coverage#6209LouiseHsu merged 14 commits intoflutter:mainfrom
Conversation
| - (BOOL)shouldAddStorePayment:(SKPayment *)payment product:(SKProduct *)product; | ||
|
|
||
| // Dependency Injection | ||
| - (FIAPRequestHandler *)getHandler:(SKRequest *)request; |
There was a problem hiding this comment.
Instead of creating a method that will be overwritten with a partial mock I think you should pass in a factory to the constructor InAppPurchasePlugin.
InAppPurchasePlugin *plugin =
[[InAppPurchasePlugin alloc]
initWithReceiptManager:nil
handlerFactory:^(){ return [[FIAPRequestHandler alloc] initWithRequest:request]; }];You were concerned about fiddling with the init method since it has users. You can get around that by adding this new initializer to InAppPurchasePlugin+TestOnly.h and calling it from the existing initWithRequest: initializer.
Partial mocks are something we should try hard to avoid. We've had to use them in the past because of oddities that disallow us to test without crashing. That isn't the case here.
There was a problem hiding this comment.
I think I'm misunderstanding something sorry - so for example
What will these lines then look like? How do I reference the handlerFactory and ask it to please spit out another handler?
Additionally, the
request param is only available within startProductRequestProductIdentifiers: , so how would I be able to call it inside the initWithRequest: initializer?
There was a problem hiding this comment.
They would look like this:
FIAPRequestHandler *handler = self.handlerFactory();
[self.requestHandlers addObject:handler];You don't want to call the block in your init method. You want to hold on to it so that whenever you want a handler you can call it.
This is going to help: http://fuckingblocksyntax.com/ ;)
gaaclarke
left a comment
There was a problem hiding this comment.
The switch to using a block looks great. There' just one problem with the property type.
packages/in_app_purchase/in_app_purchase_storekit/darwin/Classes/InAppPurchasePlugin+TestOnly.h
Outdated
Show resolved
Hide resolved
| @property(strong, nonatomic) FlutterMethodChannel *transactionObserverCallbackChannel; | ||
|
|
||
| // Callback channel to dart used for when a function from the transaction observer is triggered. | ||
| @property(strong, nonatomic) FIAPRequestHandler * (^handlerFactory)(SKRequest *); |
There was a problem hiding this comment.
| @property(strong, nonatomic) FIAPRequestHandler * (^handlerFactory)(SKRequest *); | |
| @property(copy, nonatomic) FIAPRequestHandler * (^handlerFactory)(SKRequest *); |
You want to use copy for blocks properties since that will force any resources on the stack to be copied to the heap ( https://stackoverflow.com/questions/4081831/how-to-store-blocks-in-properties-in-objective-c#comment11631504_4081903 )
| - (instancetype)initWithReceiptManager:(FIAPReceiptManager *)receiptManager | ||
| handlerFactory:(FIAPRequestHandler * (^)(SKRequest *))handlerFactory { | ||
| self = [self initWithReceiptManager:receiptManager]; | ||
| _handlerFactory = handlerFactory; |
There was a problem hiding this comment.
| _handlerFactory = handlerFactory; | |
| _handlerFactory = [handlerFactory copy]; |
For the same reason mentioned above.
gaaclarke
left a comment
There was a problem hiding this comment.
Okay, I went back an gave a full review. I think we are close.
|
|
||
| SKPaymentTransactionStub *paymentTransaction = | ||
| [[SKPaymentTransactionStub alloc] initWithMap:transactionMap]; | ||
| NSArray *array = [NSArray arrayWithObjects:paymentTransaction, nil]; |
There was a problem hiding this comment.
| NSArray *array = [NSArray arrayWithObjects:paymentTransaction, nil]; | |
| NSArray *array = @[paymentTransaction]; |
| startProductRequestProductIdentifiers:argument | ||
| completion:^(SKProductsResponseMessage *_Nullable response, | ||
| FlutterError *_Nullable startProductRequestError) { | ||
| XCTAssertNotNil(error); |
There was a problem hiding this comment.
You'll want an expectation here to make sure that this is actually called.
| startProductRequestProductIdentifiers:argument | ||
| completion:^(SKProductsResponseMessage *_Nullable response, | ||
| FlutterError *_Nullable startProductRequestError) { | ||
| XCTAssertNotNil(error); |
There was a problem hiding this comment.
Same here, you need an expectation.
|
|
||
| [plugin refreshReceiptReceiptProperties:properties | ||
| completion:^(FlutterError *_Nullable error) { | ||
| XCTAssertNotNil(error); |
There was a problem hiding this comment.
omg this is scary cuz I forgot to replace the handler alloc-ing in the actual refreshReciept function, but the tests were still passing. once i added the expectation, it failed - except the expectation wasnt failing, it was the other asserts. 😱
..._app_purchase/in_app_purchase_storekit/example/shared/RunnerTests/InAppPurchasePluginTests.m
Show resolved
Hide resolved
|
auto label is removed for flutter/packages/6209, due to - The status or check suite Mac_arm64 macos_platform_tests master - packages has failed. Please fix the issues identified (or deflake) before re-applying this label. |
|
(hitting rerun and adding the autosubmit flag since it was an infra failure) |
|
auto label is removed for flutter/packages/6209, due to - The status or check suite Mac_arm64 macos_platform_tests master - packages has failed. Please fix the issues identified (or deflake) before re-applying this label. |
|
auto label is removed for flutter/packages/6209, due to - The status or check suite Mac_arm64 macos_platform_tests master - packages has failed. Please fix the issues identified (or deflake) before re-applying this label. |
…plete test coverage (flutter/packages#6209)
flutter/packages@2aa6e3f...9b88dbc 2024-03-06 balvindersi2@gmail.com [image_picker_for_web] migrates to package:web (flutter/packages#5799) 2024-03-06 balvindersi2@gmail.com [video_player_web] migrates to package:web (flutter/packages#5800) 2024-03-06 49699333+dependabot[bot]@users.noreply.github.com [in_app_pur]: Bump org.json:json from 20240205 to 20240303 in /packages/in_app_purchase/in_app_purchase/example/android/app (flutter/packages#6253) 2024-03-05 ian@hixie.ch [rfw] Change test coverage logic to enforce 100% coverage (flutter/packages#6272) 2024-03-05 louisehsu@google.com [in_app_purchase_storekit] backfill native tests for more complete test coverage (flutter/packages#6209) 2024-03-05 stuartmorgan@google.com [tool] Add features to support GCB auto-publish flow (flutter/packages#6218) 2024-03-05 ditman@gmail.com [web] Use TrustedTypes from pkg web. (flutter/packages#6273) 2024-03-05 engine-flutter-autoroll@skia.org Roll Flutter from 65cd84b to 3b5a2ec (26 revisions) (flutter/packages#6269) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC flutter-ecosystem@google.com,rmistry@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Added more native tests for more complete test coverage.
Fixes flutter/flutter#143889
Pre-launch Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the [pub versioning philosophy], or this PR is [exempt from version changes].CHANGELOG.mdto add a description of the change, [following repository CHANGELOG style].///).