Add Code Coverage reporting

We are actually planning to port as much as possible all the new GitLab CI features that were added to Drupal core and this is one of them.
I will close this issue and open the one to track the newer and improved version.

New issue #3389921: New templates for contrib: mirror as much as possible all the GitlabCI jobs from core →

The Drupal Core template currently does not have Code Coverage enabled, re-opening the issue.

I also don't want to see #3389921: New templates for contrib: mirror as much as possible all the GitlabCI jobs from core → possibly cause contrib to require large re-writes to the template to continue using code coverage (I tested with our current _PHPUNIT_CONCURRENT setting and it breaks.)

We do have a long standing feature request in core ✨ Allow run-tests.sh to generate coverage reports Needs work , maybe that is where this work actually needs to be done.

Regarding backward compatibility, we will most likely add it as a brand-new file, so people can choose to stay in the "old" version (by doing nothing) or the "new" version (by updating the reference to the file).

Note that the _PHPUNIT_CONCURRENT is still set as experimental. Keeping the default value for it shouldn't break things and it is explained that the options are slightly different if the value changes.

I'm pretty sure that @secretsayan has some insights/suggestions to share … 🧐🤓

PCOV is now installed in PHP8.3 images

We can evaluate what we want to do with this.

Even if we don’t enable coverage testing plugins we can safely add coverage: /^\s*Lines:\s*\d+.\d+\%/ to the global gitlab file as it will not cause test failures (it is used as a trigger to detect percentages when available).

Meanwhile I added xdebug as well but only to :dev 8.3 images - it's twice slower but more precise

@cmlara - so, adding coverage: '/^\s*Lines:\s*\d+.\d+\%/' would be all that's needed? I guess we'd add it to the `phpunit` job. No other changes for reporting or something else?

Example from the recipes initiative: #3442014: Add code coverage report →
https://git.drupalcode.org/project/distributions_recipes/-/commit/2f82a0...

coverage: '/^\s*Lines:\s*\d+.\d+\%/'
Is used to allow reporting of the percentage of code that is covered. Its the simplest level of coverage reporting. It can be used in a Badge and can be displayed on the MR page to give an overall indication of changes. This check is done by looking at the job log output making it always safe to have in the GitLab CI file.

This will usually also be paired with disabling colors to ensure the logs matches the REGEX.

  _PHPUNIT_EXTRA:
    value:  --colors=never

The asset file report is what generates the Line-By-Line highlighting (red/green) in an MR on if the code is covered. However IIRC if the asset file is missing GitLab will error out. This would either need to be optional or that we make sure we always enable code coverage before adding this code to the default template.

   reports:
      coverage_report:
        coverage_format: cobertura
        path: coverage.xml

✨ Allow run-tests.sh to generate coverage reports Needs work will need to figure this out for running in Parallel. I was going to say I had only found found scripts that can combine multiple PHPUnit runs into standard (directory based) coverage reports however I may have just found a script that can convert those to cobertura format which might help us solve the run-tests.sh problem.

Non-parallel runs are much easier

  _PHPUNIT_EXTRA:
    value: --coverage-cobertura $CI_PROJECT_DIR/coverage.xml --coverage-filter $CI_PROJECT_DIR/$_WEB_ROOT/modules/custom/ --coverage-text --colors=never

Can actually enable Code Coverage during PHPUnit execution (after the pcov module is enabled). Obviously if we submit this to production for all modules we would follow #10 and place this in the actual PHPUnit run line and not in $_PHPUNIT_EXTRA.

Though as I noted previously this requires that the module NOT be symlinked as PCOV does not work well with the module code being in the root of the $CI_PROJECT_DIR and symlinked from $CI_PROJECT_DIR/$_WEB_ROOT/modules/custom/, at least with the coverage filter. This may be possibly just configuration issues on what I designed to date, once I found a working setup (after dozens of diffrent styles) I stuck with it.

Using a PHPUnit.xml file is also another method to set these settings as shown by the example in #10 Nothing wrong with using a phpunit.xml, I only avoided it as it was one more file to write, and I was having issues making pcov work with the coverage restrictions. Some of those were very much likely errors on my part at the time.

I'm going to need to look at #10 closer, one item I see different between is no use of echo -e '[pcov]\npcov.directory=.' > /usr/local/etc/php/conf.d/pcov.ini. In my experience without this PCOV would only show coverage for files in the src/ folder ignoring PHP code in the root folder, and in sub-modules IIRC. The phpunit.xml may be solving that issue.

I created an MR https://git.drupalcode.org/project/gitlab_templates/-/merge_requests/253 with the implementation of the code coverage report.
Here is an example of a successful pipeline job with it: https://git.drupalcode.org/project/test_helpers/-/jobs/2599892

It reports the code coverage results to Gitlab MR interface:

Merged result pipeline #269172 passed
Merged result pipeline passed for 2df47e4b
Test coverage 71.19% from 1 job

Could you please review the MR?

This looks interesting, thanks for starting it.

I can see in your MR107 that the project's repo also contains the new file phpunit-drupal-ci-module.xml which is being added in the gitlab templates MR into assets, and downloaded to the projects top-level folder if it does not exist. But the extra arguments supplied add this as a config file via export _PHPUNIT_COVERAGE_ARGS="-c $CI_PROJECT_DIR/$_PHPUNIT_CONFIG_FILE --coverage-text --colors=never". What happens if the project already has a phpunit config file, but it is named differently, such as the default phpunit.xml?

I will test the MR and see how it goes.

First run, I forgot to add new variable _PHPUNIT_COVERAGE: 1

Second run failed
https://git.drupalcode.org/project/scheduler/-/pipelines/269432
This project does have its own phpunit.xml which is the config used by default. I don't know if that is the cause of the error, it is probably something simple, but I'll let you investiagte.

FYI pcov added to PHP 8.4 image (with patch) https://git.drupalcode.org/project/drupalci_environments/-/commit/6982c0...

Second run failed
https://git.drupalcode.org/project/scheduler/-/pipelines/269432

@jonathan1055, thanks for the testing! It's failed because runs in the concurrent mode, which uses the web/core/scripts/run-tests.sh script. And this script, unfortunately, doesn't support the --coverage-text argument:

executing: sudo SYMFONY_DEPRECATIONS_HELPER='disabled' MINK_DRIVER_ARGS_WEBDRIVER='["chrome", {"browserName":"chrome","goog:chromeOptions":{"args":["--disable-dev-shm-usage","--disable-gpu","--headless"]}}, "http://localhost:9515"]' -u www-data php web/core/scripts/run-tests.sh --color --keep-results --concurrency 32 --repeat '1' --sqlite 'sites/default/files/.sqlite' --dburl mysql://drupaltestbot:drupaltestbotpw@database/mysql --url http://localhost/web --verbose --non-html  -c /builds/project/scheduler/phpunit-drupal-ci-module.xml --coverage-text --colors=never  scheduler_js
...
  ERROR: Unknown argument '--coverage-text'.

I will prepare a patch for Drupal Core to fix this, but for now - try to test with no-parallel mode using _PHPUNIT_CONCURRENT=0 - it runs phpunit directly.

OK if it only runs when _PHPUNIT_CONCURRENT=0 maybe it needs a conditional if _PHPUNIT_CONCURRENT=1 give a message and do not add the coverage options.

I re-ran with _PHPUNIT_CONCURRENT=0 and did get some coverage output. However, the log is huge, > 35,000 lines and it seems to be checking not only this project, but also the other module dependencies loaded for the test, and 3rd-party software (webdriver, twig, symfony, etc)
https://git.drupalcode.org/project/scheduler/-/jobs/2609518

However, the log is huge, > 35,000 lines and it seems to be checking not only this project, but also the other module dependencies loaded for the test, and 3rd-party software (webdriver, twig, symfony, etc)

Hope that fixed this in the last commit, please retry.

Failed earlier this time, with
curl: (3) URL using bad/illegal format or missing URL
https://git.drupalcode.org/project/scheduler/-/jobs/2610500

I think I will leave the testing until you've done some proper test runs yourself, to iron out these things :-)

@jonathan1055, with your project, I found a problem in the MR and fixed it, here is your green pipeline: https://git.drupalcode.org/issue/scheduler-3445052/-/jobs/2612741

It's better to have testing on different projects to catch more bugs, not only on my one project ;)

Seems the coverage reporting is not supported in the parallel (concurrent) mode at all, so I added throwing an error if both modes are enabled:

if [[ "$_PHPUNIT_CONCURRENT" == "1" ]]; then
  echo "Error: The coverage mode is not supported the parallel mode (_PHPUNIT_CONCURRENT=1). Set the _PHPUNIT_CONCURRENT=0 to enable coverage reporting." >&2
  exit 1
fi

Seems all the issues were fixed, so moving back to Needs review.

Thanks so much for working on this. In the provided phpunit.xml file, there is reference to a class that no longer exist in Drupal 11.
Setting to "Needs work" based on that.

Adding some testing cases to the description so we can cover some of the cases.

I also added reporting the per-line code coverage using Cobertura, cuz GitLab doesn't support JUnit - only Cobertura or JaCoCo, see https://docs.gitlab.com/ee/ci/testing/test_coverage_visualization/index....

And it works well - a job: https://git.drupalcode.org/project/test_helpers/-/jobs/2626173 and the screenshot:

Thanks for working on this, murz, but I have reservations about adding this functionality in the form proposed here, for several reasons:

1. It only works when _phpunit_concurrent=0 however our preferred/recommended/default is likely to change to run with concurrency, see #3447087: Switch to using run-tests.sh by default →
2. The coverage details and result are for the entire project codebase so when the testing is split into separate jobs using @group or parallel jobs the result is meaningless because each job only runs a subset of the tests so the actual coverage is much higher than what is reported
3. Additional runtime - for example the Scheduler javascript test from #22 took 7 mins 3 seconds, but the equivalent test using concurrent=0 without coverage only took 3 mins 13 secs. I know this is only a sample of one so more test runs would be needed to prove it, but at first glance is appears significant
4. There may be implications on CPU but I have not got that info yet
5. The coverage is run for every variant phpunit job but this is unnecessary, extra work

I would like to suggest that instead of adding this in to the existing phpunit job we create a new separate job for it, called ‘Test Coverage’ which would be manually triggered thus only run on demand, and only with "current" core, no variants, to save resources. Test coverage does not need to be run in every pipeline as it won’t be changing all the time. The new job would ignore the _phpunit_concurrent variable and always run the phpunit binary directly. It would ignore @group and always run all tests in the project so that a meaningful result was produced. The new job can extend phpunit in a similar way to 'test-only changes'

I also think it would be better to get the phpunit.xml file from the core directory in the job, and not store it in the GitLab templates repo, so that the correct version is used for whatever Core is being tested. I have not looked in detail to see exactly what changes are done to this file. Ideally it would be better to find another way entirely. Maybe get from core if it does not exist, then use sed to modify the file.

I am sure we'll find a good way to create the code coverage reports, as it will be a big benefit to developers and maintainers. I just think we need to take a step back from the current approach.

I like above approach to run ‘Test Coverage’ or 'Code Coverage' on demand.

Some notes after looking at this:

• Recently, a script was introduced to massage the core phpunit.xml.dist, see 🐛 #3480767 Causes break in behavior of tests configuration Active . We can probably extend the logic here.
• I also have reservations regarding performance as mentioned in #27. Plus one for making it an additional manual job.
• We should add documentation for the different settings, limitations regarding concurrency and the coverage configuration.

If you are fine with it, I can continue with some changes in the existing merge request.

I also like the idea of a new on-demand job. Adding that new feature will be easier than modifying this key part of the CI system.

@simonbaese - there was no additional work since September and the issue is not assigned so I'd say feel free to work on the existing MR, or create a new one as needed. I've rebased the "main" branch from the fork, because the other branch (linked to the existing MR) was giving a conflict. Feel free to create a new branch and MR and borrow as much as you need from the existing MR taking into account all the feedback above.

This is a work in progress. There is an error in the phpunit-enable-coverage.sh script. Maybe someone can help.

Recent changes on the merge request:

• Introduced phpunit (coverage) job that needs to be triggered manually.
• Introduced variable OPT_IN_PHPUNIT_COVERAGE to enable the job mentioned above.
• Using the prepare-phpunit-xml.php script to provide coverage definition for contrib modules.
• Properly uploading coverage artifacts.

There are some open questions:

• How to properly merge PHPUnit arguments when using the coverage job? At the moment, it concats the PHPUNIT_COVERAGE_ARGS with PHPUNIT_EXTRA. This may lead to duplications, which result in PHPUnit warnings.
• How to enable Xdebug or PCOV properly? I see multiple variants in this issue, the comments and other contrib modules. Note that "line coverage, branch coverage, and path coverage data will be collected, processed, and reported. This requires a code coverage driver that supports path coverage. Path Coverage is currently only implemented by Xdebug."
• What are the correct coverage definitions in phpunit.xml? I saw some concerns in 🐛 #3480767 Causes break in behavior of tests configuration Active . Currently, it points to the project files in web/modules/custom/... and excludes tests folders.

And, can someone help out with the test failures in the merge request of this issue? Some changes might be out-of-scope for this issue.

Rebasing the fork will eliminate one error.

Then see the suggestion for the other

PHP_EXTENSIONS_SUBDIRECTORY=$(ls -d "$PHP_EXTENSIONS_DIRECTORY"/* | head -n 1)
                              ^-- SC2012: Use find instead of ls to better handle non-alphanumeric filenames.
For more information:
  https://www.shellcheck.net/wiki/SC2012

Rebasing might fix the curl issue as well.

I guess that the variables in the curl URL are not resolved correctly. Not sure how to fix this, though. Maybe some fresh eyes can help.

I found out why my test project was not working. I assumed that when you set a custom project and ref also the _GITLAB_TEMPLATES_REPO/REF and therefore _CURL_TEMPLATES_REPO/REF would be set as well. Maybe this is something we can address in another issue or add documentation.

So, the issue with the script is resolved. We can continue with the questions from #31.

Sorry I'm away from the computer most of the time so I'm just doing quick checks. Yeah, in this case, because it's a new file only introduced in this MR, it might need setting the other variables too, as explained here https://project.pages.drupalcode.org/gitlab_templates/info/testing-mrs/#... (feel free to make suggestions on how to improve that if needed).

Re #31 questions:

* How to properly merge PHPUnit arguments when using the coverage job? At the moment, it concats the PHPUNIT_COVERAGE_ARGS with PHPUNIT_EXTRA. This may lead to duplications, which result in PHPUnit warnings.
--> I'd just document this behaviour in the variable description and the documentation page (still missing in the MR), that way we'd only add values in PHPUNIT_COVERAGE_ARGS that are not present in PHPUNIT_EXTRA.

* How to enable Xdebug or PCOV properly? I see multiple variants in this issue, the comments and other contrib modules. Note that "line coverage, branch coverage, and path coverage data will be collected, processed, and reported. This requires a code coverage driver that supports path coverage. Path Coverage is currently only implemented by Xdebug."
--> Given that path coverage is only available in xdebug, I'd opt for that. This article is useful too: https://thephp.cc/articles/pcov-or-xdebug?ref=phpunit
--> Maybe we can code it in such a way where we can use either tool, but I'd consider this out of the scope of this issue, but could be done in a follow-up.

* What are the correct coverage definitions in phpunit.xml? I saw some concerns in 🐛 #3480767 Causes break in behavior of tests configuration Active . Currently, it points to the project files in web/modules/custom/... and excludes tests folders.
--> I'd opt for a "sensible" default, and we can discuss it in the MR if needed.

* How to treat modules requiring another test bootstrap? I am specifically thinking about existing site tests.
--> Can you give a more detailed example about a case that would need this and not the core bootstrap file?

* How does PHPUnit know about the database connection - although it is not defined in the phpunit.xml?
--> PHPUnit extends .phpunit-base, which extends .testing-job-base, which includes the "with-database" service, and also extends ".test-variables" which contain the DB credentials to use.

This is great, thanks for progressing it @simonbaese. For testing this MR, in addition to

  - project: issue/gitlab_templates-3389338
    ref: 3389338-phpunit-coverage

in your .gitlab-ci.yml, you just need to add the following two variables -

variables:
  _CURL_TEMPLATES_REPO: issue/gitlab_templates-3389338
  _CURL_TEMPLATES_REF: 3389338-phpunit-coverage

The you can remove the @todo hard-coding in the MR

Separate to the questions in #31 and the replies in #37 I have also reviewed the MR and would like to make the following suggestions

1. The new variable to control this job is currenly named OPT_IN_PHPUNIT_COVERAGE. But the 'OPT_IN' variables are used to control which variants (core versions and php versions) to test. A better and more consistent choice would be the 'SKIP_' variables, which determine which jobs are run. So maybe rename it to SKIP_TEST_COVERAGE, with default 0. This will also need the rule to be changed from .opt-in-phpunit-coverage-rule to .skip-coverage-rule
2. The script currently halts when _PHPUNIT_CONCURRENT is set to 1. Back in #27 I suggested that this variable should be ignored and the coverage is always run and uses the phpunit binary directly. That means we can still get a coverage report when the main phpunit tests are run concurrently. As we are extending the .phpunit-base then this will require the variable _PHPUNIT_CONCURRENTto be set to 0 explicitly and exported in the Coverage job, ready for use in the phpunit base script.
3. Also need to set the variable _PHPUNIT_TESTGROUPS to '--all' so that the phpunit binary ignores any testgroup setting and runs all groups. The coverage report would be meaningless if only a subset of the test groups were run.
4. There is no need to have *skip-phpunit-rule in the new job, as that means you have to run phpunit if you want coverage. For flexibility, remove that rule so that the two jobs can be controlled independently.
5. I think we can remove coverage: '/^\s*Lines:\s*\d+.\d+\%/' from .phpunit-base, as this is now in the Coverage job instead. Same for the additions to artifacts: coverage_report: as these are in the new job.
6. Is the new job name "phpunit (coverage)" still the best name? Maybe ‘Test Coverage’ or ‘Code Coverage’ would give a clearer idea of what it does?
7. I am running a test but it is failing and I can't see what is happening in the log. Maybe if we move the execution of source ./phpunit-enable-coverage.sh out from the pipe into a separate statement so that we can see it being run in the log.

I hope these point make sense. Do ask if you have questions. I was going to try to make code suggestions in the MR itself, but some of the above would be difficult with that feedback.

Excellent work so far, and this is going be an excellent addition to the pipeline. Thanks.

Thinking again, ignore point 3 above. Even though the overall report would give a false impression it can be useful to run on just a subset, and removing this would be a loss of flexibility. Given the slowness of some of the runs, and timeout issues I am experiencing, having the ability to test the job on small testgroup is actually vital.