Ignore code coverage of private constructors of static classes

[morozov]

According to Scrutinizer, static classes have 0% code coverage since the only their executable code (constructor) is not executed. But it's not supposed to since it's just a way to prevent class instantiation.

Q	A
Type	improvement
BC Break	no
Fixed issues	none

Summary

Static class constructors are marked as @codeCoverageIgnore.

[Majkl578]

I really don't like production code polluted with similar annotations to hack test suites, coverage etc. :/
How about we add a test to cover these? Simply trying to instantiate these classes and expecting Error? Possibly also asserting that message matches ^Call to private \S+::__construct\(\) from invalid context\z.

[morozov]

Calling this method will most likely not make its code covered. Besides that, it's even more hacky in the way that instead of telling PHPUnit "I don't want to test this code because we're relying on a PHP language feature", we're trying to test the language feature itself. We don't write tests for type hints or other non-public class members.

A better solution would be to add these files to coverage blacklist in phpunit.xml but it would exclude entire files (not only methods) and would require copying this blacklist across all phpunit.xml replicas.

With that said, annotations seem the least of evils. I agree that we should avoid having such comments if possible (e.g. I didn't add any code style annotations in #3157 and let the build fail which is okay because of our incremental approach).

[Ocramius]

I don't see a reason to push our coverage up artificially: what is pushing this kind of patch? What is the gain?

Having 100% is surely a good aim, but I'd probably rather have a scenario that actually touches the code in question instead (not applicable here).

[morozov]

@Ocramius it's not pushing coverage up artificially. As I said, it's an exclusion of the code from being subject to coverage. Until there's another way to define a static class in PHP, we'll have to use a private constructor which cannot be executed by design and this is the only line of code in this class which can be executed in those classes.

It's not about 100% coverage, it's about having adequate metrics.

[Ocramius]

I'm fine with merging this, I just don't see a strong reason to annotate un-coverable code, heh...

[morozov]

So there are no objections against writing non-executable code in the first place? Only about the annotations? 😄

[Ocramius]

Correct, it's just about the annotations 🧐

[greg0ire]

Should a feature request be done to PHPUnit?

[morozov]

I'd love to see this handled by PHPUnit so that we don't have to do this ourselves but I don't see how PHPUnit would do that. What are the criteria for a method to be automatically excluded from code coverage? Should it be any empty private constructor? Not sure if PHPUnit is capable of detecting an empty method.