Add covers annotation support #47

TiMESPLiNTER · 2021-04-21T09:27:40Z

Implements #46

It's a PoC to see if it's way we want to add support for @covers. It actually works but of course the code needs more work and we need tests.

I pulled in the code-unit package and grabbed the code which is needed from the PHPUnit framework to support the @covers functionality. I think it's better than requiring the whole phpunit/phpunit framework just to get covers annotation support when actually only a few hundred lines of code are required.

Maybe we should move the whole code I added into a separate package and just suggest this one instead? Something like friends-of-phpspec/phpspec-code-coverage-annotation-support

Signed-off-by: TiMESPLiNTER <[email protected]>

jaylinski

Looking good. 👍

Since this generates no overhead for "normal" users and works in a backwards-compatible way, I would approve this.

Of course, as you said, it still needs some work regarding code-style, tests and documentation.

@drupol @shulard Opinions?

composer.json

src/Listener/CodeCoverageListener.php

TiMESPLiNTER · 2021-04-23T09:58:40Z

I put some more effort into it and made grumphp happy. I'm not sure what we should cover of the new code as I see so far there's not a very high percentage of code coverage... are there any guidelines?

jaylinski · 2021-04-23T18:31:33Z

Nope, there are no guidelines for testing, but there is an open issue: #8

Testing this feature will be kind of hard, since we would have to install the custom (suggested) packages in our CI job and add integration tests... 🤔 I guess you can leave it untested right now.

TiMESPLiNTER · 2021-04-27T15:42:51Z

Nope, there are no guidelines for testing, but there is an open issue: #8

Testing this feature will be kind of hard, since we would have to install the custom (suggested) packages in our CI job and add integration tests... 🤔 I guess you can leave it untested right now.

Alright. Then I would say this PR is complete. Should I remove the Docker stuff I created to test this lib or might they be useful for someone else?

jaylinski · 2021-05-29T16:20:01Z

@drupol @shulard If you have some time, please have a look at this PR.

shulard

Hello @TiMESPLiNTER !

Thank you for this PR, I made a first review regarding the global behaviour and questions regarding the PR. However I'll need to check deeper in the implementation to be sure that I understand how you use it 😄. It'll be another review soon.

shulard · 2021-05-31T07:51:51Z

src/CodeCoverageExtension.php

-            $listener = new CodeCoverageListener($consoleIO, $codeCoverage, $codeCoverageReports, $skipCoverage);
+            $coversAnnotationUtil = null;
+
+            if (class_exists('SebastianBergmann\CodeUnit\InterfaceUnit')) {


Do we really want to activate it automatically based on an external class ? I think that if we add a new configuration flag it'll be cleaner. We don't have any control on the CodeUnit interface names and (even if it's a stable API) might not rely on it directly in such test.

If the user forgot to add the correct package to its composer.json file it'll get an error. We must document that error to avoid misunderstanding.

Imo we don't need a flag to activate it explicitly. If you don't use the @covers annotations the behavior is as if there would be no support for @covers and a test class marks all code as covered it executes (as it does already as of now). Only if you put @covers annotations on classes and methods it starts covering only the stuff mentioned in the annotation.

So just by installing the code-unit package doesn't change any behavior of this package until you explicitly start writing @covers annotations and then it only changes the behavior of the class and/or method that has the annotation. All other test classes which still don't have an annotation will still behave the same as before this PR.

shulard · 2021-05-31T07:53:05Z

src/Exception/CodeCoverageException.php

+
+use Exception;
+
+class CodeCoverageException extends Exception


Haven't we removed this exception in the past ? @drupol what do you think ?

I added it for the additional annotation stuff, I could maybe rename it to CodeCoverageAnnotationException if this makes more sense for you?

shulard · 2021-05-31T07:59:58Z

docker/Dockerfile

@@ -0,0 +1,15 @@
+FROM php:7.3-cli-alpine3.12


Are this DockerFile and docker-compose.yml only required for the local development usage ? I think it can be useful but we must ask some questions :

Is 7.3 the correct PHP version ?

Do we need to allow testing locally with all the supported PHP version to help tracking bugs ?

Why using a docker-compose and not only the DockerFile with a docker run ... ?

Is Alpine Linux the correct distribution to use ?

In the GitHub action configuration we are testing the code against a large combination of PHP version. However it's not an exhaustive list only a large one.

Helping developers to work on this lib locally is nice but we need to be sure that this env will match our requirements. Also I think that those changes must not be here in this PR, maybe in another one regarding the development environment. Documentation must also be written to explain how to use the choosen tools.

Yes it's only required for local development. Basically I did it so I don't have to install any PHP extensions on my host but can spin up a local dev/test environment within a few minutes where I can run the unit tests (which need at least xdebug or pcov).

I used PHP 7.3 because it's the minimum requirement of this package (https://github.com/friends-of-phpspec/phpspec-code-coverage/blob/master/composer.json#L41). Of course we need to test it against many other versions but at least I immediately realize during development whether I use a feature that is not available in PHP 7.3 and if the code works on the minimum required PHP version. BC in PHP is quite good so all changes should then run on >7.3 versions smoothly as well.

But I will move it out into another PR.

shulard · 2021-05-31T08:01:01Z

composer.json

        "vimeo/psalm": "^4.7"
    },
    "suggest": {
        "ext-pcov": "Install PCov extension to generate code coverage.",
-        "ext-xdebug": "Install Xdebug to generate phpspec code coverage."
+        "ext-xdebug": "Install Xdebug to generate phpspec code coverage.",
+        "sebastian/code-unit": "Install code-unit to support @covers annotations in tests."


Maybe adding a not in the README about the @covers support and how to use it can help users.

jaylinski · 2022-10-07T16:08:19Z

@TiMESPLiNTER Sorry for not replying for so long. Is this PR still relevant for you?

TiMESPLiNTER added 3 commits April 21, 2021 10:31

Add covers annotation support

38a617e

Signed-off-by: TiMESPLiNTER <[email protected]>

Remove ext-json dev requirement

767c238

Signed-off-by: TiMESPLiNTER <[email protected]>

Small fix

cb95c2d

Signed-off-by: TiMESPLiNTER <[email protected]>

TiMESPLiNTER mentioned this pull request Apr 21, 2021

Support @covers annotation #46

Open

jaylinski reviewed Apr 21, 2021

View reviewed changes

composer.json Show resolved Hide resolved

src/Listener/CodeCoverageListener.php Outdated Show resolved Hide resolved

Makegrumphp happy

8d64964

shulard requested changes May 31, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add covers annotation support #47

Add covers annotation support #47

TiMESPLiNTER commented Apr 21, 2021 •

edited

Loading

jaylinski left a comment •

edited

Loading

TiMESPLiNTER commented Apr 23, 2021 •

edited

Loading

jaylinski commented Apr 23, 2021

TiMESPLiNTER commented Apr 27, 2021

jaylinski commented May 29, 2021

shulard left a comment

shulard May 31, 2021

TiMESPLiNTER Jun 1, 2021

shulard May 31, 2021

TiMESPLiNTER Jun 1, 2021

shulard May 31, 2021

TiMESPLiNTER Jun 1, 2021 •

edited

Loading

shulard May 31, 2021

jaylinski commented Oct 7, 2022

Add covers annotation support #47

Are you sure you want to change the base?

Add covers annotation support #47

Conversation

TiMESPLiNTER commented Apr 21, 2021 • edited Loading

jaylinski left a comment • edited Loading

Choose a reason for hiding this comment

TiMESPLiNTER commented Apr 23, 2021 • edited Loading

jaylinski commented Apr 23, 2021

TiMESPLiNTER commented Apr 27, 2021

jaylinski commented May 29, 2021

shulard left a comment

Choose a reason for hiding this comment

shulard May 31, 2021

Choose a reason for hiding this comment

TiMESPLiNTER Jun 1, 2021

Choose a reason for hiding this comment

shulard May 31, 2021

Choose a reason for hiding this comment

TiMESPLiNTER Jun 1, 2021

Choose a reason for hiding this comment

shulard May 31, 2021

Choose a reason for hiding this comment

TiMESPLiNTER Jun 1, 2021 • edited Loading

Choose a reason for hiding this comment

shulard May 31, 2021

Choose a reason for hiding this comment

jaylinski commented Oct 7, 2022

TiMESPLiNTER commented Apr 21, 2021 •

edited

Loading

jaylinski left a comment •

edited

Loading

TiMESPLiNTER commented Apr 23, 2021 •

edited

Loading

TiMESPLiNTER Jun 1, 2021 •

edited

Loading