Skip to content

Add failing test for parent constructor call coverage #965

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

Closed

Conversation

romm
Copy link

@romm romm commented Dec 5, 2022

Since a86b331 has been commited, a call to an exception's parent constructor will not mark the $code argument as covered.

I guess adding back \PhpParser\Node\Scalar to the list of \SebastianBergmann\CodeCoverage\StaticAnalysis\ExecutableLinesFindingVisitor::isExecutable would not be a correct fix as it would break what @Slamdunk did in a86b331. I do not know the codebase enough to be able to bring a correct fix, so I start by adding a failing test and would love to have some feedback on how to do it properly.

For reference, I ran into this issue when using Infection; see infection/infection#1734.


I rarely contribute to the PHPUnit ecosystem, so I'll take this opportunity to thank the contributors for bringing such a reliable testing framework to PHP. 🤗

@Slamdunk
Copy link
Contributor

Slamdunk commented Dec 6, 2022

Only the complete rewritten logic of #964 can have good result for Infection, see #959 and infection/infection#1750

@romm
Copy link
Author

romm commented Dec 6, 2022

There seems to be a lot of work done there, good job and thank you.

I'll subscribe to these issues and see where it's going and if I can help somehow.

@mvorisek
Copy link
Contributor

mvorisek commented Dec 6, 2022

the problem is no coverage line (covered/uncovered, simply no data) for line 8 and 9, right?

if this has something do to with parent constructor please explain, I do not understand the problem

please post here what data xdebug output, what you expect and what is output by this lib

@romm
Copy link
Author

romm commented Dec 14, 2022

Hi @mvorisek sorry I didn't notice your comment; this issue is solved with #964 anyway, thanks for your interest!

@romm romm closed this Dec 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants