r100942 MediaWiki - Code Review archive

Awesome. You may want to consider adding back in some of the functionality you killed off in r100625. Particularly, the part where the curl function was being overridden to ensure that tests you do using this object remain local, but that it returns _something_ (under your control, of course) that the rest of the object can handle normally. In my experience, it is a _very_ good idea to strongly segregate all your local testing, from everything that might have the slightest, remotest possibility of actually contacting a server, particularly if it has the slightest possibility of triggering real money changing hands in a rapid-fire, automated kind of way. The best way to do this is to always ONLY run automated tests on a special object, on which you have specially damaged the outbound communication functions beyond repair. In the gateway adapter object, I have purposefully limited all outbound communication to the curl_transaction function, to make this sort of thing easy.

I agree with you that we need to keep that separate for production.

I feel that for development, it is necessary to test as close to the heart of the code as possible.

It is key to make sure that connections point to test servers.

I always do this by setting an environment variable in the web server or the CLI:

APPLICATION_ENVIRONMENT = unittesting | localhost | development | production

I am totally going to argue with you on this one, particularly if there's any chance that this code could get fired automatically on a commit trigger. If you need to get a response back from them for a test to go through, you're testing their servers more than you're unit testing our internal code, which technically speaking is no longer unit testing. Additionally: It could be catastrophic if somebody somewhere installs this code and misses a setting, or makes one bad commit. Then suddenly we're talking international wire fraud. Yes: This is actually that dangerous. Not just for us, but for others who might install the code and fire it off. It would be infinitely better for all possible involved parties if they actually had to deliberately damage something to get into trouble. Us as well.

The way I am doing it now, it is not hitting there servers.

We are only generating the request and looking at it.

i do not attempt to connect to GlobalCollect.

We only run:

return $this->buildRequestXML();

This is safe and necessary to test.

I have no intention of running: do_transaction().

Also: Is this object being used anywhere yet? I see the last time TESTS_ADAPTER_DEFAULT was defined was r100618, prior to this object existing.

Yes it is being used to signal which adapter to test.

tests/DonationInterfaceTestCase.php 131: $adapter = isset( $adapter ) ? (string) $adapter : TESTS_ADAPTER_DEFAULT ;

Yeah, I see that, but I also see

define( 'TESTS_ADAPTER_DEFAULT', 'GlobalCollectAdapter' );

in TestConfiguration. Shouldn't this point to the test object?

This maps to the test adapter.

We are not doing unit tests against GlobalCollect, which I believe is the issue with the fixme.

We are testing that the request xml is properly generated for each payment product.