Fix payday regressions #2064
Fix payday regressions #2064
Conversation
In #2036 we started using VCR to freeze HTTP API calls into test fixtures. This is great! Let's do more of that! This commit moves the VCR configuration upstream from where it began (in test_billing.py) into the gittip.testing.Harness class. Now all tests can make expensive HTTP API calls with impunity. In fact, this knocks out a XXX comment, because we had one API call elsewhere in the test suite. VCR transparently took care of it for us! I was quite pleasantly surprised to find TestPages.yml. Yay VCR! :-) My first motivation in doing this, however, was to be able to write a test for regression I'm seeing with payday (#2057). Onward!
We want to exercise Balanced from two different test scripts. This commit moves the code common to both into gittip.testing.balanced. Depends on #2058.
These address regressions introduced by #2036.
This makes us more robust in the face of failures communicating with the Balanced API. Now we've got a blanket try/except around the parts where we talk to Balanced, so we will never totally crash because of a mishandling of some sort. The ach_credit side is not as well-factored as the charge_on_balanced side, and is not tested as a result. This commit nonetheless attempts to implement the same error handling on both sides.
At https://github.com/gittip/www.gittip.com/pull/2036/files#r9757101 @mjallday suggests that we don't need to check that merchants are underwritten, as that constraint now only applies to those receiving > $100,000/yr, and we don't have anyone there yet. Moreover, I believe this condition is backwards, and would result in no-one getting paid! Without any kind of condition here, I suspect that we'll fill our logs with NoResultError()s for everyone who is due a payout and has a CC on file with Balanced but not a bank account. Better that than not doing any payouts. :-)
|
In light of our experience struggling to merge large PRs such as #1369 and #2052/#2053, I've tried a more fine-grained approach here. Rather than review this PR, I would encourage you to start with #2058. Review and merge that, then move on to #2059, then #2063. Each of those will hopefully only take 30-60 minutes to process, so hopefully you can fit them more easily into your otherwise busy day. :-) |
|
For some reason I am not able to run a single test by hand. Running using $ honcho -e defaults.env,local.env run py.test test_billing_payday.py -s -k Multiplegives me |
|
So it seems that with VCR in play you need to run the whole test file, so no filtering with |
|
It was a good idea to split it. If only we could find a way to refresh the PR/diff against the current master (without a new commit - that is the solution google found). |
Cool. Let's do more of that. :-)
Hmm ... you could diff locally, of course. Doesn't GitHub have a generic diffing interface?
I also hit a VCR learning curve. I found that when I hit I made an attempt to scope the test fixtures to the test case level instead of the test class level. I discovered the I also hit a snag with the association of Balanced cards to Balanced accounts, in the VCR seems like a powerful tool, but I don't understand it yet. :-( |
This makes us more robust in the face of failures communicating with the Balanced API. Now we've got a blanket try/except around the parts where we talk to Balanced, so we will never totally crash because of a mishandling of some sort. The
ach_creditside is not as well-factored as thecharge_on_balancedside, and is not tested as a result. This PR nonetheless attempts to implement the same error handling on both sides.Prior art (if you'd like to review/merge this PR in smaller chunks): #2058 #2059 #2063