Use stripe-mock for tests #452

ob-stripe · 2018-02-20T11:15:55Z

Updated version of #384.

ob-stripe · 2018-03-26T15:33:53Z

I've updated the PR to add the ability to verify requests, and also stub requests (see the functional test for FileUpload for an example with stubbed requests).

I think we have a good base to start porting all tests to stripe-mock. To summarize, for each model class, we want:

a functional test class that tests all API methods (using stripe-mock whenever possible, or stubbed requests otherwise)
a model test class that tests deserialization. If the model has at least one expandable attribute, there should be two tests: one without expanding any attributes, and one expanding all attributes

If we want to be exhaustive, we should also have deserialization tests for collection classes.

@brandur-stripe @remi-stripe Any thoughts or questions? Anything I missed before we start porting all tests?

brandur-stripe · 2018-03-26T22:41:40Z

I think we have a good base to start porting all tests to stripe-mock. To summarize, for each model class, we want:

a functional test class that tests all API methods (using stripe-mock whenever possible, or stubbed requests otherwise)

a model test class that tests deserialization. If the model has at least one expandable attribute, there should be two tests: one without expanding any attributes, and one expanding all attributes
If we want to be exhaustive, we should also have deserialization tests for collection classes.

Yep, that sounds absolutely perfect. I love that it'll get us a pretty exhaustive test of all possible expansions in a pretty direct way too.

If we want to be exhaustive, we should also have deserialization tests for collection classes.

IMO, it'd be fine just to make sure that this works for just one collection, but it's up to you :)

@brandur-stripe @remi-stripe Any thoughts or questions? Anything I missed before we start porting all tests?

Just that this is going to be a monstrous amount of work :) The existing diff looks great.

ob-stripe · 2018-03-28T16:35:42Z

src/test/java/com/stripe/functional/AccountTest.java

+import com.stripe.net.APIResource;
 import com.stripe.model.*;
 import junit.framework.Assert;
 import org.junit.Test;


Kind of a nit, but at some point in the future I'd like to enforce Google's Java style in the project. Can you reorder the import like in the other tests?

All static imports first

All imports in aphabetical order

Separate import "groups" with a new line (in most cases the groups will be java, junit, then stripe)

Don't use .*, explicitly import all the classes you need.

ob-stripe · 2018-03-28T16:36:04Z

src/test/java/com/stripe/functional/AccountTest.java

-		accountParams.put("limit", 3);
-		AccountCollection accountCollection = Account.all(accountParams);
+	@Test
+	public void testAccountUpdate() throws StripeException {


Rename to testUpdate()

ob-stripe · 2018-03-28T16:36:14Z

src/test/java/com/stripe/functional/AccountTest.java

+	}
+
+	@Test
+	public void testAccountReject() throws StripeException {


Rename to testReject()

ob-stripe · 2018-03-28T16:38:31Z

src/test/java/com/stripe/model/AccountTest.java

 import org.junit.Test;

 import static org.junit.Assert.assertEquals;
 import static org.mockito.Mockito.*;


Same comment about imports as above.

ob-stripe · 2018-03-28T16:40:25Z

src/test/java/com/stripe/model/AccountTest.java

 	@Test
 	public void testDeserialize() throws StripeException, IOException {
-		String json = getResourceAsString("account.json");
+		String json = getResourceAsString("/api_fixtures/account.json");


Rather than deserializing from an embedded JSON fixture, I'd rather we use stripe-mock whenever possible.

I disagree partially because stripe-mock return non Custom accounts which won't deserialize a lot of the more complex sub-elements like legal_entity and external_accounts

I'm happy to do both but that deserialization via stripe-mock happens in most of the other tests since we retrieve an account before reject/update/etc.

ob-stripe · 2018-03-28T16:46:33Z

src/test/java/com/stripe/model/AccountTest.java

 		Account acc = APIResource.GSON.fromJson(json, Account.class);

-		assertEquals("acct_1032D82eZvKYlo2C", acc.getId());
+		assertEquals("acct_1234", acc.getId());


I don't think it's necessary to check the value of every field (especially if you're using stripe-mock instead of an embedded fixture file). Mostly what we're testing here is that fromJson does not raise an exception.

In the other deserialization tests, I used just two asserts: assertNotNull(resource) + assertNotNul(resource.getId()) to check for the unlikely case where deserialization would not raise an exception but return null, or return an instance with null values in attributes.

For models with submodels (e.g. AccountPayoutSchedule), we should create a separate deserialization test for the submodel, e.g. in model/AccountPayoutScheduleTest.java:

String accountData = getFixture("/v1/accounts/acct_123"); String data = getDataAt(accountData, "payout_schedule"); AccountPayoutSchedule object = APIResource.GSON.fromJson(data, AccountPayoutSchedule.class); assertNotNull(object); assertNotNull(object.getInterval());

(cf. ChargeOutcomeTest.java in the first commit)

That seems pretty painful to maintain for the Account resource and all its sub-resources and various versions.

They are separate classes in the code, so they should be tested separately. It will certainly be a bit painful to cover all models (mostly due to the fact that our coverage is pretty spotty right now), but once that's done, maintenance should be minimal.

remi-stripe · 2018-03-28T19:15:18Z

@ob-stripe PTAL. I addressed all your comments (I think) and changed the Account model tests to only use stripe-mock's fixture. Then I moved LegalEntityTest to the new framework

ob-stripe · 2018-03-28T19:22:15Z

👍 Nice, looks great! Thanks for also deleting the old fixtures that are no longer used after you updated the tests.

ob-stripe · 2018-06-07T14:13:42Z

Okay, I think this is ~ready.

Coverage numbers are still fairly low primarily because of the large number of untested deprecated methods (I've tried configuring the Cobertura plugin to ignore those, but it doesn't seem to be working too well) and some setter methods (which should also be excluded from coverage but I haven't looked into it yet).

Note that there is one non-test change in this PR: I added a new method Stripe.overrideUploadBase() in order to be able to use stripe-mock to test file uploads. We already had similar methods for the API and for OAuth anyway.

r? @brandur-stripe @remi-stripe

remi-stripe

Lots of nitpicky/minor comments.

Also did we make sure to clean up all old fixtures and only have used ones in the new folder?

remi-stripe · 2018-06-07T14:57:50Z

src/main/java/com/stripe/Stripe.java

-  public static final String UPLOAD_API_BASE = "https://uploads.stripe.com";
  public static final String LIVE_API_BASE = "https://api.stripe.com";
  public static final String CONNECT_API_BASE = "https://connect.stripe.com";
+  public static final String UPLOAD_API_BASE = "https://files.stripe.com";


This change is safe right? I still have not fully understand the switch between files/uploads so wanted to flag.

Yes, both files.stripe.com and uploads.stripe.com point to the same IP.

remi-stripe · 2018-06-07T14:59:54Z

src/test/java/com/stripe/functional/AccountTest.java

+    );
+  }
+
+  // TODO: Decide if sub-resources tests should live here.


flagging that we should decide on that TODO

So I kind of decided that already by adding CardTest and BankAccountTest, though currently they only test the customer sources case. We can add a few more tests specifically for instanceURL though.

remi-stripe · 2018-06-07T15:04:20Z

src/test/java/com/stripe/functional/ApplicationFeeTest.java

+    assertNotNull(resources);
+    verifyRequest(
+        APIResource.RequestMethod.GET,
+        String.format("/v1/application_fees")


should we verify that params are passed?

I think it's fine to only verify params when there are params. Otherwise, we'd need to initialize and pass an empty map which is a bit annoying.

but there are params here right?

remi-stripe · 2018-06-07T15:05:58Z

src/test/java/com/stripe/functional/BalanceTransactionTest.java

+    assertNotNull(resources);
+    verifyRequest(
+        APIResource.RequestMethod.GET,
+        String.format("/v1/balance/history")


should we verify that params are sent? (won't repeat it for all retrieve, might be worth fixing all)

same as above, we are explicitly passing limit but not verifying it.

remi-stripe · 2018-06-07T15:07:30Z

src/test/java/com/stripe/functional/BankAccountTest.java

+
+  @Test
+  public void testRetrieve() throws StripeException {
+    final Customer customer = Customer.retrieve(CUSTOMER_ID);


why do we have final here and not on any of the other examples? Should we fix them all? (annoying I know but consistency is king)

Immutability is good, so ideally all variables that can be declared as final should be. I think there's a linter rule that require that variables whose declaration is more than X lines away from their first use be declared as final, which is what prompted me to start using it in some places, and then thought that we might as well use it everywhere.

In practice, I don't think it's worth stressing about too much.

remi-stripe · 2018-06-07T15:57:26Z

src/test/resources/api_fixtures/file_upload.json

@@ -0,0 +1,10 @@
+{
+  "id": "file_1BrE64KCFFPkgtRhvXK81rmT",


random but let's not put a real id!

remi-stripe · 2018-06-07T15:58:30Z

src/test/resources/api_fixtures/card.json

@@ -0,0 +1,26 @@
+{
+  "id": "card_1CaO622eZvKYlo2C22sr7okI",


remi-stripe · 2018-06-07T15:58:48Z

src/test/resources/api_fixtures/file_upload_list.json

+  "object": "list",
+  "data": [
+    {
+      "id": "file_1BrE64KCFFPkgtRhvXK81rmT",


remi-stripe · 2018-06-07T15:59:08Z

src/test/resources/api_fixtures/source_detached.json

@@ -0,0 +1,37 @@
+{
+  "id": "src_1BoJ2NKrZ43eBVAb7Qj7bnQL",


remi-stripe · 2018-06-07T15:59:20Z

src/test/resources/api_fixtures/source_detached.json

+      "routing_number": "110000000",
+      "swift_code": "TSTEZ122"
+  },
+  "client_secret": "src_client_secret_CCiTVxSUWh1dzjv2qPyYuz55",


src_..._123

brandur-stripe · 2018-06-07T18:56:07Z

Can you maybe kick this back to me once Remi's feedback is satisfied and I'll give it one more pass over? Just looking at the general structure, I suspect that I won't have many qualms — the broad strokes all look totally right. I'm sure we'll find more to fix, but we can easily offload that to other PRs. This one is likely to become stale quickly.

ob-stripe · 2018-06-07T21:18:14Z

Alright, I probably missed more than a few things, but:

all tests now use final whenever possible
the tests should be a lot more consistent
params are tested whenever we send params
I removed unnecessary fixtures
all fixtures now use fake IDs (smth_123)
I added a charge_outcome.json fixture file and enabled the ChargeOutcome deserialization test
I added some janky but working tests for connection and read timeouts in RequestOptionsTest

ptal @remi-stripe

remi-stripe · 2018-06-07T21:24:06Z

src/test/java/com/stripe/functional/RequestOptionsTest.java

+  @Test
+  public void testConnectTimeout() throws IOException, StripeException {
+    // Kind of terrible, but let's test connection timeouts with a random external IP.
+    Stripe.overrideApiBase("https://254.254.254.254");


Damn, what have I done 😱

Yeah, actually, this wasn't a great idea (and also kind of rude to whoever owns this IP). I changed it to api.stripe.com. Hopefully Travis' servers are more than 1 millisecond away from Stripe's 😐

remi-stripe · 2018-06-07T21:25:25Z

Thank you for going through all the changes thoroughly. Will defer to brandur on the second review.

Co-authored-by: Rémi Jannel <[email protected]> Co-authored-by: Olivier Bellone <[email protected]>

ob-stripe · 2018-06-07T23:26:41Z

ptal @brandur-stripe

brandur-stripe · 2018-06-08T00:30:26Z

src/test/java/com/stripe/BaseStripeTest.java

+    }
+
+    Mockito
+        .doReturn(APIResource.GSON.fromJson(response, clazz))


Is this indentation right? Seems weird that the style is two spaces everywhere else, but then four spaces when chaining method calls like this.

brandur-stripe · 2018-06-08T00:34:27Z

Wow, WHAT A DIFF! Thanks for doing so much work.

Okay, admittedly I only took a very high-level pass to see if I could spot anything untoward, but this LGTM. Very much looking forward to getting this in.

ptal @ob-stripe

ob-stripe force-pushed the ob-stripe-mock branch 2 times, most recently from 6d8e83e to 8a99660 Compare February 20, 2018 14:18

remi-stripe force-pushed the ob-stripe-mock branch 2 times, most recently from 23bf49e to 1b5b5cc Compare March 13, 2018 13:51

ob-stripe force-pushed the ob-stripe-mock branch 9 times, most recently from 28c06da to 73591ed Compare March 26, 2018 15:25

ob-stripe force-pushed the ob-stripe-mock branch from 73591ed to 651e536 Compare March 26, 2018 15:35

ob-stripe force-pushed the ob-stripe-mock branch 6 times, most recently from f91f035 to ffc949a Compare March 28, 2018 12:56

ob-stripe mentioned this pull request Mar 28, 2018

Fix encoding of arrays #473

Merged

ob-stripe force-pushed the ob-stripe-mock branch 2 times, most recently from f77cd7f to d43b790 Compare March 28, 2018 14:42

ob-stripe commented Mar 28, 2018

View reviewed changes

remi-stripe force-pushed the ob-stripe-mock branch 2 times, most recently from 4319402 to 59fbce5 Compare March 28, 2018 19:14

ob-stripe force-pushed the ob-stripe-mock branch 5 times, most recently from 7be7bcb to c0bda66 Compare June 7, 2018 14:07

ob-stripe changed the title ~~[WIP] Use stripe-mock for tests~~ Use stripe-mock for tests Jun 7, 2018

ob-stripe force-pushed the ob-stripe-mock branch from c0bda66 to d449372 Compare June 7, 2018 14:11

remi-stripe reviewed Jun 7, 2018

View reviewed changes

ob-stripe force-pushed the ob-stripe-mock branch 4 times, most recently from 7794837 to bf3f769 Compare June 7, 2018 18:04

ob-stripe force-pushed the ob-stripe-mock branch 4 times, most recently from 1400a37 to e7859ad Compare June 7, 2018 21:15

remi-stripe reviewed Jun 7, 2018

View reviewed changes

remi-stripe assigned brandur-stripe Jun 7, 2018

Port test suite to stripe-mock

e39d6a6

Co-authored-by: Rémi Jannel <[email protected]> Co-authored-by: Olivier Bellone <[email protected]>

ob-stripe force-pushed the ob-stripe-mock branch from e7859ad to e39d6a6 Compare June 7, 2018 21:35

brandur-stripe reviewed Jun 8, 2018

View reviewed changes

ob-stripe added the approved label Jun 8, 2018

ob-stripe merged commit 83b95ca into master Jun 8, 2018

ob-stripe deleted the ob-stripe-mock branch June 8, 2018 08:56

Use stripe-mock for tests #452

Use stripe-mock for tests #452

Uh oh!

Conversation

ob-stripe commented Feb 20, 2018

Uh oh!

ob-stripe commented Mar 26, 2018

Uh oh!

brandur-stripe commented Mar 26, 2018

Uh oh!

ob-stripe Mar 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

remi-stripe commented Mar 28, 2018

Uh oh!

ob-stripe commented Mar 28, 2018

Uh oh!

ob-stripe commented Jun 7, 2018

Uh oh!

remi-stripe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brandur-stripe commented Jun 7, 2018

Uh oh!

ob-stripe commented Jun 7, 2018

Uh oh!

Choose a reason for hiding this comment

ob-stripe Mar 28, 2018 •

edited

Loading