-
Notifications
You must be signed in to change notification settings - Fork 50
Add subscriptions integration tests (4603) #3425
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
Conversation
scenarios: - Preventing duplicate renewals for recent subscriptions - Creating renewal orders for subscriptions older than 8 hours - Handling subscriptions with explicit renewal meta - Ensuring payment methods are correctly transferred to renewal orders - Verifying transaction IDs are properly set on orders - Managing subscription status transitions during renewal - Handling edge cases like missing parent orders and empty subscription arrays
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks great, just a couple of small notes:
- CI is failing, maybe we can skip those failing tests on CI for now.
- There is no cleanup for created database records (orders, subscriptions...), we can skip this for this PR as probably there are also other existing tests that does no clean the database, so I suggest to create a separated PR for ensuring db records are cleared after each test.
public function createCustomerIfNotExists(int $customer_id= 1): int | ||
{ | ||
$customer = new \WC_Customer($customer_id); | ||
if (! empty($customer->get_email() )) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this condition correct? or should be inverted?
$mockOrderEndpoint = $this->mockOrderEndpoint('CAPTURE', true); | ||
$c = $this->setupTestContainer($mockOrderEndpoint); | ||
|
||
$paymentToken = $this->setupPaymentToken($this->customer_id, $gateway_id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$paymentToken
seems not used in this test.
$mockOrderEndpoint = $this->mockOrderEndpoint('CAPTURE', false); | ||
$c = $this->setupTestContainer($mockOrderEndpoint); | ||
|
||
$paymentToken = $this->setupPaymentToken($this->customer_id, PayPalGateway::ID); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$paymentToken
seems not used in this test.
$c = $this->setupTestContainer($mockOrderEndpoint); | ||
|
||
// Setup payment token and subscription | ||
$paymentToken = $this->setupPaymentToken($this->customer_id, PayPalGateway::ID); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$paymentToken
seems not used in this test.
public function test_process_empty_subscriptions_array() | ||
{ | ||
$c = $this->getContainer(); | ||
$logger = $c->get('woocommerce.logger.woocommerce'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$logger
seems not used in this test.
Yeah I will create myself an issue to handle preconditions and teardown of all the tests |
*/ | ||
public function mockOrderEndpoint(string $intent = 'CAPTURE', bool $success = true): object | ||
{ | ||
$order_endpoint = $this->getMockBuilder(OrderEndpoint::class) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we were using Mockery in most of the tests in this project. Should we maybe continue that for consistency, or should we switch to PHPUnit mocks? I am not sure if they have any significant difference.
*/ | ||
public function test_vaulting_is_enabled_when_subscription_mode_is_vaulting_api() | ||
{ | ||
add_filter('user_has_cap', function ($allcaps, $caps, $args) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This filter is not being removed, might cause test pollution
This PR introduces a new IntegrationMockedTestCase class for testing and enhances the existing PayPalSubscriptionsRenewalTest class with more test coverage.
The new class provides reusable testing infrastructure including methods for creating mock orders, subscriptions, payment tokens, and simulating PayPal API responses. The PR expands the subscription renewal tests with detailed assertions for various scenarios including transaction ID handling, payment method consistency, subscription status changes, and parent order updates. Tests now follow a more structured Given-When-Then format with documentation of test intentions and expected behaviours.