code core
../vendor/bin/phpunit tests/Drupal/Tests/TestSuites/TestSuiteBaseTest.php
PHP Fatal error:  Class 'Drupal\Tests\TestSuites\TestSuiteBase' not found in core/tests/Drupal/Tests/TestSuites/TestSuiteBaseTest.php on line 101

TestSuiteBase is not part of the autoloader, so PHPUnit does no know where to find it. All Drupal core tests should be executable standalone, so we need to require the file manually.

Discovered while running Drupal core phpunit tests on Travis CI with a different test suite setup https://travis-ci.org/klausi/drupal/jobs/157279819

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

klausi created an issue. See original summary.

klausi’s picture

Status: Active » Needs review
FileSize
569 bytes

Patch.

Grimreaper’s picture

Hello,

Thanks for the patch.

I have just tested it against the 8.2.x and it solved the issue. I encountered this issue when trying to configure automated tests in PhpStorm. (#2782221-4: Result summary Area plugin not displayed when there is no result.)

Unfortunately for me, now I have another issue:

Warning: mkdir(): Permission denied Drupal\Component\PhpStorage\FileStorage->createDirectory()() (Line: 165)

And I don't know what is the directory that is tried to be created. I follow https://www.drupal.org/node/2116263 and https://www.drupal.org/node/2288559

As I tested against 8.2.x, should I change the status into RTBC?

dawehner’s picture

I'm wondering whether we should add core/tests/TestSuites to the autoloader inside the bootstrap.php

Mile23’s picture

Version: 8.3.x-dev » 8.2.x-dev
FileSize
6.2 KB

This is a bug fix of the testing system that helps prevent regressions of the testing system. So it should be in the 8.1.x branch. The patch applies in 8.2.x and 8.3.x as well.

The idea with leaving TestSuites in another namespace was that it was all about discovery and not about testing per se. But since we have to test the discovery process, we can just move it to where it makes sense to autoload.

This patch moves the test suite classes to \Drupal\TestSuites\*

The test stays where it is. This solves this issue and also #2781203: Move TestSuite/* into a dedicated namespace.

$ ./vendor/bin/phpunit -c core/ core/tests/Drupal/Tests/TestSuites/TestSuiteBaseTest.php 
PHPUnit 4.8.11 by Sebastian Bergmann and contributors.

..

Time: 258 ms, Memory: 5.75Mb

OK (2 tests, 4 assertions)
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

This looks like a really nice solution to the problem ...

klausi’s picture

Looks good, but this will break test execution for everybody that has copied a custom phpunit.xml. I guess we don't care too much about that since tests are not part of the API compatibility promise? In #2499239: Use test suite classes to discover different test types under phpunit, allow contrib harmony with run-tests we also changed phpunit.xml without thinking about that, so this is probably ok.

Otherwise RTBC+1, this is certainly the most cleanest solution to the problem.

catch’s picture

With the customized phpunit.yml what would happen if we copied the classes back to their old location? Usually we try not to break test runs in patch releases unless it's necessary to fix a (non-testing) bug.

Mile23’s picture

I moved TestSuiteBase here because we're trying to test it and because we also have #2781203: Move TestSuite/* into a dedicated namespace

It was probably a mistake to put them in weird places to begin with. I'd hate to put a bunch of require_once all over this system to allow the test to run.

Also:

Usually we try not to break test runs in patch releases unless it's necessary to fix a (non-testing) bug.

This won't break test runs for the testbot or for anyone doing core dev unless they've rigged their own phpunit.xml file as mentioned.

If all your CI script does is say phpunit -c core/ then you're golden. And as @klausi mentioned, we did this previously to add the suite classes to begin with. I'm pretty sure other than this issue and the duplicate, we haven't heard of any problems.

We could split this back out into two issues and get @klausi's test running using a require_once to load the class we're resting, and then address the locations of the files in #2781203: Move TestSuite/* into a dedicated namespace. In that case, the patch in #2 is the way to go.

catch’s picture

Status: Reviewed & tested by the community » Needs review

OK I prefer patch #2 for this issue in 8.2.x, then a new issue for 8.3.x for the move.

Mile23’s picture

Here's a re-upload of #2 just to keep it clear. Please give all credit to @klausi. :-)

And a manual test run:

$ ./vendor/bin/phpunit -c core core/tests/Drupal/Tests/TestSuites/TestSuiteBaseTest.php 
PHPUnit 4.8.11 by Sebastian Bergmann and contributors.

..

Time: 332 ms, Memory: 5.75Mb

OK (2 tests, 4 assertions)
catch’s picture

Status: Needs review » Reviewed & tested by the community

Thanks. RTBC-ing that patch, but please open the follow-up, I think that'd be OK for a minor release (i.e. 8.3.x).

Mile23’s picture

The follow-up is #2781203: Move TestSuite/* into a dedicated namespace which I just re-opened. :-)

xjm’s picture

Issue tags: +rc eligible

I think @catch, @effulgentsia, @Cottser, @webchick, and I also agreed this can be rc eligible as a non-disruptive testing improvement. Thanks @Mile23.

  • catch committed dc509d0 on 8.3.x
    Issue #2794715 by Mile23, klausi: TestSuiteBaseTest cannot be executed...
catch’s picture

Committed/pushed to 8.3.x, thanks!

Leaving RTBC against 8.2.x.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Er rc eligible, so cherry picked.

  • catch committed 37651b1 on 8.2.x
    Issue #2794715 by Mile23, klausi: TestSuiteBaseTest cannot be executed...

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 11: testsuite-2794715-2.patch, failed testing.

dawehner’s picture

Status: Needs work » Fixed

Status: Fixed » Closed (fixed)

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