Page MenuHomePhabricator

Validate @covers tags during normal test runs
Closed, ResolvedPublic

Description

I am not sure, if this is possible for a codesniffer, but it would be nice to validate @covers against the autoloader to see, if the covers makes sense

Feel free to close, if that is not possible.

See test case:
https://gerrit.wikimedia.org/r/#/c/366547/2/tests/phpunit/includes/specials/SpecialCreateTopicPageTest.php

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 27 2017, 8:27 PM

Hmmm, isn't this something that PHPUnit should do?

+cc @Krinkle

Krinkle added a comment.EditedJul 28 2017, 3:52 AM

@Legoktm Indeed. For PHPCS to statically be aware of all classes seems unrealistic at this point. The Phan tool tries to do this, and does so quite well, but at a cost (complexity, speed). So in theory perhaps Phan could be made to validate @covers annotation in the same way that it also validates references to classes/methods in real code.

But ultimately, yeah, PHPUnit is the one evaluating these tags, and as you'd expect, it does in fact validate these already. The thing is, however, it only does so when running with coverage enabled. And while we run PHPUnit that way for small repositories (e.g. PHP libraries), we don't run PHPUnit that way for mediawiki-core, because it takes 2-3 hours to run PHPUnit tests for core when enabling coverage. Instead, we run a separate Jenkins job a few times a day. And that separate job does correctly detect invalid @covers values and fail the build accordingly. (But most people don't notice that.)

Given this task was filed based on an issue in an extension, there is a broader issue here: We can't run PHPUnit with coverage on extensions right now. To get coverage reports for extensions, we need to either run PHPUnit "from" the extension repo context somehow (instead of from core with core's phpunit.xml file), or somehow override the coverage whitelist when running tests for an extension.

In addition, once running PHPUnit with coverage is possible for extensions, we can probably do so by default for the test-pipeline jobs for extensions (so that we validate patches early, and also allows viewing Clover reports within Jenkins' UI).

Thanks for the information, than phpcs only can look for wrong tags like "@cover"[1], but not for wrong classes.

[1]https://phabricator.wikimedia.org/rMWda6235ce3a426effc7dc4d30a6c638c9ddd1886f#f714faad

Umherirrender triaged this task as Lowest priority.Jul 28 2017, 11:14 AM
Legoktm renamed this task from Check @covers against autoloader with codesniffer to Validate @covers tags during normal test runs.Dec 22 2017, 7:34 AM
Legoktm claimed this task.
Legoktm updated the task description. (Show Details)

Re-triaging under MediaWiki-Core-Testing since this is a PHPUnit issue, and not something easily doable in MW-CS.

Change 399775 had a related patch set uploaded (by Legoktm; owner: Legoktm):
[mediawiki/core@master] Verify that all @covers tags are sane when running tests

https://gerrit.wikimedia.org/r/399775

Change 399950 had a related patch set uploaded (by MaxSem; owner: MaxSem):
[mediawiki/extensions/EventLogging@master] Remove bogus @covers annotation

https://gerrit.wikimedia.org/r/399950

Change 399956 had a related patch set uploaded (by MaxSem; owner: MaxSem):
[mediawiki/extensions/CirrusSearch@master] Fix @covers annotations

https://gerrit.wikimedia.org/r/399956

Change 399956 merged by jenkins-bot:
[mediawiki/extensions/CirrusSearch@master] Fix @covers annotations

https://gerrit.wikimedia.org/r/399956

Change 399775 merged by jenkins-bot:
[mediawiki/core@master] Verify that all @covers tags are sane when running tests

https://gerrit.wikimedia.org/r/399775

Umherirrender closed this task as Resolved.Dec 29 2017, 8:45 PM

@covers used in tests extending MediaWikiTestCase are now checked.
Other extends of PHPUnit_Framework_TestCase can use the MediaWikiCoversValidator directly.