Christian, thanks for the information about DatabaseBase for mocking, that makes sense.
I don't know PHP at all, but I know something about how to do automated tests. Besides manipulating tables directly, I've seen a couple of other things in the unit tests that struck me as strange also:
* at least one test relies on a hard-coded path to a file on disk * there is a global timeout variable used by a number of tests settable (I think to 1s, 10s, 60s). (As opposed to for example, polling for a particular state from within the test itself)
On Friday Erik sent a msg. to Engineering "Alternatives to the 20% approach" where he discussed a number of aspects of code review. My impression is that production code gets a lot of scrutiny, but the tests do not. I wonder if we reviewed the unit tests for good practice as well if that would eventually reduce the "chore" and "burden" required by the current situation with code review.
-Chris
PS It would be great to see some of this information on http://www.mediawiki.org/wiki/Manual:PHP_unit_testing which is pretty spare right now.
On Sun, Jul 1, 2012 at 5:49 AM, Christian Aistleitner < christian@quelltextlich.at> wrote:
Hi Platonides,
On Sat, Jun 30, 2012 at 03:45:14PM +0200, Platonides wrote:
On 30/06/12 14:24, Christian Aistleitner wrote:
[ Mocking the database ]
[...] One would have to abstract database access above the SQL layer (separate methods for select, insert, ...) [...]
You still need to implement some complex SQL logic.
One might think so, yes. But as I said, one would mock /above/ the SQL layer. For typical database operations, SQL would not even get generated in the first place!
Consider for example code containing $db->insert( $param1, $param2, ... );
The mock db's insert function would compare $param1, $param2, ... against the invocations the test setup injected. If there is no match, the test fails. If there is a match, the mock returns the corresponding return value right away. No generating SQL. No call to $db->tableName. No call to $db->makeList. No call to $db->query. No nothing. \o/
But maybe you hinted at DatabaseBase::query? DatabaseBase::query should not be used directly, and it's hardly is. We can go for straight for parameter comparison there as well. No need to parse the SQL.
Unit testing is about decoupling and testing things in isolation. With DatabaseBase and the corresponding factories, MediaWiki has a layer that naturally decouples business logic from direct database access.
Use the decoupling, Luke!
Christian
P.S.: As an example for decoupling and mocking in MW, consider tests/phpunit/maintenance/backupTextPassTest.php:testPrefetchPlain. This test is about dumping a wiki's database using prefetch.
The idea behind prefetch is to use an old dump and use texts from this old dump instead of asking the database for every single text of the new dump.
To test dumping using prefetch /without/ mocking, one would have to setup an XML for the "old" dump. This old dump's XML would get read, parsed, interpreted, ... upon each single test invocation. Tedious and time consuming. Upon each update of the XML format, we'd also have to update also the XML representation of the "old" XML dump. Yikes! [1] Besides, it duplicates efforts, as reading dumps, interpreting them is a separate issue and dealt with in isolation already in tests/phpunit/maintenance/backupPrefetchTest.php
So the handling of the old dump reading, ... has been mocked out. All that's necessary for this is lines 143--153 and line 160 of tests/phpunit/maintenance/backupTextPassTest.php:testPrefetchPlain
$prefetchMock is the mock for the prefetch (i.e.: old dump). $prefetchMap models the expected parameters and return values of the mocked method. So for example invoking $prefetchMock->prefetch( $this->pageId1, $this->revId1_1 ) yields "Prefetch_________1Text1" .
[1] Yes, we had that situation recently, when the parentid tag got introduced [2]. The XML dumps of tests/phpunit/maintenance/backupTextPassTest.php tests/phpunit/maintenance/backupPrefetchTest.php were updated. So the tests assert that both dumping, and prefetch works with parentid. But we did not have to touch the mock due to this decoupling.
[2] See commit d04b8ceea67660485245beaa4aca1625cf2170aa https://gerrit.wikimedia.org/r/#/c/10743/
-- ---- quelltextlich e.U. ---- \ ---- Christian Aistleitner ---- Companies' registry: 360296y in Linz Christian Aistleitner Gruendbergstrasze 65a Email: christian@quelltextlich.at 4040 Linz, Austria Phone: +43 732 / 26 95 63 Fax: +43 732 / 26 95 63 Homepage: http://quelltextlich.at/
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l