This is a followup to

https://www.drupal.org/project/drupal/issues/2974911#comment-12680705

One minor issue I noticed: I was seeing code coverage results for the src/Tests directory

What this highlights is that this issue was not 100% successful

#2478115: 8% of the files having zero test coverage are incorrectly listed.

The long and the short of this issue is that is a more comprehensive exclusion pattern is needed.

In addition to a case sensitivity issue, classes of this pattern are also incorrectly showing up in the test results.

./modules/simpletest/src/WebTestBase.php

      <!-- Exclude all test modules, tests etc -->
      <exclude-pattern>*/tests/*</exclude-pattern>
      <exclude-pattern>*/Tests/*</exclude-pattern>

Plus patterns for any obvious test classes and traits.

CommentFileSizeAuthor
#10 interdiff-3062122-4-10.txt1.18 KBmartin107
#10 3062122-10.patch1005 bytesmartin107
#4 3062122-4.patch562 bytesmartin107
#2 3062122-2.patch466 bytesmartin107
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

martin107 created an issue. See original summary.

martin107’s picture

martin107 CreditAttribution: martin107 as a volunteer commented
Assigned: martin107 » Unassigned
Issue summary: View changes
FileSize
3062122-2.patch466 bytes

patch

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude as a volunteer and at ezCompany commented
Status: Active » Needs work 
+++ b/core/phpunit.xml.dist
@@ -66,6 +66,8 @@
+      <exclude-pattern>*/simpletests/*</exclude-pattern>

The IS says excluding simpletest dir, so this has a trailing 's' too much.

I also think that we shouldn't exclude the simpletest dir as a whole since it also contains non-test code like the testing UI, which should be part of a coverage report.

martin107’s picture

martin107 CreditAttribution: martin107 as a volunteer commented
Status: Needs work » Needs review
FileSize
3062122-4.patch562 bytes

I also think that we shouldn't exclude the simpletest dir as a whole since it also contains non-test code like the testing UI, which should be part of a coverage report.

Ok looking at a sample of the failing tests

ResourceTestBase
FieldKerenelTest
ResourceResponseTestTrait

This is a better response

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude as a volunteer and at ezCompany commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

@martin107 yeah that makes sense, updated the IS a bit to reflect the proposed change

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented

exclude-pattern is probably old style. It's not documented in PHPUnit 6 (see https://phpunit.de/manual/6.5/en/appendixes.configuration.html), and actually throws a warning in PHPUnit 7 (see #2950132: Support PHPUnit 7 optionally in Drupal 8, while keeping support for ^6.5).

Whitelisting Files for Code Coverage

The element and its children can be used to configure the whitelist for the code coverage reporting.

<filter>
  <whitelist processUncoveredFilesFromWhitelist="true">
    <directory suffix=".php">/path/to/files</directory>
    <file>/path/to/file</file>
    <exclude>
      <directory suffix=".php">/path/to/files</directory>
      <file>/path/to/file</file>
    </exclude>
  </whitelist>
</filter>
martin107’s picture

martin107 CreditAttribution: martin107 as a volunteer commented
Assigned: Unassigned » martin107
Status: Reviewed & tested by the community » Needs work

mondrake++

Thank you for the information -- about Phpunit7

It is great to have many eyes look things over.

I will sort this out.

martin107’s picture

martin107 CreditAttribution: martin107 as a volunteer commented

For the record I did the last round of code coverage on

./vendor/bin/phpunit --version
PHPUnit 6.5.14 by Sebastian Bergmann and contributors.

When I try this in my phpunit.xml ( see below )

I still see these inappropriate items ls in the Project risk section

Drupal\Tests\jsonapi\Functional\ResourceTestBase
Drupal\block\Tests\BlockTestBase
Drupal\content_translation\Tests\ContentTranslationTestBase
ResourceResponseTestTrait

back in Nov 2016
https://www.drupal.org/project/drupal/issues/2478115#comment-11758884

I did a little stackoverflow search and noted all the problems people were having to get wildcards working.
The response was always "use a modern phpunit version".

I was hoping our push to use updated versions via composer would resolve these issues. Back then I settled on the exclude-pattern -- That is not longer appropriate.

It takes a long time to iterate - I am thinking maybe I should wait until this patch is in

#2974911: Allow run-tests.sh to generate coverage reports

which will make things faster.

Maybe others will read the documentation differently and see that I am making a stupid mistake

  <!-- Filter for coverage reports. -->
  <filter>
    <whitelist>
      <directory>./includes</directory>
      <directory>./lib</directory>
      <directory>./modules</directory>
      <directory>../modules</directory>
      <directory>../sites</directory>
      <!-- Exclude all test modules, tests etc -->
      <exclude>
        <directory processUncoveredFilesFromWhitelist="TRUE">*/test/*</directory>
        <directory processUncoveredFilesFromWhitelist="TRUE">*/Test/*</directory>
        <directory suffix="Test.php">*/simpletest/*</directory>
        <directory suffix="TestTrait.php">*/simpletest/*</directory>
        <directory suffix="TestBase.php">*/simpletest/*</directory>
      </exclude>
      <exclude-pattern>*/tests/*</exclude-pattern>
     </whitelist>
  </filter>
Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented

I was successful with this:

  <!-- Filter for coverage reports. -->
  <filter>
    <whitelist>
      <directory>./includes</directory>
      <directory>./lib</directory>
      <!-- Extensions can have their own test directories, so exclude those. -->
      <directory>./modules</directory>
      <exclude>
        <directory>./modules/*/src/Tests</directory>
        <directory>./modules/*/tests</directory>
      </exclude>
      <directory>../modules</directory>
      <exclude>
        <directory>../modules/*/src/Tests</directory>
        <directory>../modules/*/tests</directory>
        <directory>../modules/*/*/src/Tests</directory>
        <directory>../modules/*/*/tests</directory>
      </exclude>
      <directory>../sites</directory>
     </whitelist>
  </filter>

This still has some problems for custom/contrib because we can't use a pattern. It also still excludes everything under sites, which could include other custom/contrib extensions.

Also the documentation is kind of not good when it comes to telling you how wildcard expansions might or might not work. https://phpunit.de/manual/6.5/en/appendixes.configuration.html

martin107’s picture

martin107 CreditAttribution: martin107 as a volunteer commented
Assigned: martin107 » Unassigned
Status: Needs work » Needs review
FileSize
3062122-10.patch1005 bytes
interdiff-3062122-4-10.txt1.18 KB

Thanks Mile23, that the second time in a week you have solved my problems.

This still has some problems for custom/contrib because we can't use a pattern

I think your solution is good and we should stop there.

That the beauty of open source .. I don't like the wildcard support of phpunit -- submit a patch.

Composer provides "installer paths" so the position of contrib modules needs to be assessed on a install by install basis.

My motivation is to reduce the cognitive load of the next developer ... when assessing project risks.

People with more specific needs, and I am one, can just follow your lead and add the modules they care about.

Mile23++

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer and at PreviousNext commented
Category: Feature request » Task
Priority: Minor » Normal
Status: Needs review » Reviewed & tested by the community

If this is blocking a normal task #2950132: Support PHPUnit 7 optionally in Drupal 8, while keeping support for ^6.5 then this is a normal task.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 10: 3062122-10.patch, failed testing. View results

martin107’s picture

martin107 CreditAttribution: martin107 as a volunteer commented
Status: Needs work » Reviewed & tested by the community
Issue tags: +Random test failure

The code changes are ignored unless xdebug or similar is enabled.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 10: 3062122-10.patch, failed testing. View results

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Status: Needs work » Reviewed & tested by the community

#14 seems a random failure too

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed 1dfafff and pushed to 8.8.x. Thanks!

  • catch committed 1dfafff on 8.8.x 
    Issue #3062122 by martin107, Lendude, mondrake, Mile23, jibran: phpunit...

Status: Fixed » Closed (fixed)

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

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia at Acquia commented

I posted a follow-up to take into account submodules in #3208428: PHPUnit doesn't filter out test files within submodules for code coverage reports.