Updated: Comment #N

Problem/Motivation

Having installed the vendor dir for a contrib module the vendor components tests are included by simpletest_phpunit_get_available_tests.

Visiting http://drupal.d8/admin/config/development/testing gives

PHP Fatal error: Class 'TestCase' not found in /.../modules/graphapi/vendor/clue/graph-uml/tests/Graph/Uml/ClassDiagramBuilderTest.php

Removing the graphapi/vendor makes http://drupal.d8/admin/config/development/testing appear but is a DX #fail IMHO as that stop contrib module development.

Running 

cd modules/graphapi
composer install --no-dev

installs the FATAL classes.

Proposed resolution

Simpletest should not include tests for 'alien' vendor dirs.

According to #2 we should only use whitelisting. This can be accomplished to include

modules/*/tests
sites/*/*/tests

Remaining tasks

Is it OK not to support sub modules /modules/my-module/modules/my-submodule/tests thus dictate all tests resides in the /modules/my-module/tests?

Update https://drupal.org/node/2116043 to reflects the directories modules/*/tests and sites/*/*/tests

User interface changes

API changes

CommentFileSizeAuthor
#5 core-phpunit-2203747-5-do-not-test.patch712 bytesclemens.tolboom
#1 core-phpunit-no-contrib-vendor-do-not-test.patch549 bytesclemens.tolboom
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
CreditAttribution: clemens.tolboom commented
Status: Active » Needs review
FileSize
core-phpunit-no-contrib-vendor-do-not-test.patch549 bytes

I tried to fix this through exclude through phpunit.xml.dist but we need a pattern and no path.

Reading through http://phpunit.de/manual/3.7/en/appendixes.configuration.html does not give me much hints.

Attached patch is probably a good direction. I'm not sure which one we need:

  1. modules/*/tests
  2. modules/*/lib
  3. modules/*/src
clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
CreditAttribution: clemens.tolboom commented

According to https://github.com/sebastianbergmann/php-code-coverage/issues/222 we should only use whitelisting. Is that the solution to this problem?

Our /modules directories are dynamic in nature so that will not do I guess.

[edit]http://phpunit.de/manual/3.7/en/appendixes.configuration.html#appendixes... does not give me enough info[/edit]

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Status: Needs review » Needs work

Patch seems good enough, I think. It's not 100% foolproof, i.e. there could be "tests" directory somewhere in vendor, but it's better than the status quo, at least.

We should add the same to the modules in sites, though, to be consistent.

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
CreditAttribution: clemens.tolboom commented
Issue summary: View changes
clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
CreditAttribution: clemens.tolboom commented
Status: Needs work » Needs review
FileSize
core-phpunit-2203747-5-do-not-test.patch712 bytes

I've checked https://drupal.org/node/2116043 and indeed all php-unit test should be in **/my-module/tests

Trouble is where go tests for sub modules?

And what about unit tests for themes?

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
CreditAttribution: clemens.tolboom commented
Issue summary: View changes

Updated the remaining tasks.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Status: Needs review » Reviewed & tested by the community

I'm not exactly sure the comment is needed (especially because the same is true for core modules as well), but it can't hurt I guess. Looks good!

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
CreditAttribution: clemens.tolboom commented
Status: Reviewed & tested by the community » Needs review

I've tested this manually for both /modules/*/tests and /sites/*/modules/tests .. that works as expected.

But this is Simpletest UI killer skipping test for directory layouts like

/modules/contrib/*/tests
/modules/custom/*/tests
/sites/*/modules/contrib/*/tests
/sites/*/modules/custom/*/tests

Mitigating for this is people can run their tests from anywhere like

cd /core
# random path
phpunit ../sites/all/modules/tests/

Should we add these contrib | custom directories?

      <directory>../modules/*/tests</directory>
      <directory>../modules/*/contrib/*/tests</directory>
      <directory>../modules/*/custom/*/tests</directory>
      <directory>../sites/*/modules/*/tests</directory>
      <directory>../sites/*/modules/contrib/tests</directory>
      <directory>../sites/*/modules/custom/tests</directory>
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented

See #2056607-28: /core/phpunit.xml.dist only finds top-level contrib modules, this was not done on purpose.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

FWIW, this is double-flawed right now. The PHPUnit bootstrap.php file performs multiple, massive filesystem directory scans.

Instead of hard-coding any broken glob patterns, we're discovering + recursing on every boot of phpunit anyway already, and a test suite in XML is just simply a serialization of PHPUnit_Framework_TestSuite. It is pointless to do both.

In fact, triple-flawed. Even if you have phpunit 4.x installed globally, to advance on #8:

$ phpunit -c core/phpunit.xml.dist
[…WAIT…]
PHPUnit 3.7.21 by Sebastian Bergmann.

Configuration read from core/phpunit.xml.dist
...
Crell’s picture

Crell CreditAttribution: Crell commented

Marked #2137441: Simpletest tries to use tests in module vendor directories as a duplicate, even though it's older as this one has traction.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom commented
Status: Needs review » Closed (duplicate)

This is a duplicate of #2203747: Current PHPUnit configuration scans into contrib vendor dirs.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom commented

Er, I meant #2182231: Testing scans for tests in contrib vendor directory.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom commented
Priority: Normal » Major
Status: Closed (duplicate) » Needs review

Re-opening as I realized this one is further along towards a solution. Bumping to major.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom commented
Title: simpletest_phpunit_get_available_tests scans into contrib vendor dirs » Current PHPUnit configuration scans into contrib vendor dirs
jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom commented

One possible approach that wouldn't involve reworking the whitelist, and allow contrib to keep unit tests with submodules, would be to implement our own test loader (PHPUnit_Runner_TestSuiteLoader), and load UnitTestCase implementations instead of the default PHPUnit_Framework_TestCase.

The downside of this is that unit tests directly implementing PHPUnit_Framework_TestCase would not be run.

daffie’s picture

daffie CreditAttribution: daffie commented
Component: simpletest.module » phpunit
cilefen’s picture

cilefen CreditAttribution: cilefen commented
Status: Needs review » Closed (cannot reproduce)
Issue tags: +Triaged at DrupalCon Los Angeles 2015

@phillamb168 and triaged this issue at Los Angeles.

We found we could not recreate the issue at all in the GUI but we could verify with phpunit that it is now fixed. Here is how we know:

We created a fake module in /modules that has a unit test and noted that when running phpunit -c core/, our unit test is executed along with the core tests. Then, in the fake module directory, we added some libraries from composer, composer require symfony/console. The vendor tests are not executed when running phpunit -c core/.

Then, we checked out and older version of the test configuration, git checkout 0b89120 core/phpunit.xml.dist. Then when we ran phpunit -c core/, the vendor tests were discovered and executed.

phpunit.xml.dist has changed significantly in the last year. Issues like #2232861: Create BrowserTestBase for web-testing on top of Mink added the whitelisting recommended in this issue's summary. This issue has been fixed.

clemens.tolboom’s picture

clemens.tolboom
Primary language Dutch
Location Groningen, 🇳🇱/🇪🇺
CreditAttribution: clemens.tolboom as a volunteer commented

@cilefen and @phillamb168 thanks for doing the triage.