[PhpUnitBridge] Added a CoverageListener to enhance the code coverage report #23149

lyrixx · 2017-06-12T14:46:47Z

Q	A
Branch?	3.4
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	-
License	MIT
Doc PR	symfony/symfony-docs#8416

The code coverage computed by PHPUnit is not very accurate by default as it marks a line as tested as soon as it has been executed.
For example, if you have two classes A and B where A is using B and you write test only for the class A then the class B will be marked as tested.

You can fix this issue by adding @covers A on top of the class ATest, but it's a bit boring.

This Listener add this annotation on each test if it's applicable:

If an annotation already exists, we do nothing.
We try to find the SUT thanks to the Test class name, if it does not exist, we do nothing

If you wan to see it in action: https://github.com/lyrixx/phpunit-auto-cover

The PR is not finished, I think we could add this listener to symfony itself.

What do you think?

stof · 2017-06-12T14:52:44Z

@lyrixx is there a way to opt-out the guessing for some test cases (smoke tests used to cover things through functional tests for instance) ?

nicolas-grekas · 2017-06-12T14:56:55Z

I like it :)

lyrixx · 2017-06-12T14:59:21Z

@stof As soon as you add an annotation on your test class, the listener adds nothing.
Thus, if it's really needed, we could make this listener configurable.

dunglas · 2017-06-14T18:24:40Z

Nice!

lyrixx · 2017-06-22T13:19:34Z

This PR did not get so much attraction. IMHO, it's nice.

Should I finished it or it will be a waste of time?
(This is really easy, as we have to deal with many PHPUnit version (PSR4 vs Pear))

Simperfit · 2017-06-22T13:38:23Z

IMO It should be finished, I may use it ;)

sebastianbergmann · 2017-06-22T14:54:52Z

A test listener should only listen. In the future, it will only be able to listen.

The proper solution to this problem is to follow best practices and set forceCoversAnnotation="true" in your PHPUnit configuration.

stof · 2017-06-22T14:57:15Z

@sebastianbergmann well, this listener is an attempt at guessing the covered class based on a convention instead of forcing to put the annotation everywhere. What would be the proper way to implement such convention-based coverage configuration in PHPUnit ?

fabpot · 2017-06-22T15:04:46Z

My first reaction was: why is it related to Symfony? Why isn't it something part of PHPUnit itself it that's something useful?

lyrixx · 2017-06-22T15:09:22Z

@fabpot that's a good question. I guess I created the PR in Symfony because I contribute more to Symfony than to PHPUnit. And we already have some listener in Symfony.

medinae · 2017-06-22T17:26:09Z

Useful feature 👍 I agree with @lyrixx as others similar listeners are existing in SF.

fabpot · 2017-07-03T07:22:03Z

@lyrixx Can you add some information in the documentation?

lyrixx · 2017-07-04T08:19:57Z

@fabpot Keep in mind this PR is really not ready. I have to make it work for different PHPUnit version.

anyway, what documentation are you referring to? (local PHP doc or the symfony/symfony-docs repo?)

lyrixx · 2017-07-04T12:58:57Z

I updated the PR to support PHPUnit 5.* and 6.*.

Locally, I also updated all phpunit.xml.dist to add the listener, but the phpunit bridge is not required. Should I update them too?

keradus · 2017-07-09T19:01:03Z

IMO, if we already try to figure out what is the expected class to be tested, we could already put it into code and do it only once, commit and forget about it. no need to do it all the times on runtime.
Then, it could be watched to have the annotation for new tests, eg with suggestion of @sebastianbergmann and PHP CS Fixer/fabbot php_unit_test_class_requires_covers rule

anyway, big 👍 for strict coverage

nicolas-grekas · 2017-07-19T06:29:10Z

Would it be possible to add a test, by mean of a phpt for example? this would ensure that we can detect whenever phpunit changes its internals and breaks the feat.
Since this is purely opt-in, I'm personally OK to accept it. With a doc PR :)

keradus · 2017-07-19T07:00:30Z

@nicolas-grekas , why do it in runtime instead of actually store it as annotation at all test files, as I suggest ?

nicolas-grekas · 2017-07-19T07:10:08Z

@keradus the purpose of this PR is exactly to leverage a convention in order to not have to add @cover to all test files... because it's boring to repeat yourself in each and every test class I guess? :)

keradus · 2017-07-19T08:36:45Z

explicit is better than implicit ;)
on top of that, if automate guess wrongly, you don't know (as you don't even see what was chosen) and collect data not how you wish to. if for some reason you wish to have some non-standard cover restriction in given test, guessing on the fly won't help.

lyrixx · 2017-07-19T12:34:03Z

if automate guess wrongly,

how is it possible?

and BTW this feature is opt-in ; So if you don't like it, don't use it ;)

keradus · 2017-07-19T15:25:53Z

how is it possible?

it could as there is no tests, so if one day there will be some newer autoloader spec, dir structure or anything, it could silently fail matching src class for given test class

and BTW this feature is opt-in ; So if you don't like it, don't use it ;)

of course, but still I think it's nice to think about other possible solutions.
on one side you have logic hidden, on other you have it explicit

lyrixx · 2017-09-21T11:02:40Z

Arf, I don't know how to exclude my "Fixture" test from the Symfony test suite. I have try many things without success :/

… SUT

lyrixx · 2017-09-22T10:43:58Z

Hi.
So:

I managed to fix test on travis & appveyor
fabbot is wrong
I addressed issues raised by @keradus
I added a note in the CHANGELOG

lyrixx · 2017-09-22T11:09:48Z

symfony/symfony-docs#8416

nicolas-grekas · 2017-09-26T11:53:48Z

Still 👍 on my side, ping @symfony/deciders

dunglas · 2017-09-26T11:54:33Z

👍

nicolas-grekas · 2017-09-26T14:40:00Z

@lyrixx can you add an entry in the CHANGELOG file please?

lyrixx · 2017-09-26T16:10:06Z

Oups, I forgot to push a commit. Fixed.

xabbuh · 2017-09-26T19:49:57Z

src/Symfony/Bridge/PhpUnit/CoverageListener.php

+    class_alias('Symfony\Bridge\PhpUnit\Legacy\CoverageListener', 'Symfony\Bridge\PhpUnit\CoverageListener');
+// Using an early return instead of a else does not work when using the PHPUnit
+// phar due to some weird PHP behavior (the class gets defined without executing
+// the code before it and so the definition is not properly conditional)


fabbot does not seem to be happy with the comments not being indented

It's a false positive. I ignored it.

How is that a false positive?

fabpot · 2017-09-26T23:19:39Z

Thank you @lyrixx.

…e code coverage report (lyrixx) This PR was squashed before being merged into the 3.4 branch (closes #23149). Discussion ---------- [PhpUnitBridge] Added a CoverageListener to enhance the code coverage report | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | symfony/symfony-docs#8416 --- The code coverage computed by PHPUnit is not very accurate by default as it marks a line as tested as soon as it has been executed. For example, if you have two classes A and B where A is using B and you write test only for the class A then the class B will be marked as tested. You can fix this issue by adding `@covers A` on top of the class ATest, but it's a bit boring. This Listener add this annotation on each test if it's applicable: * If an annotation already exists, we do nothing. * We try to find the SUT thanks to the Test class name, if it does not exist, we do nothing --- If you wan to see it in action: https://github.com/lyrixx/phpunit-auto-cover --- The PR is not finished, I think we could add this listener to symfony itself. What do you think? Commits ------- e17206d [PhpUnitBridge] Added a CoverageListener to enhance the code coverage report

…rixx, javiereguiluz) This PR was merged into the 3.4 branch. Discussion ---------- [PHPUnitBridge] Added docs for the CoverageListener For symfony/symfony#23149 Commits ------- c21ab16 Fixed code indentation e78804b Minor rewords 81365f3 [PHPUnitBridge] Added docs for the CoverageListener

carsonbot added Status: Needs Review PhpUnitBridge Feature labels Jun 12, 2017

lyrixx force-pushed the phpunit-auto-cover branch from 449e406 to 0afa3cf Compare June 12, 2017 14:47

nicolas-grekas added this to the 3.4 milestone Jun 12, 2017

lyrixx force-pushed the phpunit-auto-cover branch from 0afa3cf to 16fe7dc Compare June 12, 2017 15:10

lyrixx force-pushed the phpunit-auto-cover branch 3 times, most recently from 652c5ef to 8e4f1aa Compare July 4, 2017 09:37

lyrixx force-pushed the phpunit-auto-cover branch from d5f4199 to 110d57c Compare September 21, 2017 10:47

Added test for the CoverageListener

30381f4

lyrixx force-pushed the phpunit-auto-cover branch from 110d57c to 30381f4 Compare September 21, 2017 11:03

lyrixx added 4 commits September 22, 2017 11:33

Added an option to throw a warning if the SUT solver can not find the…

e764cfe

… SUT

Added more tests + support more annotations

b99c193

Better FS structure & avoid to run fixture test in the main test suite

87aebab

Added a BC layer for older PHPUnit

239bc35

lyrixx force-pushed the phpunit-auto-cover branch from 8b4223d to 239bc35 Compare September 22, 2017 10:29

lyrixx added 3 commits September 22, 2017 12:30

'Fixed' tests on HHVM

a710cf2

Fixed tests on Windows

1e92acd

Added note in CHANGELOG

aad02e6

lyrixx mentioned this pull request Sep 22, 2017

[PHPUnitBridge] Added docs for the CoverageListener symfony/symfony-docs#8416

Merged

javiereguiluz approved these changes Sep 26, 2017

View reviewed changes

Fixed issue with older PHPUnit Version

b80489f

xabbuh reviewed Sep 26, 2017

View reviewed changes

fabpot approved these changes Sep 26, 2017

View reviewed changes

fabpot closed this Sep 26, 2017

lyrixx deleted the phpunit-auto-cover branch September 27, 2017 08:22

This was referenced Oct 18, 2017

Release v3.4.0-BETA1 #24609

Merged

Release v4.0.0-BETA1 #24612

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PhpUnitBridge] Added a CoverageListener to enhance the code coverage report #23149

[PhpUnitBridge] Added a CoverageListener to enhance the code coverage report #23149

lyrixx commented Jun 12, 2017 •

edited

Loading

stof commented Jun 12, 2017

nicolas-grekas commented Jun 12, 2017

lyrixx commented Jun 12, 2017 •

edited

Loading

dunglas commented Jun 14, 2017

lyrixx commented Jun 22, 2017 •

edited

Loading

Simperfit commented Jun 22, 2017

sebastianbergmann commented Jun 22, 2017 •

edited

Loading

stof commented Jun 22, 2017

fabpot commented Jun 22, 2017

lyrixx commented Jun 22, 2017

medinae commented Jun 22, 2017

fabpot commented Jul 3, 2017

lyrixx commented Jul 4, 2017

lyrixx commented Jul 4, 2017

keradus commented Jul 9, 2017 •

edited

Loading

nicolas-grekas commented Jul 19, 2017

keradus commented Jul 19, 2017

nicolas-grekas commented Jul 19, 2017 •

edited

Loading

keradus commented Jul 19, 2017

lyrixx commented Jul 19, 2017 •

edited

Loading

keradus commented Jul 19, 2017

lyrixx commented Sep 21, 2017

lyrixx commented Sep 22, 2017 •

edited

Loading

lyrixx commented Sep 22, 2017

nicolas-grekas commented Sep 26, 2017

dunglas commented Sep 26, 2017

nicolas-grekas commented Sep 26, 2017

lyrixx commented Sep 26, 2017

xabbuh Sep 26, 2017

lyrixx Sep 26, 2017

fabpot Sep 26, 2017

fabpot commented Sep 26, 2017

[PhpUnitBridge] Added a CoverageListener to enhance the code coverage report #23149

[PhpUnitBridge] Added a CoverageListener to enhance the code coverage report #23149

Conversation

lyrixx commented Jun 12, 2017 • edited Loading

stof commented Jun 12, 2017

nicolas-grekas commented Jun 12, 2017

lyrixx commented Jun 12, 2017 • edited Loading

dunglas commented Jun 14, 2017

lyrixx commented Jun 22, 2017 • edited Loading

Simperfit commented Jun 22, 2017

sebastianbergmann commented Jun 22, 2017 • edited Loading

stof commented Jun 22, 2017

fabpot commented Jun 22, 2017

lyrixx commented Jun 22, 2017

medinae commented Jun 22, 2017

fabpot commented Jul 3, 2017

lyrixx commented Jul 4, 2017

lyrixx commented Jul 4, 2017

keradus commented Jul 9, 2017 • edited Loading

nicolas-grekas commented Jul 19, 2017

keradus commented Jul 19, 2017

nicolas-grekas commented Jul 19, 2017 • edited Loading

keradus commented Jul 19, 2017

lyrixx commented Jul 19, 2017 • edited Loading

keradus commented Jul 19, 2017

lyrixx commented Sep 21, 2017

lyrixx commented Sep 22, 2017 • edited Loading

lyrixx commented Sep 22, 2017

nicolas-grekas commented Sep 26, 2017

dunglas commented Sep 26, 2017

nicolas-grekas commented Sep 26, 2017

lyrixx commented Sep 26, 2017

xabbuh Sep 26, 2017

Choose a reason for hiding this comment

lyrixx Sep 26, 2017

Choose a reason for hiding this comment

fabpot Sep 26, 2017

Choose a reason for hiding this comment

fabpot commented Sep 26, 2017

lyrixx commented Jun 12, 2017 •

edited

Loading

lyrixx commented Jun 12, 2017 •

edited

Loading

lyrixx commented Jun 22, 2017 •

edited

Loading

sebastianbergmann commented Jun 22, 2017 •

edited

Loading

keradus commented Jul 9, 2017 •

edited

Loading

nicolas-grekas commented Jul 19, 2017 •

edited

Loading

lyrixx commented Jul 19, 2017 •

edited

Loading

lyrixx commented Sep 22, 2017 •

edited

Loading