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 < [email protected]> 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: [email protected] > 4040 Linz, Austria Phone: +43 732 / 26 95 63 > Fax: +43 732 / 26 95 63 > Homepage: http://quelltextlich.at/ > --------------------------------------------------------------- > > _______________________________________________ > Wikitech-l mailing list > [email protected] > https://lists.wikimedia.org/mailman/listinfo/wikitech-l > _______________________________________________ Wikitech-l mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/wikitech-l
