Problem/Motivation

When using a testing.services.yml file in the parent site, currently any kernelTest will fatal because KernelTestBase will try to copy this file to a non-existent location

Proposed resolution

Copy it to the right location and fix a silly concatenation while we are at it.

Remaining tasks

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Lendude created an issue. See original summary.

Lendude’s picture

Title: Using testing.services.yml in a kernelTest fatals » Using testing.services.yml in a kernelTest fatals when trying to copy this to the test site
Status: Active » Needs review
FileSize
1.53 KB
2.54 KB

Here is a test and a fix.

Not happy with the test since this writes to the parent site, but since that is what we are trying to test I don't see a way around that. Better ideas are welcome.

tstoeckler’s picture

Status: Needs review » Needs work

Yes, this makes sense. For context, this is how $this->siteDirectory is set up:

    $this->siteDirectory = vfsStream::url('root/' . $test_site_path);

so it ends up being something like vfs://root/sites/simpletest/123456789. Since $this->root is the actual (host) Drupal root URL the prepending doesn't make any sense.

Regarding test coverage: Yeah, I don't think we can get away with modifying anything in sites/default as part of a test. If people actually use that feature and happen to run KernelTestBaseTest we would either be deleting their test overrides or the test would fail due to permission issues. Neither is desirable, IMO. So let's just not test this. The fact that this explicitly involves the host/parent system is also the reason why this is currently completely untested. So far no one has been able to think of a way to test this in isolation...

Also, I found #2927487: \Drupal\KernelTests\KernelTestBase::bootEnvironment() has some dead code while reviewing this.

Lendude’s picture

Status: Needs work » Needs review
FileSize
1.01 KB

@tstoeckler thanks for the feedback.

Ok so just the fix then. The test-only should still illustrate the problem even if it's just a one off thing.

The last submitted patch, 2: 2927476-2-TEST_ONLY.patch, failed testing. View results

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, perfect!

To make up for the lack of automated test coverage, here's the output my shell where I verified the correctness of the patch:


/path/to/drupal/core/tests $ php ../../vendor/bin/phpunit --configuration ../ 'Drupal\KernelTests\Core\Asset\ResolvedLibraryDefinitionsFilesMatchTest'
PHPUnit 4.8.36 by Sebastian Bergmann and contributors.

Testing Drupal\KernelTests\Core\Asset\ResolvedLibraryDefinitionsFilesMatchTest
.

Time: 2.6 minutes, Memory: 6.00MB

OK (1 test, 552 assertions)
/path/to/drupal/core/tests $ touch ../../sites/default/testing.services.yml
/path/to/drupal/core/tests $ php ../../vendor/bin/phpunit --configuration ../ 'Drupal\KernelTests\Core\Asset\ResolvedLibraryDefinitionsFilesMatchTest'
PHPUnit 4.8.36 by Sebastian Bergmann and contributors.

Testing Drupal\KernelTests\Core\Asset\ResolvedLibraryDefinitionsFilesMatchTest
E

Time: 958 ms, Memory: 6.00MB

There was 1 error:

1) Drupal\KernelTests\Core\Asset\ResolvedLibraryDefinitionsFilesMatchTest::testCoreLibraryCompleteness
copy(/path/to/drupal/vfs://root/sites/simpletest/41735229/services.yml): failed to open stream: No such file or directory

/Users/tobias.stoeckler/workspace/d8/vendor/symfony/phpunit-bridge/DeprecationErrorHandler.php:262
/Users/tobias.stoeckler/workspace/d8/core/tests/Drupal/KernelTests/KernelTestBase.php:331
/Users/tobias.stoeckler/workspace/d8/core/tests/Drupal/KernelTests/KernelTestBase.php:243
/Users/tobias.stoeckler/workspace/d8/core/tests/Drupal/KernelTests/Core/Asset/ResolvedLibraryDefinitionsFilesMatchTest.php:95

FAILURES!
Tests: 1, Assertions: 0, Errors: 1.
/path/to/drupal/core/tests $ curl https://www.drupal.org/files/issues/2927476-4.patch | git apply --index
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  1034  100  1034    0     0   8970      0 --:--:-- --:--:-- --:--:--  8991
/path/to/drupal/core/tests $ php ../../vendor/bin/phpunit --configuration ../ 'Drupal\KernelTests\Core\Asset\ResolvedLibraryDefinitionsFilesMatchTest'
PHPUnit 4.8.36 by Sebastian Bergmann and contributors.

Testing Drupal\KernelTests\Core\Asset\ResolvedLibraryDefinitionsFilesMatchTest
.

Time: 2.64 minutes, Memory: 6.00MB

OK (1 test, 552 assertions)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 4: 2927476-4.patch, failed testing. View results

Lendude’s picture

Status: Needs work » Reviewed & tested by the community

Unrelated fail

  • xjm committed 148db4b on 8.5.x
    Issue #2927476 by Lendude, tstoeckler: Using testing.services.yml in a...

  • xjm committed 8df8547 on 8.4.x
    Issue #2927476 by Lendude, tstoeckler: Using testing.services.yml in a...
xjm’s picture

Version: 8.5.x-dev » 8.4.x-dev
Status: Reviewed & tested by the community » Fixed

Thanks for the test patch and the verification in #6; those demonstrate the bug and fix nicely. I agree we should not futz with sites/default in a test. (Reminds my of my first core issue... #1212992: Prevent tests from deleting main installation's tables when parent::setUp() is not called.)

The silly concatenation fix is out of scope and it did actually distract me from the fix, because I spent a couple minutes trying to figure out why that also needed a change before I saw it was just a silly out-of-scope cleanup. But I will let it slide just this once. :P

Committed to 8.5.x, and cherry-picked to 8.4.x as a test-only bugfix. Thanks!

Lendude’s picture

Sorry @xjm! Won't try to sneak out-of-scope stuff in next time :)

Thanks for committing!

Status: Fixed » Closed (fixed)

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