openfoodfoundation / openfoodnetwork

Connect suppliers, distributors and consumers to trade local produce.
https://www.openfoodnetwork.org
GNU Affero General Public License v3.0
1.11k stars 719 forks source link

[Spike] Automate Stripe testing #4639

Closed lin-d-hop closed 4 years ago

lin-d-hop commented 4 years ago

Description

As a tester and developer I want to know if Stripe testing can be fully automated.

Currently testers test that new enterprises can be connected to a Stripe account and this account can then be used to take payments in every release. This is the main automation win for testers.

Acceptance Criteria & Tests

To give an overview - the following is a list of all the places users/enterprises interact with Stripe in the platform:

  1. Enterprise Owners/Managers can take payments from backoffice Orders -> Payments
  2. Shoppers can save cards from Accounts interface
  3. Shoppers can give permission for enterprises to charge their cards in Accounts interface
  4. Shoppers can buy with cards at Checkout
  5. Shoppers can save cards at Checkout
  6. Enterprise Owners/Managers can create subscriptions with cards that a shopper has previously saved and given permission for the enterprise to charge from
  7. Enterprise Owners/Manager can refund - partially and fully - credit owning to shoppers.

This is a spike so no testing required.

Matt-Yorkley commented 4 years ago

Am I right in thinking the plan is something like this?

luisramos0 commented 4 years ago

Hey Matt, we dont have a plan. Kristina was assigned to this just before she stopped contributing regularly and I have never really had a look at it.

Your plan is one option. I found this now: https://github.com/stripe/stripe-mock

luisramos0 commented 4 years ago

@mkllnk mentioned a few tools they use in CERES? VCR? I think now is the right time for us to decide what's the tech approach to this challenge. First we need a list of options :-) @mkllnk can you comment? Thanks!

mkllnk commented 4 years ago

Well, Ceres Fair Food doesn't use Stripe. The used payment gateway has simple API calls without webhooks. But my idea to got forward would be:

luisramos0 commented 4 years ago

great, thanks Maikel!

I see that we are already using webmock for most of our stripe tests. Webmock is mostly used for our controller tests but some of our feature tests are using webmock already. I wonder if all we need to do is to add some more compreensive checkout feature specs for stripe that use webmock.

I think the difference between webmock and vcr is that VCR will call the real endpoint and store the response while using webmock we have to write down the response ourselves. After having a look now, I dont think that VCR would bring a lot more than what we can do with webmock. I think our challenge is really to write the more advanced tests that the new SCA workflows involve with redirects, etc. Thoughts?

mkllnk commented 4 years ago
Matt-Yorkley commented 4 years ago

I think in most cases the code on our end is just looking at the response code and checking basic fields like error in the response, right? It looks like it would be quite easy to do with Webmock :+1:

luisramos0 commented 4 years ago

This is how we are doing it right now here

I wonder if it's just a matter of expanding these tests to cover more scenarios.

We would also have to do this type of stubbing in feature tests to test the redirects.

Matt-Yorkley commented 4 years ago

Yes, I think that's what we need. We have these issues where a card is not accepted, the stripe response seems reasonable, but then something weird happens on our end with persisting orders or payments. Some feature tests with stubbed responses on rejected payments and then checking our order and payment objects would be really great.

luisramos0 commented 4 years ago

:ok_hand: we have a plan. This will be top of dev ready again very soon :+1:

luisramos0 commented 4 years ago

this is related to #5014

Matt-Yorkley commented 4 years ago

This epic is moving into dev ready, so it would be good to clarify things a bit. I feel like there's potentially a few levels of complexity here and there's definitely multiple solutions. It might be that we ultimately need multiple "phases" of implementation here, with different solutions.

I'm basically imagining this epic on an ease-value matrix where we lump the simpler work into phase one (which can go into dev-ready this week), and the more complicated work into phase two (which we can argue about later :wink:).

As a rough plan for phase one:

Maybe phase two would cover things like subs and webhooks, and might require something beyond Webmock? How does that sound?

Matt-Yorkley commented 4 years ago

I've seen the stripe-ruby-mock gem recommended in multiple places for Stripe testing in Rails, and there's a guide for writing specs with it here. It provides lots of helpers for mocking Stripe behavior, and has pre-defined fixtures for various Stripe responses that can be called easily.

...this library overrides stripe-ruby's request method to skip all http calls and instead directly return test data. This allows you to write and run tests without the need to actually hit stripe's servers.

luisramos0 commented 4 years ago

Nice. I think we can do two things as part of this issue:

I'd go with webmock only, a new dependency stripe-ruby-mock may be an over kill (I think we would need to keep it updated according to stripe upgrades). Subs can be left out of this story :+1:

Matt-Yorkley commented 4 years ago

If we run into issues with requests made from javascript we can try capybara-webmock which allows stubbing requests made from javascript in the browser in feature specs.

luisramos0 commented 4 years ago

ah, ok, maybe we will need that, nice!

Matt-Yorkley commented 4 years ago

I've had a quick play with this and the stripe js form doesn't load, which messes with the tests. Maybe we can try this gem from thoughtbot: fake_stripe, which looks like it replaces stripe.js during tests with something the suite can actually use.

luisramos0 commented 4 years ago

what about the spec in spec/features/consumer/shopping/checkout_stripe_spec.rb?

Matt-Yorkley commented 4 years ago

what about the spec in spec/features/consumer/shopping/checkout_stripe_spec.rb?

Ah, I hadn't updated master for a while when I made a branch to play with Stripe. This test in that spec only checks the use of saved cards, and I think the form elements for that are OFN and not Stripe.

The issue is that when StripeJS loads, it injects an iframe with the form elements into the page, and that iframe does not load in the test suite, so it can't be filled out. There's also some other interactions between our Angular classes and the StripeJS Element class that fail without it. The fake_stripe gem has a fixture to replace StripeJS, but it has a few issues: the newer versions don't work with our version of Capybara, and the faked Stripe class is missing some methods that our Angular code interacts with. I've made a very rough working version of it here: #6050

luisramos0 commented 4 years ago

ok :+1: nice approach. I think we can still test quite a few things with saved cards. I mean, just expanding the existing spec with saved cards: create different saved cards and checkout with them. I wonder if we could test the sca redirects with saved cards. Even if we use fake_stripe I think we should do the most without it so we are as much independent from that dependency as possible.

I'd prefer to find a solution without any other dependency because any dependency here will break and we will have to upgrade, debug, fix. It's a bit of an investment.... but I dont think we have an alternative! Having said this, I think your PR looks great!

So, I'd write as many tests as possible with saved cards and no dependencies. As an example of a test: we can have two saved cards and pay with a failed one first and then try another one that works but only after sca auth. And I'd add necessary dependencies to test scenarios we cant test with saved cards: your PR.

luisramos0 commented 4 years ago

Hello @Matt-Yorkley you have created https://github.com/openfoodfoundation/openfoodnetwork/pull/6050 and talked a bit about this in the last few weeks. Do you want to assign yourself to this issue?

lin-d-hop commented 4 years ago

@Matt-Yorkley agreed to take it on with me in Slack. I'm going to assign you Matt :smile: Let me know if you need to not do this one. It's top of the priorities list so we'd love to get it done.... but obvs looking after yourself if more important :heart:

If you want this Spike to become issues (and a new epic) let me know. Happy to help.

Matt-Yorkley commented 4 years ago

So I've done a spike already and made a PR here that gives us a way to handle Stripe requests in specs: #6050. I'll update it now and move it to code review.

I think it needs some review and discussion to see if we all agree on the proposed implementation. It adds two basic tests and a helper that can be used as a template for writing the other specs in the list. :+1:

Matt-Yorkley commented 4 years ago

There's some behavior we should probably cover that caused issues recently and isn't tested, which is: attempting a payment that fails, then immediately attempting another payment that succeeds. Multiple payment objects are created and their states are different.

Matt-Yorkley commented 4 years ago

I'll close this spike and create a new epic and we can curate a list of what we want there.