Skip to content

Conversation

@stuartmorgan-g
Copy link
Collaborator

Updates the production code to use a single instance of each of the two Pigeon host APIs, rather than a private per-file copy, so that tests can inject an alternate implementation directly instead of relying on the method channel hook and Pigeon's dartHostTestHandler.

Part of flutter/flutter#159886

Pre-Review Checklist

Footnotes

  1. Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. 2 3

Updates the production code to use a single instance of each of the two
Pigeon host APIs, rather than a private per-file copy, so that tests can
inject an alternate implementation directly instead of relying on the
method channel hook and Pigeon's dartHostTestHandler.

Part of flutter/flutter#159886
Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the in_app_purchase_storekit package to improve testability by removing the use of Pigeon's Dart test generator. The changes introduce a centralized mechanism for managing Pigeon host API instances, using global variables that can be replaced with mocks in tests. This is achieved by creating a new in_app_purchase_apis.dart file. The production code is updated to use these global API instances, and the test files are updated to use the new mocking strategy. The fake platform implementations are also updated to implement the real API interfaces, which makes the test setup more realistic. The changes are consistent, well-structured, and effectively achieve the goal of improving the testing architecture. The code quality is excellent.

@stuartmorgan-g
Copy link
Collaborator Author

This is less clean that all the other PRs for flutter/flutter#159886 because we don't have one central class to DI the alternate implementation into, and in fact there are static methods and top-level functions that use this.

The other alternative I can see here is to make a test injection method per file, like the (unused) one that used to be in sk_payment_queue_wrapper.dart‎, but that seems less maintainable.

expect(
() async => SKReceiptManager.retrieveReceiptData(),
throwsA(const TypeMatcher<PlatformException>()),
throwsA(const TypeMatcher<Exception>()),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This change is because the test started failing. The FakeStoreKitPlatform in this file throws an Exception, not a PlatformException. There is a different FakeStoreKitPlatform in a different file that throws a PlatformException, so it seems like this test was probably passing because somehow the wrong fake implementation was registered when it ran?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant