Figure out how to check for unintentionally covered code

1+2 is the correct answer.

::mad scientist:: I TRIED TO TELL THEM BUT THEY WOULDN'T LISTEN!!1!!

Anyway.

Option #4 is to kill \Drupal. Which I would endorse. I'm pretty sure it's less practical than any of the other options, however.

The example code on the github issue has a @uses \Drupal (analogue) for the base class, whereas UnitTestCase is not annotated that way.

The problem in that case would be that it's impossible to get coverage for \Drupal using that test case. So the solution would be that all tests of \Drupal would have to be from another base class.

That's much more doable than converting *all* tests.

So:

1. Add @uses \Drupal to UnitTestCase.
2. Convert unit tests of \Drupal to subclass \PHPUnit_Framework_TestCase (or add another suitable subclass if it's needed).

I'm pretty sure the \Drupal tests don't need the container or what-have-you.

And a patch. :-)

Which, I now realize, is against 8.0.x, not 8.1.x, but hopefully it will apply.

Patch still applies to 8.3.x. Re-running tests.

Thinking @neclimdul might have an opinion on this: #2824313: All unit tests should use UnitTestCase base class

checkForUnintentionallyCoveredCode has been deprecated in PHPUnit 5, see https://github.com/sebastianbergmann/phpunit/wiki/Release-Announcement-f...

so combined with #2795317: Allow PHPUnit 6+ support for object mocking, not sure what this means for the future of this issue, but thought I'd ask.

I think #12 means that we close this issue as won't fix.

Closing this issue based on #12.

Do we want to remove this line?

<!-- TODO set checkForUnintentionallyCoveredCode="true" once https://www.drupal.org/node/2626832 is resolved. -->

Reached here from core/phpunit.xml.dist. Let's remove it.

Wait, but shouldn't we add beStrictAboutCoversAnnotation to phpunit.xml.dist then, instead?

Did not know we have an alternative. @tstoeckler++

This yields results like this:

$ ./vendor/bin/phpunit -c core/ --testsuite unit --coverage-html ../coverage -v --stop-on-risky
PHPUnit 6.5.9 by Sebastian Bergmann and contributors.

[...]

There was 1 risky test:

1) Drupal\Tests\Core\Database\Driver\pgsql\PostgresqlConnectionTest::testEscapeTable with data set #0 ('nocase', 'nocase')
This test executed code that is not listed as code to be covered or used:
- Drupal::unsetContainer
- Drupal\Component\FileCache\FileCacheFactory::setConfiguration
- Drupal\Component\FileCache\FileCacheFactory::setPrefix
- Drupal\Core\Database\Connection::__construct
- Drupal\Core\Database\Connection::escapeTable
- Drupal\Core\Database\Connection::setPrefix
- Drupal\Core\Database\Driver\pgsql\Connection::__construct
- Drupal\Core\Database\Driver\pgsql\Connection::doEscape

OK, but incomplete, skipped, or risky tests!
Tests: 4, Assertions: 5, Risky: 1.

If you look at PostgresqlConnectionTest you see that it has @coversDefaultClass and @covers annotations. But it lacks @use for code paths that aren't within that default class and its methods. Like this:

/**
 * @coversDefaultClass \Drupal\Core\Database\Driver\pgsql\Connection
 * @group Database
 */
class PostgresqlConnectionTest extends UnitTestCase {

This means that even though our test is annotated to cover \Drupal\Core\Database\Driver\pgsql\Connection, our report says that it is not covered, because we set beStrictAboutCoversAnnotation.

In that report you'll also see that \Drupal::unsetContainer() and file cache management are still a problem and that's what the code changes in #4 try to fix.

Also, if you limit to --group Database (for brevity's sake) you see this result:

OK, but incomplete, skipped, or risky tests!
Tests: 89, Assertions: 124, Risky: 45.

That means roughly half of the existing @covers annotations are being ignored because of the way UnitTestCase works, and because we don't have perfect isolation in the tests themselves.

I won't set this to NW but I'll add the 'needs followup' tag because if we commit to this, it means we need a followup to modify at least UnitTestCase (and maybe KTB) to @use \Drupal and so forth (or add the changes from #4), and also to explicitly mark functional tests as @coversNothing.

I think that'd be a good change, but I'm not sure any actual maintainer wants to go down that path at this stage.

BTW the issue in #2 is closed due to lack of feedback, but was referenced by another very similar issue: https://github.com/sebastianbergmann/php-code-coverage/issues/629

I dunno this one feels a weird one for me. This only has an effect if xdebug is on so no tests on DrupalCI are going to fail because of it. I think until we've got code coverage reports being generated somewhere this setting is not very useful for us. If you are running code coverage locally and you want to do this there is nothing to stop you setting this up in your local phpunit.xml as you see fit. I think out phpunit.xml.dist should contain how DrupalCI runs the tests and not anything else. If people want to do advance PHPUnit configuration they should be using PHPUnit docs and examples not ours.

I'm not sure if we want to have #19, even as a goal later down the line. I do think that #17 is something we should do, as it removes a reference to a setting that will no longer work.

Found related issue

Marked #2893232: Deprecated configuration setting "checkForUnintentionallyCoveredCode" used as duplicate.

Given #23 let's call this issue postponed on #2974911: Allow run-tests.sh to generate coverage reports. We currently don't do a coverage report for core for the project CI, AFAICT.