Problem/Motivation

We want d.o to ship a tarball without dev dependencies, for a variety of reasons including security. This means we have to change the build process so it uses composer install --no-dev: #2745355: Use "composer install --no-dev" to create tagged core packages

However, the simpletest module has undeclared dependencies on the phpunit/phpunit package. If we ship a --no-dev tarball, simpletest module will crash when users enable it and visit admin/config/development/testing.

We have an issue for disallowing extensions from enabling if they have unmet composer-based dependencies: #2494073: Prevent modules which have unmet Composer dependencies from being installed However, that issue is moving slowly, and might not be able to address dependencies of core extensions.

Therefore: We need to turn simpletest into a special case and give it the ability to check whether PHPUnit is installed.

Proposed resolution

  • Add a PHPUnit check to simpletest_requirements().
  • Add a PHPUnit check to run-tests.sh.
  • Add a composer.json file to the simpletest module, though currently we have no mechanism by which Composer can discover it.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#11 interdiff.txt464 bytesMile23
#11 2780093_11.patch1.56 KBMile23
#5 interdiff.txt1.11 KBMile23
#5 2780093_5.patch2.01 KBMile23
#2 2780093_2.patch2.01 KBMile23
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Mile23 created an issue. See original summary.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Status: Active » Needs review
FileSize
2780093_2.patch2.01 KB

Patch...

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Issue tags: +run-tests.sh
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented

In general this issue is a great idea IMHO

  1. +++ b/core/modules/simpletest/simpletest.install
@@ -18,9 +18,19 @@
+  $has_phpunit = class_exists('\PHPUnit_Framework_TestCase');

+++ b/core/scripts/run-tests.sh
@@ -34,6 +34,11 @@
+if (!class_exists('PHPUnit_Framework_TestCase')) {

    Potential Nitpick award winner: Either use ::class or at least go with either trailing slash and without training slash but be consistent :) 

  2. +++ b/core/modules/simpletest/simpletest.install
@@ -18,9 +18,19 @@
+    'value' => $has_phpunit ? t('Enabled') : t('Not found'),

    Any reason to not go with "Found" and "Not found"? Enabled is a bit of an odd word for a PHP library

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Version: 8.3.x-dev » 8.2.x-dev
FileSize
2780093_5.patch2.01 KB
interdiff.txt1.11 KB

Potential Nitpick award winner: Either use ::class or at least go with either trailing slash and without training slash but be consistent :)

Interestingly... there are no CS guidelines for .sh files. :-) Also, can't use ::class because it might not exist.

Addressed.

I really should have opened this up as an 8.2.x issue, because it improves testing, which I believe is allowed during beta. It applies either way.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented
Status: Needs review » Reviewed & tested by the community

Thank you @Mile23!

Also, can't use ::class because it might not exist.

Fair point :)

Eric_A’s picture

Eric_A CreditAttribution: Eric_A commented

This might even be considered a bug fix, given that after composer install --no-dev simpletest module will crash when users enable it.

The module level composer.json does not seem to be needed for this task, but surely would be a nice kick-off for a feature request like #1398772: Replace .info.yml with composer.json for extensions.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 5: 2780093_5.patch, failed testing.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Status: Needs work » Reviewed & tested by the community

Moving back to RTBC, re-running tests.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Needs work

Let's not introduce a core module composer.json until the approach and what should go in them is agreed. Everything else in the patch looks fine.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Status: Needs work » Needs review
FileSize
2780093_11.patch1.56 KB
interdiff.txt464 bytes

Reasonable. :-)

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented
Status: Needs review » Reviewed & tested by the community

Well fair

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Issue summary: View changes
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Issue summary: View changes

Testing the premise of the issue...

 8.3.x  alex: ~/dev/sites/drupal8alt.dev > rm -rf vendor/phpunit
 8.3.x  alex: ~/dev/sites/drupal8alt.dev > d en simpletest -y
The following extensions will be enabled: simpletest
Do you really want to continue? (y/n): y
simpletest was enabled successfully.                                                                                                                           [ok]
simpletest defines the following permissions: administer unit tests

However if you go to admin/config/development/testing you'll see

Fatal error: require(): Failed opening required '/Volumes/devdisk/dev/sites/drupal8alt.dev/vendor/composer/../phpunit/phpunit/src/Framework/TestCase.php' (include_path='.:/usr/local/php5/lib/php') in /Volumes/devdisk/dev/sites/drupal8alt.dev/vendor/symfony/class-loader/ApcClassLoader.php on line 110

So there is a true dependency.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed b8500a9 to 8.3.x and bdf69f9 to 8.2.x. Thanks!

  • alexpott committed b8500a9 on 8.3.x 
    Issue #2780093 by Mile23: Have simpletest, run-tests.sh enforce their...

  • alexpott committed bdf69f9 on 8.2.x 
    Issue #2780093 by Mile23: Have simpletest, run-tests.sh enforce their...
Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented

rm -rf vendor/phpunit removes the vendor directory, but doesn't change the autoload dump. Since PHPUnit generates a giant classmap and doesn't use PSR-0/4, the class 'exists.'

A better demo would be

$ rm -rf vendor/
$ composer install --no-dev
$ drush en simpletest

Should we follow-up on the scenario from #14? This seems like a good best-faith effort already.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

@Mile23 yep I think you're right the fix is fine. Hence my commit.

Status: Fixed » Closed (fixed)

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

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Version: 8.2.x-dev » 8.1.x-dev

Hmm... As it turns out, this was never applied to 8.1.x (or lower). So therefore we still can't package 8.1.x without dev. Unless someone were to file an issue like this: #2804663: Have simpletest, run-tests.sh enforce their dependency on PHPUnit in 8.1.x

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Version: 8.1.x-dev » 8.2.x-dev