After generating code coverage reports with

./vendor/bin/phpunit --coverage-html ~/d8CodeCoverage/

I see some of what we report is nonsense :-

For example the report states there is zero test coverage for the file EntityViewControllerTest.php

Test classes by definition cannot have tests!

It also makes no sense to talk about code coverage for the 89 files that end *TestBase.php

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug fix /Feature... it brings the code coverage report closer to reality, that is it drastically reduces the stated number of classes without test coverage!
Issue priority Normal
Unfrozen changes This not a code change
Disruption No disruption
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

martin107’s picture

Issue summary: View changes
Status: Active » Needs review
Issue tags: +Quick fix
FileSize
473 bytes

Rewrote issue summary.

Supplied working patch

martin107’s picture

Title: Aggregate code coverage statistics are overly pesimistic » Alter php.xml.dist to remove test classes from code coverage reports.
Issue summary: View changes
FileSize
530 bytes

When I now inspect the code coverage report I see these files with the follow suffix

*TestBase.php

have floated to the top of the zero test coverage tables. Which is again gibberish

find . -name '*TestBase.php' | wc -l

tells me that there are 89 files we should also be ignoring for test coverage.

martin107’s picture

Issue summary: View changes
dawehner’s picture

+++ b/core/phpunit.xml.dist
@@ -33,6 +33,11 @@
+      <!-- by defintion Test classes have no tests. -->
+      <exclude>
+        <directory suffix="Test.php">./</directory>
+        <directory suffix="TestBase.php">./</directory>
+      </exclude>
      </whitelist>

What do you think about excluding vendor itself?

martin107’s picture

What about vendor ....I think we have that covered.

Looking at the coverage report ... I see the top level directories.

1) core
2) modules
3) sites

when I descend into core

My choices are

lib
module

vendor is excluded.

but back to the top level I can lookup the code coverage for

drupal/sites/default/default.settings.php

- so that is wrong .. but a minor correction as we are only talking about a handful a php files.
hundred of files have been excluded by the patch as it stands...

Its getting late ... I will fix that up tomorrow..

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Mh, I have seen vendor being in there at some point. Never mind.

martin107’s picture

I woke up this morning thinking, yeah I must have missed it, it got to still be in there....

The project risk section should be used to highlight crappy code.

Suppose our biggest risk is in the vendor section I think that is something we need to know.

Someday we are going to need to swap out our own heartbleed thing...

I am planning to look at this wealth of data some more ... we can alway discuss the vendor thing in another issue:)

PS
Just wanted to clarify what I said above for vendor subdirectories the code complexity measure is a valid indicator ...the test coverage metric will not be.

martin107’s picture

Issue summary: View changes

Add beta evaluation.

Wim Leers’s picture

+++ b/core/phpunit.xml.dist
@@ -33,6 +33,11 @@
+      <!-- by defintion Test classes have no tests. -->

s/defintion/definition/

"by … tests." suggests it's a proper phrase, which means "by" should really be "By".

Both can be fixed upon commit.

martin107’s picture

FileSize
530 bytes
524 bytes

Thanks ... Ah it is my mess I should clean it up :)

Wim Leers’s picture

+++ b/core/phpunit.xml.dist
@@ -33,6 +33,11 @@
+      <!-- By defintion test classes have no tests. -->

Still … s/defintion/definition/

:P

martin107’s picture

FileSize
531 bytes
525 bytes

More haste less speed....

martin107--

Wim Leers’s picture

Perfect now. No worries, happens to all of us :)

The last submitted patch, 10: test-2458487-10.patch, failed testing.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed f76c925 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed f76c925 on 8.0.x
    Issue #2458487 by martin107: Alter php.xml.dist to remove test classes...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

Mile23’s picture

This issue is a regression against #2056607: /core/phpunit.xml.dist only finds top-level contrib modules leading to #2498531: phpunit_example tests not discovered in the commandline

Please let's revert this and find another solution so that contrib works.

Mile23’s picture