Stripe merge requestshttps://lab.civicrm.org/extensions/stripe/-/merge_requests2021-04-10T10:48:39Zhttps://lab.civicrm.org/extensions/stripe/-/merge_requests/148Improve IPN/Webhook tests2021-04-10T10:48:39ZRichImprove IPN/Webhook tests@mattwire This is FYI at the moment; I don't need your input but it's always welcome.
Of note:
The big picture is that this MR means we're no longer touching the actual Stripe HTTP end point; everything is entirely local. This is good ...@mattwire This is FYI at the moment; I don't need your input but it's always welcome.
Of note:
The big picture is that this MR means we're no longer touching the actual Stripe HTTP end point; everything is entirely local. This is good (practice) because it reduces the focus of what we're testing to our code, not Stripe's API. And it makes the tests run much faster because (a) we're not waiting on http requests, and (b) we don't need to do `sleep(5)` any more :-)
I've done work in the main body of the extension (i.e. outside of tests/) along the lines of:
- Move away from using Stripe's static methods (which are basically impossible to test/mock) to their newer client + services methods. You've started this work anyway; this just continues it.
- We already stored a stripeClient object on the payment class, but now this is instantiated when the `CRM_Core_Payment_Stripe` is instantiated. **I think this is ok, right?**. What this means is that we can then mock the Stripe API client, so that all calls to the stripe API will then actually be using the mock - not that they should know about it.
- Use `Civi\Payment\System::singleton()->getById()` to get the `CRM_Core_Payment` object instead of instantiating `CRM_Core_Payment_Stripe` outselves with `new`. Because the former is a cached singleton pattern, it enables us to ensure the centralised (possibly mocked) stripeClient object instead of making new ones that aren't.
Beyond that I don't think I've made any changes to the actual code; the rest of my work is in the IPN test class.
There are also a couple of reverted commits that offer useful temporary helpers to mocking stuff, but you definitely would not want these to go live :scream:
The existing IPN test now runs entierly on a mocked stripeClient. It's passing. But I've not changed anything about the test itself or written others yet and the code is a bit of a mess at this stage.
## About mocking methods
I've used PHPUnit's built in mock bulider (noting that my old favourite Prophecy is being unbundled from phpunit v9). This lets you stub existing classes, but neuters all the methods.
I've also used a "PropertySpy" class where the class has no methods [in use]. This lets you provide data that you know is required, and if anything else is accessed it flags it up. It helps keep the mocking to the minimum required data for the thing we want to test, while making the tests fail if new code starts requiring things we haven't mocked. Currently this is chucked in the bottom of the IPN test class; it's only test code, it would never be accessed in non-test code.
Note that with the current IPN test, if you comment out the mock code section, it will run as it did before, using the HTTP API.
Going forward:
- I'd like to tighten up the signatures of the mocks a bit; put in some expectations. At the moment I do things like `$mockEventsService->method('retrieve')->willReturn($mockEvent)` but really I should be checking that retrieve was called with the ID of the mockEvent before merrily returning what we expect.
- The mock generation is verbose but over several new tests I'll be able to move out chunks into [DRY](https://en.wikipedia.org/wiki/Don%27t_repeat_yourself) helper methods.
- Now this basic infrastructure in place I can look to add tests for other IPN situations.6.6https://lab.civicrm.org/extensions/stripe/-/merge_requests/145update checks for whether a trxn has been processed.2021-02-19T18:03:27Zjamieupdate checks for whether a trxn has been processed.This commit removes some backward compatibility code that is no longer relevant.This commit removes some backward compatibility code that is no longer relevant.6.6