Use generated API responses in stripe-ruby tests

[brandur]

This patch introduces a narrow working slice of tests that use data
generated from within the API itself to produce sample responses from a
stubbed out API in the test suite. This has a few advantages over the
current situation:

• More accurate test data back from the stub. The current TestData
  system is extremely prone to eroding (this is the biggest advantage).
• Basic validation gets run on incoming request parameters.
• Stubbing is run at a lower level (the HTTP library instead of the
  Stripe module) so we get a little more certainty that things work as
  expected.
• Contributing new tests (especially for new resources) becomes very
  easy because no sections of TestData are required.

Replacing the entire suite will take a few hours and involving a lot of
copy and pasting, but is entirably doable and after we're done we can
remove the entire TestData module and the monkey patching on the
Stripe module.

This should be considered prototype-only for the time being. I'll solicit
more feedback internally before coming to a merge decision.

[kyleconroy]

How do we verify that the correct endpoint is being hit? If I changed Stripe::Account.retrieve to point to /v1/charges, I think this test would still pass.

[brandur]

Good question. Yes indeed it would. I'll give you a few alternatives here in how we could handle this.

/cc @russelldavis For an opinion on this one as well.

First off, we could re-use the "traditional" test style where we inject values into a response and make sure those come back rather than whatever was generated:

    should "be retrievable with generated responses" do
      without_legacy_stubs do
        stub_api do
          get "/v1/account" do
            generated_response.merge!({
              :charges_enabled => false,
              :details_submitted => false,
              :email => "test+bindings@stripe.com",
            })
          end
        end

        a = Stripe::Account.retrieve
        assert_equal "test+bindings@stripe.com", a.email
        assert !a.charges_enabled
        assert !a.details_submitted
      end
    end

Secondly, we could take advantage of the fact that we're using closures here and do something simple like:

    should "be retrievable with generated responses" do
      without_legacy_stubs do
        call_received = false
        stub_api do
          get "/v1/account" do
            received = true
          end
        end

        Stripe::Account.retrieve
        assert call_received
      end
    end

Thirdly, I could build in a very basic mocking framework that could be called as mock_api instead of stub_api:

    should "be retrievable with generated responses" do
      without_legacy_stubs do
        mock = mock_api do
          get "/v1/account"
        end
        Stripe::Account.retrieve
        assert mock.satisfied?
      end
    end

The first option will be the easiest thing for now and will fall out naturally as we move all the tests over. We could add the second or third though so that we have a more forward-thinking direction here.

[kyleconroy]

Great ideas. I'm a huge fan on standardizing on option 1 as it verifies the correct endpoint is hit and returns a correct (and up to date) API response. 👍

[kyleconroy]

I should add that I don't think it's very valuable to even have tests without the stub API call, as we aren't really asserting anything /shrug

[brandur]

@kyleconroy Cool! I took one more look at this and came up with a fourth option that's pretty lightweight and minimizes new code by making Webmock work a bit harder. Here's the diff (it's also pushed to this branch):

diff --git a/test/stripe/account_test.rb b/test/stripe/account_test.rb
index 80bfbec..f9647b0 100644
--- a/test/stripe/account_test.rb
+++ b/test/stripe/account_test.rb
@@ -18,6 +18,8 @@ module Stripe
         assert_equal "test+bindings@stripe.com", a.email
         assert !a.charges_enabled
         assert !a.details_submitted
+
+        assert_requested :get, "#{Stripe.api_url}/v1/account"
       end
     end

@@ -30,6 +32,7 @@ module Stripe
     should "be retrievable with generated responses (easy edition)" do
       without_legacy_stubs do
         Stripe::Account.retrieve
+        assert_requested :get, "#{Stripe.api_url}/v1/account"
       end
     end

As you can see, it minimizes new code, and also allows us to assemble very lightweight (two line) endpoint tests that provide high-quality responses while also guaranteeing that endpoints were called.

The only downside is that the framework doesn't force you to assert the right URL, but the current state of affairs doesn't provide this guarantee either so we're not losing anything.

Thoughts?

[kyleconroy]

Ahh, this is also a great approach. I like that fact that you don't have to assert the URL, just that you can and it all works.

[kyleconroy]

Rebasing this on master should make the tests pass

[brandur]

Rebasing this on master should make the tests pass

@kyleconroy +1. Thanks!

[russelldavis]

Can you make this a module-level ivar under the Stripe module rather than a global? (I guess you'll need an attr_accessor as well.)

[brandur]

Can you make this a module-level ivar under the Stripe module rather than a global? (I guess you'll need an attr_accessor as well.)

+1. Done in 2afc076.

[brandur]

Err, and c5ba202 to re-enable the traditional behavior by default.

[brandur]

And a slightly more general note on this one: I'm still having trouble with the schema on some of the "polymorphic" endpoints (i.e. external accounts).

[kyleconroy]

Maybe we should be okay with less than 100% coverage?

[brandur]

Maybe we should be okay with less than 100% coverage?

I'm more than fine with that, but these polymorphic endpoints are pretty considerable in number, and I've already run into a few cases where I need them when trying to convert more pieces of the test suite.

If things get really stuck I'll give up on it, but there's some switch that allows these methods to produce a response resource when hit from the docs, but which isn't invoked during schema generation. Finding that switch is hard, but this will be easy once I have it.

[brandur]

Alright, this still needs a lot of work, and I still haven't fixed our problems with polymorphic endpoints, but the tests here are finally green across all versions.

[brandur]

Closing in favor of #499.