Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow PHPUnit to bootstrap from core. #3007

Closed
wants to merge 8 commits into from

Conversation

danepowell
Copy link
Contributor

@danepowell danepowell commented Aug 13, 2018

As part of #2982, we created our own PHPUnit bootstrap.php file in order to support multiple versions of PHPUnit.

This created a bit of a problem for folks testing Drupal core and modules. Because BLT forces you to use that bootstrap file, you can't use core's own bootstrap.php, and thus if you try to run a PHPUnit test according to the BLT docs you'll end up with errors like this:

PHP Fatal error: Class 'Drupal\Tests\UnitTestCase' not found in docroot/modules/contrib/simple_oauth/tests/src/Unit/EntityCollectorTest.php on line 19

I see a few ways to tackle this problem. In general, I question the wisdom in trying to maintain our own copy of bootstrap.php. Why not just use core's? And why not simply delegate the choice of which bootstrap file to use to phpunit.xml?

This PR takes a middle road by just removing the CLI argument forcing you to use BLT's bootstrap file. But I'd love to hear additional arguments for or against removing our custom bootstrap file altogether, and just use core's instead.

@mikemadison13
Copy link
Contributor

@danepowell FYI we CAN bootstrap from core right now using the config key in the phpunit options in blt.yml. See #1435. I'm not sure if this is significantly different than what you are trying to do? but you should be able to do this and bootstrap core without any code changes today:

phpunit:
  -
    path: '<your test directory>'
    config: '${docroot}/core/phpunit.xml.dist'

@ba66e77
Copy link
Contributor

ba66e77 commented Aug 29, 2018

If the config approach solves the issue of being able to bootstrap and run the tests, then the question is more about maintainability and if we should be trying to run a custom bootstrap.php.

My default stance is to remove what we don't actually need, but I don't have strong feeling about it in this case. @danepowell I assume you're advocating for removing it. @mikemadison13 are you advocating for keeping it?

* Update local-development.md to reference committing DrupalVM files.
@ba66e77
Copy link
Contributor

ba66e77 commented Aug 30, 2018

I tested this by applying this patch and adding the following lines to my blt/local.blt.yml:

phpunit:
  -
    config: ${docroot}/core/phpunit.xml.dist
    path: ${docroot}/core/modules/views/tests/src/Unit

Running those, I get successful runs of the views tests:

$ blt tests:phpunit
[Testing\PHPUnit] Running PHPUnit  --log-junit /var/www/newblt/reports/phpunit/results.xml /var/www/newblt/docroot/core/modules/views/tests/src/Unit -v --configuration /var/www/newblt/docroot/core/phpunit.xml.dist
[Testing\PHPUnit] Running /var/www/newblt/vendor/bin/phpunit --log-junit /var/www/newblt/reports/phpunit/results.xml /var/www/newblt/docroot/core/modules/views/tests/src/Unit -v --configuration /var/www/newblt/docroot/core/phpunit.xml.dist in /var/www/newblt/docroot/core/modules/views/tests/src/Unit
PHPUnit 6.5.12 by Sebastian Bergmann and contributors.

Runtime:       PHP 7.1.20-1+ubuntu16.04.1+deb.sury.org+1
Configuration: /var/www/newblt/docroot/core/phpunit.xml.dist

Testing /var/www/newblt/docroot/core/modules/views/tests/src/Unit
...............................................................  63 / 317 ( 19%)
............................................................... 126 / 317 ( 39%)
............................................................... 189 / 317 ( 59%)
............................................................... 252 / 317 ( 79%)
............................................................... 315 / 317 ( 99%)
..                                                              317 / 317 (100%)

Time: 6.55 seconds, Memory: 42.00MB

OK (317 tests, 1172 assertions)

Remaining vendor deprecation notices (18)

  18x: The Drupal\views\ViewsData::get method is deprecated (NULL $key deprecated in Drupal 8.2.x and will be removed in 9.0.0. Use getAll() instead.).
    4x in ViewExecutableTest::testAddHandler from Drupal\Tests\views\Unit
    4x in ViewExecutableTest::testAddHandlerWithEntityField from Drupal\Tests\views\Unit
    2x in HandlerBaseTest::testGetEntityTypeForFieldWithRelationship from Drupal\Tests\views\Unit\Plugin
    1x in HandlerBaseTest::testGetEntityTypeForFieldOnBaseTable from Drupal\Tests\views\Unit\Plugin
    1x in FieldTest::testAccess from Drupal\Tests\views\Unit\Plugin\field
    1x in FieldTest::testQueryWithGroupByForBaseField from Drupal\Tests\views\Unit\Plugin\field
    1x in FieldTest::testQueryWithGroupByForConfigField from Drupal\Tests\views\Unit\Plugin\field
    1x in ViewsDataHelperTest::testFetchFields from Drupal\Tests\views\Unit
    1x in ViewsHandlerManagerTest::testGetHandlerBaseInformationPropagation from Drupal\Tests\views\Unit
    1x in ViewsHandlerManagerTest::testGetHandlerOverride from Drupal\Tests\views\Unit
    1x in ViewsHandlerManagerTest::testGetHandlerNoOverride from Drupal\Tests\views\Unit

[Testing\PHPUnit] Done in 6.607s

Caveat I had to make sure I was on 9.x-dev and where the symfony/phpunit-bridge package is added to the BLT require-dev set.

ba66e77 and others added 5 commits August 30, 2018 14:26
* Adding replication and verification step sections and clean up formatting.

* Adding new guidelines for PRs to the CONTRIBUTING docs.
… tag to source repo. (acquia#2992)

* Adding a cutSourceTag method and invoking when deploying tag.

* Set default deploy.tag_source config setting and check it when deploying.

* Refactoring to use a single cutTag method.

* Remove line about expectation you already have a tag on your source before you tag a release.

* Warn if user is creating a tag but tag_source option is false.

* Updating deploy documentation.
@ba66e77 ba66e77 changed the base branch from 9.x to 9.2.x September 7, 2018 10:34
@ba66e77 ba66e77 added the Enhancement A feature or feature request label Sep 7, 2018
@danepowell
Copy link
Contributor Author

It looks like updating the PR pulled in some extraneous commits 😢

@ba66e77
Copy link
Contributor

ba66e77 commented Sep 7, 2018

Closing in favor of #3071, which just cherry-picks the meaningful bit of this PR into the 9.2.x branch.

@ba66e77 ba66e77 closed this Sep 7, 2018
@ba66e77 ba66e77 changed the base branch from 9.2.x to 9.x September 7, 2018 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement A feature or feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants