pjcj / Devel--Cover

Code coverage metrics for Perl
http://www.pjcj.net/perl.html
93 stars 89 forks source link

Really questionable design decision when target in @INC #332

Open EvanCarroll opened 8 months ago

EvanCarroll commented 8 months ago

If I do

PERL5LIB=wtf PERL5OPT=-MDevel::Cover /usr/bin/perl /wtf/foo.pm;

Nothing works: empty report generated.

----- ------ ------ ------ ------ ------ ------
File    stmt   bran   cond    sub   time  total
----- ------ ------ ------ ------ ------ ------
Total    n/a    n/a    n/a    n/a    n/a    n/a
----- ------ ------ ------ ------ ------ ------

However.... If I do,

cp /wtf/foo.pm /foo.pm
PERL5LIB=wtf PERL5OPT=-MDevel::Cover /usr/bin/perl /foo.pm;

Everything works. This is actually documented... HOWEVER Nothing works like this.

* If the file matches a RE given as a select option, it will be included
* Otherwise, if it matches a RE given as an ignore option, it won't be included
* Otherwise, if it is in one of the inc directories, it won't be included
* Otherwise, it will be included

This is supremely bizarre. Why?! Can't we change this behavior to either,

pjcj commented 8 months ago

Hello! The reason for working this way is that in general we're not interested in coverage for core perl modules or cpan modules. And, my suspicion initially, was that in general we wouldn't be interested in the coverage of any modules found in @INC directories. But, as you saw, there is a way to override this.

If we were to change this, what would you have expected to see from your first example - the same as for the second? Would you expect to see coverage for any core or cpan modules used? What about any other modules in /wtf? Or any modules in other directories added to @INC?

EvanCarroll commented 6 months ago

If we were to change this, what would you have expected to see from your first example - the same as for the second?

Yes, the same as the second.

Would you expect to see coverage for any core or cpan modules used?

My answer there would be that I would not personally use this to hide coverage of core modules. I would make the utility behave consistently everywhere. If you want to hide the coverage from modules in @INC, even when explicitly targeted I would add another option that does that --unless-in-inc. But it's still not clear to me why anyone would ever want that.

pjcj commented 6 months ago

But you didn't see any core or cpan coverage in your second example, did you? Hence my followup questions.

I'm not really sure what you meant by "everything works" in the second example because you would usually run coverage on your test suite rather than on a module. So perhaps there's something else going on here. Is foo.pm just a trivial test module?

I'm not really convinced that you do want to see core and cpan coverage along with coverage of your own modules. Apart from being expensive to calculate, what are you going to do with this information?

Well, perhaps you do want that, but I'm fairly sure that most people don't. That's why things are the way they are. But if you do want coverage for absolutely everything it's fairly easy to get - use -select .

EvanCarroll commented 6 months ago

No, I don't. In the second example I don't see any cpan coverage. But that's because presumably I'm targeting a specific file. Why would I see cpan coverage in it. Here is my coverage report,

---------- ------ ------ ------ ------ ------ ------
File         stmt   bran   cond    sub   time  total
---------- ------ ------ ------ ------ ------ ------
/Acme.pm   30.6    0.0    0.0   44.7  100.0   20.9
Total        30.6    0.0    0.0   44.7  100.0   20.9
---------- ------ ------ ------ ------ ------ ------

The problem is Acme.pm is in /usr/local/acme which is also in @INC. This is not a defensible development strategy, but we need to support it. How can I target files in /usr/local/acme?

pjcj commented 3 months ago

It's been a while since I have replied here. Part of the reason is because I lost enthusiasm every time I read the title. But also I'm really not sure what you are trying to do. Or perhaps why.

Is your foo.pm a modulino? That's the only use case I can think of for doing what you are doing. If so, why is its directory in $PERL5LIB? If not, please come back with a real example or, at least, real code so that I can understand a real use case.

Devel::Cover is primarily designed for the case where you have a distribution to be tested, probably in lib and tests to do that, probably in t. These are probably in the same repository. Though none of that has to be true, and the module can be used in many ways. I'm not against changing things to accommodate other use cases, but I need to be able to understand the use case first, and I don't get that understanding from what you have described so far.

EvanCarroll commented 3 months ago

I work for cPanel. cPanel puts it's code, binary, and tests /usr/local/cpanel which is in PERL5LIB. I want to test the coverage of the code in a pipeline.

I'm not defending cPanel's practice. It's wrong. But it's also not against any rule of Perl. It's just wrong by convention. I'm only suggesting that perhaps the library shouldn't impose a convention the language itself doesn't impose.

pjcj commented 3 months ago

Two things:

  1. If you're going somewhat against convention in your code layout I don't think it's too surprising that you may need to go somewhat against convention when using tools against that code layout.

  2. I think you're asking for the equivalent of $DEVEL_COVER_OPTIONS=-select,. to be the default. I'm not convinced you actually do want that, but if you really do it's fairly easy to just set that environment variable. You might prefer $DEVEL_COVER_OPTIONS=-select,/usr/local/cpanel but even then you might want to be more selective and not collect coverage for the tests themselves, for example.

In any case, I think the default is correct for most people who use more standard layouts. Try using $DEVEL_COVER_OPTIONS=-select,. and I think you may agree. But if you don't, you have a fairly easy way to get the behaviour you want.