Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

[In_App_Purchase] Improve testability #2016

Merged
merged 14 commits into from
Sep 3, 2019
Merged

[In_App_Purchase] Improve testability #2016

merged 14 commits into from
Sep 3, 2019

Conversation

juliocbcotta
Copy link
Contributor

@juliocbcotta juliocbcotta commented Aug 26, 2019

Description

This PR creates a factory for BillingClient, with that was possible to remove a secondary constructor that was used just for test.

With the introduction of the factory there was the need to update some tests as it was testing states that would never be reached in the normal plugin flow (due the usage of the constructor for testing). Basically, the test cases depended on the state of BillingClient that would be null if no previous setup was executed.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See [Contributor Guide]).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze) does not report any problems on my PR.
  • I read and followed the [Flutter Style Guide].
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I updated pubspec.yaml with an appropriate new version according to the [pub versioning philosophy].
  • I updated CHANGELOG.md to add a description of the change.
  • I signed the [CLA].
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  • No, this is not a breaking change.

@juliocbcotta juliocbcotta changed the title [WIP] [In_App_Purchase] Improve testability [In_App_Purchase] Improve testability Aug 29, 2019
@juliocbcotta juliocbcotta removed the WIP label Aug 29, 2019
@mklim mklim self-assigned this Aug 30, 2019
Copy link
Contributor

@mklim mklim left a comment

Choose a reason for hiding this comment

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

Thank you for the improvement! I have a minor optional/subjective suggestion.

import com.android.billingclient.api.BillingClient;
import io.flutter.plugin.common.MethodChannel;

public class BillingClientFactory {
Copy link
Contributor

Choose a reason for hiding this comment

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

So this works and it's definitely an improvement over the current code. But there's some subjective style nits I have with this approach.

  • It's not final, so it could be subclassed in unexpected ways in the future. https://stackoverflow.com/a/218761. I think we need this to be non-final for it to be mockable, which leads to...
  • When this is used in the test, it's mocked and then the mock is returning the other mock BillingClient. That totally works but it's usually better to use real objects whenever possible.

It adds a little extra overhead, but I'd prefer that this were an interface that the test had it's own implementation instead.

interface BillingClientFactory {
  BillingClient createBillingClient(Context context, MethodChannel channel);
}

// Instantiated in InAppPurchasePlugin#registerWith
class BillingClientFactoryImpl implements BillingClientFactory {
  public BillingClient createBillingClient(Context context, MethodChannel channel) {
    return BillingClient.newBuilder(context)
          .setListener(new PluginPurchaseListener(channel))
          .build();
  }
}

And then the tests could have a real implementation of this that returned the mock:

  @Before
  public void setUp() {
    MockitoAnnotations.initMocks(this);

    BillingClientFactory factory = (Context context, MethodChannel channel) -> mockBillingClient;
    plugin = new InAppPurchasePlugin(factory, registrar, mockMethodChannel);
  }

What do you think?

Copy link
Contributor Author

@juliocbcotta juliocbcotta Aug 31, 2019

Choose a reason for hiding this comment

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

I think all your suggestions are fair.
Mockito v2 can mock final classes! \o/
https://stackoverflow.com/a/40018295/2742962
but it comes with some overhead /o\
As we control the abstraction, let's go with the interface. :-D

@juliocbcotta
Copy link
Contributor Author

@mklim
I just ran pub global run flutter_plugin_tools format and now the CI is complaining. Would you help me? I am using flutter master channel here.

@cyanglaz
Copy link
Contributor

cyanglaz commented Sep 3, 2019

It seems to complain about a recent change I made in the connectivity.
90aaca8 didn't complain about the formatting tho. I will keep an eye out.

@cyanglaz
Copy link
Contributor

cyanglaz commented Sep 3, 2019

@BugsBunnyBR I think the commit passed the formatting check. Could you try again after merging master, and also make sure your flutter is on master?

@juliocbcotta
Copy link
Contributor Author

@BugsBunnyBR I think the commit passed the formatting check. Could you try again after merging master, and also make sure your flutter is on master?

This branch is even with the master branch, I also ran pub global run flutter_plugin_tools format from flutter channel master.

@mklim
Copy link
Contributor

mklim commented Sep 3, 2019

@BugsBunnyBR @cyanglaz it looks for some reason the formatting check is just failing now. We have logic to only run checks against "changed" plugins which must have failed here. Or it's possible that the formatting rules changed since the PRs were merged in. I'll submit a fix to unblock this for now.

@mklim
Copy link
Contributor

mklim commented Sep 3, 2019

@BugsBunnyBR it looks like the formatter is complaining about a change it applied through you to this branch, a07f6e2. I bet the problem is the same that I had, running pub global run flutter_plugin_tools format with a different version of clang-format than CI on your PATH. I fixed this locally by installing clang-format-7 and passing it to the tool, so pub global run flutter_plugin_tools format --clang-format=clang-format-7.

Edited to add: I filed flutter/flutter#39767 to make sure that the versions don't go out of sync like this.

@juliocbcotta
Copy link
Contributor Author

@mklim Thank you! Let's see if the CI likes the new commit.

Copy link
Contributor

@mklim mklim left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks again!

@mklim mklim merged commit 8236a73 into flutter:master Sep 3, 2019
noelmansour pushed a commit to snowble/plugins that referenced this pull request Nov 13, 2019
This PR creates a factory for BillingClient, with that was possible to remove a secondary constructor that was used just for test.

With the introduction of the factory there was the need to update some tests as it was testing states that would never be reached in the normal plugin flow (due the usage of the constructor for testing). Basically, the test cases depended on the state of BillingClient that would be null if no previous setup was executed.
mormih pushed a commit to mormih/plugins that referenced this pull request Nov 17, 2019
This PR creates a factory for BillingClient, with that was possible to remove a secondary constructor that was used just for test.

With the introduction of the factory there was the need to update some tests as it was testing states that would never be reached in the normal plugin flow (due the usage of the constructor for testing). Basically, the test cases depended on the state of BillingClient that would be null if no previous setup was executed.
sungmin-park pushed a commit to sungmin-park/flutter-plugins that referenced this pull request Dec 17, 2019
This PR creates a factory for BillingClient, with that was possible to remove a secondary constructor that was used just for test.

With the introduction of the factory there was the need to update some tests as it was testing states that would never be reached in the normal plugin flow (due the usage of the constructor for testing). Basically, the test cases depended on the state of BillingClient that would be null if no previous setup was executed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants