Wrong date conversion in test_gateway_charge

Does test gateway even need to perform this validation? I think uc_credit already validates expiry dates, so perhaps this can be removed.

Making it nicer ;)

I think the $month = $month % 12; was probably a faulty attempt to ensure that the month is always between 1 and 12 inclusive. That works for month = 1 through 11, but fails because it changes month 12 to 0.

uc_credit uses select elements for month and year, which guarantees that the input values have the correct range. Specifically, $month will never be <1 or >12. uc_credit also ensures that the expiration year >= current year && the expiration month >= current month. I consider this a simple sanity check of the user input, useful to ensure the user doesn't make a data entry mistake and useful to prevent submissions of obviously bad data to the gateway.

However, I do think we should continue to do this check in test_gateway.

First, because real gateways check the expiration date too. The check is not as simple as the one we use in test_gateway - a real gateway will typically verify that the entered expiration date is in the future *and* that it matches the actual expiration date for that card number. We can't do that second bit in the test_gateway, but that shouldn't stop us from doing the first check.

Second, because test_gateway is also used for Simpletest. Although our test cases are currently all web test cases, I'd really like to see some unit tests here - these would not be subject to form validation performed in uc_credit. And for the future, it would be nice to abstract payment out so that the gateway was not necessarily called only from uc_credit (see for example http://drupal.org/project/payment). Again, this bypasses checks performed in uc_credit. So it makes sense to me to check *all* typical gateway functionality that we can here.

Now that I think of it, why didn't this problem show up in the tests? The tests generate three transactions each with a random month between 1 and 12 inclusive, so statistically we should see the credit card tests fail about 23% of the time (.23 ~= 1 - 11/12 * 11/12 * 11/12 for those of you following along at home). But we don't, so there must be a problem with the tests somewhere.

The attached patch fixes the expiration date check, but I'm leaving this issue at "needs work" because we still need to figure out why the test didn't catch this.

The tests are actually faulty in a subtly different way.

They generate a random expiry date between 1/2012 and 12/2022, so there are 132 possible combinations to select from. At the time of writing these should fail anyway if they manage to generate one of the four combinations prior to 5/2012. The failure case described in this issue is only triggered for 12/2012 (or whatever the current year is during the test), so one test has a 5/132 chance of failure if run today.

I think we should rewrite these tests to include the following cases:

- failure if expiry date is in a previous year
- failure if expiry date is last month
- success if expiry date is this month
- success if expiry date is in a future year

Sounds good, but I'd still like to keep some amount of randomness in there to improve test coverage. The test cases should be designed to catch this sort of fence post error if it is re-introduced in the future. With just the above four tests, the tests would not have caught this error unless they were run in December. Perhaps:

- failure if expiry date is in a previous randomized year && randomized month
- failure if expiry date is in a previous randomized month of this year
- success if expiry date is this month && this year
- success if expiry date is in a subsequent randomized month && this year
- success if expiry date is in a future year && randomized month

With this set, the fourth test should pick up an error like in the OP.

I don't really like the way Drupal's SimpleTest implementation uses random data. PHPUnit and other unit/integration testing frameworks I have used don't work in this way; they use fixed (or at least more reliable) sets of test data, which means the tests are consistent and don't rely on coincidence to catch edge cases. Perhaps we should just test all 36 options between January (year-1) to December (year+1) every time to avoid this problem.

I have no objection to that. Dates are a pretty limited set, so it's easy to check all combinations instead of a random sampling. I think the idea of random sampling is that pre-planned data necessarily reflects only what you *think* will be input, and not necessarily everything that *can* be input. This is especially true of things like names, which might use a wide variety of non-ascii characters. Since the tests will theoretically be run frequently (every time a patch is submitted or a commit is made), using random data can provide wider coverage for the amount of time spent, and potentially uncover unexpected problems.

We can't test year-1 (easily) as it doesn't appear in the dropdown, so these patches test year to year+2. The second patch incorporates the fix from #2.

Committed.

Why did you remove all those tests from testCheckout() ?

Because they seem irrelevant to testing credit card functionality - the same set of tests already appears in uc_cart.

Forgot that this needs backporting to 6.x.

Committed #15.