User "Catrope" changed the status of MediaWiki.r82577. Old Status: new New Status: fixme
User "Catrope" also posted a comment on MediaWiki.r82577. Full URL: https://secure.wikimedia.org/wikipedia/mediawiki/wiki/Special:Code/MediaWiki/82577#c21164 Commit summary: improve namespace related methods MWNamespace::getTalk() could give erroneus results when using it on specials namespaces (NS_MEDIA, NS_SPECIAL). It now use MWNamespace::isMethodValidFor() which will throw an exception if a special namespace was given. MWNamespace::getSubject() is now returning identity for specials namespaces. New MWNamespace::getAssociated() used to find out the subject page of a talk page and vice versa. Special namespaces will results in an exception. TESTS: Added tests for almost complete code coverage. Functions relying on global $wgCanonicalNamespaces are still incomplete though. MWNamespace::isMovable() needs more assertions. Tests results (ignoring incomplete tests output): $ php phpunit.php --filter MWNamespace PHPUnit 3.5.10 by Sebastian Bergmann. .........IIIII.......... Time: 1 second, Memory: 31.75Mb OK, but incomplete or skipped tests! Tests: 24, Assertions: 99, Incomplete: 5. Comment: <pre> + $this->object = new MWNamespace; </pre> This is useless (MWNamespace only has static methods) and unused. <pre> + $this->assertEquals( MWNamespace::getTalk( NS_MAIN), NS_TALK ); </pre> It's <code>assertEquals( $expected, $actual )</code> , so you've got the order wrong. This appears to apply to all calls in this test from a quick glance. <pre> + public function testGetTalkExceptions() { + $this->assertNull( MWNamespace::getAssociated( NS_MEDIA ) ); + $this->assertNull( MWNamespace::getAssociated( NS_SPECIAL ) ); + } </pre> This doesn't work the way you think it does. Because an exception stops the execution of the function, this doesn't test that '''both''' calls throw an exception, just that '''one''' of them does. Because the NS_MEDIA call throws an exception, the NS_SPECIAL call is never executed, so we'd never notice if that were to stop throwing an exception. Conversely, if NS_MEDIA were to stop throwing an exception but NS_SPECIAL still threw one, we'd never notice either. You would need to either split this up into two tests or use a data provider, I guess. You did do this properly in <code>testGetAssociatedExceptionsForNsMedia()</code>. <pre> + public function testIsContent() { </pre> Since the set of content namespaces is configurable, this test breaks on setups with funky configs. If you add e.g. <code>$wgContentNamespaces[] = NS_USER;</code> to your config, this test will fail. To test this properly you will have to exercise full control over <code>$wgContentNamespaces</code> for every assertion. <pre> + public function testHasSubpages() { </pre> Same thing; this was fixed properly though. _______________________________________________ MediaWiki-CodeReview mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
