stripe-perl icon indicating copy to clipboard operation
stripe-perl copied to clipboard

unit tests should exercise all combinations of Net::Stripe object argument types

Open sherrardb opened this issue 4 years ago • 2 comments

this is a general note, and a very-long-term todo list item.

basically, what i have discovered while working on #94, #134, #137, #138, etc, is that there are various combinations of argument types that we are not testing for, based on the values we claim to accept in the Kavorka signature for a given method.

for example, take post_customer(). it accepts Net::Stripe::Customer|Str for customer and it accepts Net::Stripe::Card|Net::Stripe::Token|Str|HashRef for card with Str representing either token id or card id. that leaves us with 2 x 5 combinations that we should be testing.

then, add to that the fact that post_customer() can be called to create a customer or update an existing customer, and we have 2 x 2 x 5 combinations.

while this might seem like overkill, the reality is that any of these individual combinations is subject to fail either because of how the method is written or because of the restrictions of the Stripe API. for example, the API call for post_charge() works when passing a token id with no customer, but fails when passing token id with an existing customer.

so my current approach, which is subject to change :-), is to add appropriate unit tests for the stated Net::Stripe object arguments, and to either add more-nuanced validation to provide a predictable, pre-API-call error for the combinations that we know not to work, or to remove the TypeConstraint from the Kavorka signature if an argument type fails in all combinations.

side note: the parameter validation seems to be made a little bit cleaner by having separate methods for creation and updating in cases where the acceptable argument types differ between the two calling forms.

sherrardb avatar Jan 15 '20 14:01 sherrardb

correction, the combinations for post_customer() are not 2 x 2 x 5 because some combinations are not valid from a logical standpoint, ie Net::Stripe::Customer and customer id are not a valid values for customer if you are creating a new customer.

that is, unless we are allowing for cases where the user calls Net::Stripe::Customer->new() directly. but my reading of the docs and tests does not indicate that this is expected/allowed.

so that leaves us with more like 15 necessary tests in the case of post_customer().

sherrardb avatar Jan 15 '20 14:01 sherrardb

also worth noting, for optional arguments, there are cases that are not as clear cut as "are we creating or updating a customer?" for example, post_charge() works without passing a card if the customer passed has an attached card. so we should ensure that our testing and in-method validation takes these nuances into account as well.

where feasible, i am inclined to generate explicit errors in Net::Stripe rather than unnecessarily hitting the API and simply passing back the produced error. but that does not seem reasonable, in this case at least, since it would require us calling get_customer() or get_cards() in order to determine whether the customer has any attached cards.

sherrardb avatar Jan 15 '20 16:01 sherrardb