New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/branch coverage #400

Closed
wants to merge 18 commits into
base: master
from

Conversation

4 participants
@Maks3w
Copy link
Contributor

Maks3w commented Nov 4, 2015

This PR continue the work @sebastianbergmann made on feature/path-coverage branch.

Includes PRs #398 and #399

Close #380

TODO:

  • Clover XML Report. I won't add support for cond line types.

@Maks3w Maks3w force-pushed the Maks3w:feature/branch-coverage branch 4 times, most recently from 6bfe5a0 to 91387b8 Nov 4, 2015

@sebastianbergmann

This comment has been minimized.

Copy link
Owner

sebastianbergmann commented Nov 4, 2015

Thank you, @Maks3w, for this contribution. It will take a while for me to review this as I am very busy at the moment.

@Maks3w Maks3w changed the title WIP Feature/branch coverage Feature/branch coverage Nov 5, 2015

@Maks3w

This comment has been minimized.

Copy link
Contributor

Maks3w commented Nov 5, 2015

This PR is finish and ready for review. I suggest start for #398 and #399 for make the changeset more tiny.

@sebastianbergmann

This comment has been minimized.

Copy link
Owner

sebastianbergmann commented Nov 5, 2015

Yes, of course, I will look at #398 and #399 first. In the meantime it would help me (and others) if you could attach screenshots (maybe from the https://github.com/sebastianbergmann/money/ project) to this issue. Thanks!

@Maks3w

This comment has been minimized.

Copy link
Contributor

Maks3w commented Nov 5, 2015

These commits show the differences in the output respect master.

Maks3w@482cb3f
Maks3w@5e87574
Maks3w@4bc19d0
Maks3w@91387b8

Basically I've added the Path data close to the line statistics.

Now I'm playing with executing this in a real library project and I try to make it work. Seems does not recognize the paths but I don't know yet why.

@sebastianbergmann

This comment has been minimized.

Copy link
Owner

sebastianbergmann commented Nov 5, 2015

Ah, okay. So for now it's "only" textual. Still a good starting point. Do you also want to implement something like http://derickrethans.nl/images/content/paths-covered-mockup.png?

@Maks3w

This comment has been minimized.

Copy link
Contributor

Maks3w commented Nov 5, 2015

No sorry, too much. I'm trying to use this data for colorize paths in coveralls.io (using coverage 0/1 value instead hits per line)

@Maks3w

This comment has been minimized.

Copy link
Contributor

Maks3w commented Nov 5, 2015

I've found a bug related to namespaces. I'll fix it later.

@Maks3w

This comment has been minimized.

Copy link
Contributor

Maks3w commented Nov 5, 2015

Now works.

@derickr Any chance for add a whitelist coverage feature to XDebug? I think will save lots of memory because it now tracks all the PHPUnit framework stuff.

@sebastianbergmann

This comment has been minimized.

Copy link
Owner

sebastianbergmann commented Nov 5, 2015

@Maks3w http://bugs.xdebug.org/view.php?id=1144 is about pushing the filtering from PHPUnit down to Xdebug.

@sebastianbergmann

This comment has been minimized.

Copy link
Owner

sebastianbergmann commented Nov 5, 2015

Path coverage is always shown as 0% for me:

screenshot from 2015-11-05 15-20-03

@Maks3w

This comment has been minimized.

Copy link
Contributor

Maks3w commented Nov 5, 2015

@Maks3w

This comment has been minimized.

Copy link
Contributor

Maks3w commented Nov 5, 2015

About the HTML Mockup I suggest to do the same I expect to do with coveralls. Turn green or red each line if is covered by a branch or not.

Some tests have too many paths > 4096 which is impossible to print.

@Maks3w

This comment has been minimized.

Copy link
Contributor

Maks3w commented Nov 5, 2015

@sebastianbergmann I've restored the compatibility with Xdebug < 2.3.2. However I suggest force pathCoverage param from PHPUnit

@Maks3w Maks3w force-pushed the Maks3w:feature/branch-coverage branch from 82a4f23 to 235c50c Nov 5, 2015

@sebastianbergmann

This comment has been minimized.

Copy link
Owner

sebastianbergmann commented Nov 6, 2015

Please rebase against current master, thanks.

@sebastianbergmann

This comment has been minimized.

Copy link
Owner

sebastianbergmann commented Nov 6, 2015

BTW: Thank you for your contributions. I am happy that somebody else works on this code :-) I would be even happier if you would configure your Git client to properly store your name and email with your commits (see https://github.com/sebastianbergmann/phpunit/blob/master/CONTRIBUTING.md#workflow).

Keep up the good work!

@Maks3w Maks3w force-pushed the Maks3w:feature/branch-coverage branch from 235c50c to 7bb2c4c Nov 6, 2015

@Maks3w

This comment has been minimized.

Copy link
Contributor

Maks3w commented Nov 6, 2015

Rebased.

I don't understand what do you refer with configure my author data on Git. All my commits have a name and email.

@Maks3w

This comment has been minimized.

Copy link
Contributor

Maks3w commented Nov 6, 2015

Something is broken. Tests pass but resume is wrong.

@sebastianbergmann

This comment has been minimized.

Copy link
Owner

sebastianbergmann commented Nov 6, 2015

Maks3w <github.maks3w@virtualplanets.net> is what my Git client shows for your commits. I doubt that "Maks3w" is your real name :-) But no worries.

@Maks3w Maks3w force-pushed the Maks3w:feature/branch-coverage branch from 7bb2c4c to 8f476ba Nov 6, 2015

@Maks3w

This comment has been minimized.

Copy link
Contributor

Maks3w commented Nov 6, 2015

@sebastianbergmann Done. Was a problem while skipping path coverage if xdebug didn't support

@sebastianbergmann

This comment has been minimized.

Copy link
Owner

sebastianbergmann commented Nov 6, 2015

Re: #400 (comment)

Sure, PHPUnit will need a new configuration option (off by default, IMHO) to control path coverage.

@Maks3w

This comment has been minimized.

Copy link
Contributor

Maks3w commented Nov 6, 2015

No, Maks3w it's not my real name and my skin is not yellow :-)

@Maks3w

This comment has been minimized.

Copy link
Contributor

Maks3w commented Nov 6, 2015

I've open sebastianbergmann/phpunit#1937 about path coverage switch on PHPUnit configuration

@Maks3w

This comment has been minimized.

Copy link
Contributor

Maks3w commented Nov 6, 2015

@Maks3w Maks3w referenced this pull request Nov 6, 2015

Closed

Path coverage switch #1938

@Maks3w Maks3w force-pushed the Maks3w:feature/branch-coverage branch 3 times, most recently from 6134834 to e9437eb Nov 6, 2015

@sebastianbergmann

This comment has been minimized.

Copy link
Owner

sebastianbergmann commented Nov 6, 2015

  • The test suite for PHP_CodeCoverage itself fails when path coverage is disabled
  • The HTML report must not have path coverage related columns etc. when path coverage is disabled
@Maks3w

This comment has been minimized.

Copy link
Contributor

Maks3w commented Nov 6, 2015

I know. I just did the lowest effort so the library still functional.

Do you think is needed to test both scenarios? (path on/off)

@Maks3w

This comment has been minimized.

Copy link
Contributor

Maks3w commented Nov 6, 2015

Exclude the path information in the HTML output will require duplicate the templates or develop a conditional statement the template.

Will also require duplicate all the test fixtures.

I think 0/0 could be a valid value as has been always in the Clover report (conditionals="0" coveredconditionals="0")

Probably the only thing we could do is paint the values (text and html outputs) with a disabled color (gray)

Restore backward compatibility
Enable pathCoverage by default only when supported

@Maks3w Maks3w force-pushed the Maks3w:feature/branch-coverage branch from e9437eb to 267336f Nov 6, 2015

@Maks3w

This comment has been minimized.

Copy link
Contributor

Maks3w commented Nov 6, 2015

I've rewritten the bc commit. Loops are enough safe for to not depend of the pathCoverage setting value.

@Maks3w Maks3w closed this Nov 9, 2015

@Maks3w Maks3w reopened this Nov 9, 2015

@Maks3w

This comment has been minimized.

Copy link
Contributor

Maks3w commented Nov 9, 2015

Tested with Xdebug 2.4.0beta1

@Maks3w Maks3w closed this Nov 9, 2015

@Maks3w Maks3w reopened this Nov 9, 2015

@derickr

This comment has been minimized.

Copy link

derickr commented Dec 8, 2015

How is this coming along?

@derickr

This comment has been minimized.

Copy link

derickr commented Dec 19, 2015

poke?

@Maks3w Maks3w referenced this pull request Feb 29, 2016

Closed

update from master #425

@sebastianbergmann

This comment has been minimized.

Copy link
Owner

sebastianbergmann commented Apr 21, 2016

Closing #400 and #425 because they have run out-of-sync with master. Please coordinate, @Maks3w and @posey2010, and send a new pull request. Thanks!

@Maks3w

This comment has been minimized.

Copy link
Contributor

Maks3w commented Apr 21, 2016

@sebastianbergmann I won't spent more time on this for nothing. This PR has been open for 4 months. If you won't merge it why open a new PR

@sebastianbergmann

This comment has been minimized.

Copy link
Owner

sebastianbergmann commented Apr 21, 2016

I did not merge it because there were "competing" / conflicting merge requests and I got confused. I hope that a new pull request will be less confusing. Sorry :-(

@elynnaie

This comment has been minimized.

Copy link

elynnaie commented Aug 21, 2017

Please don't give up on this! Having branch and path coverage would be extraordinarily helpful!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment