Attached patch adds the phpunit library only, in the interest of simplifying #1901670: Start using PHPUnit for unit tests.

The composer.json is pointed to a fork on my github which is necessary to resolve a conflict with symfony yaml. This can be replaced with the official version once symfony 2.2 is released, which should be very soon.

CommentFileSizeAuthor
#5 d8-vendor-phpunit-5.patch2.84 MBmsonnabaum
#2 d8-vendor-phpunit-2.patch2.79 MBmsonnabaum
#1 d8-vendor-phpunit-1.patch2.83 MBmsonnabaum
d8-vendor-phpunit.patch2.83 MBmsonnabaum
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

msonnabaum’s picture

msonnabaum CreditAttribution: msonnabaum commented
FileSize
d8-vendor-phpunit-1.patch2.83 MB

Accidentally removed the readme.

msonnabaum’s picture

msonnabaum CreditAttribution: msonnabaum commented
FileSize
d8-vendor-phpunit-2.patch2.79 MB

And here's a version with the latest composer, because things change I guess.

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia commented
Status: Needs review » Reviewed & tested by the community

The concept of using PHPUnit got good support in #1901670: Start using PHPUnit for unit tests and elsewhere, and that issue's integration with Drupal is pretty far along, so makes sense to me to get the library in first, and let that issue go through some additional reviews and rerolls without 3MB of patch baggage.

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia commented
Status: Reviewed & tested by the community » Needs work

I RTBC'd #1. #2 is missing composer.json.

msonnabaum’s picture

msonnabaum CreditAttribution: msonnabaum commented
FileSize
d8-vendor-phpunit-5.patch2.84 MB

I'm dumb. One more time.

msonnabaum’s picture

msonnabaum CreditAttribution: msonnabaum commented
Status: Needs work » Needs review
effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia commented
Category: feature » task
Priority: Normal » Major
Status: Needs review » Reviewed & tested by the community

Thanks. Also, recategorizing as a major task, as we've done for prior "add this library" issues.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Assigned: Unassigned » Dries

I agree with moving over to phpunit for unit tests in general, although I haven't reviewed the other issue in any depth yet.

Dries usually likes to commit new libraries so moving over to him.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Now go here: http://drupal.org/node/1901670. :)

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Fixed » Active

Ugh. Wait a moment.

We intensively discussed this topic in the past and everyone agreed that it was one of the worst ideas ever to include simpletest in core and that we do not want to include PHPUnit in core.

Here's the more elaborate battle plan: #1567500: [meta] Pave the way to replace the testing framework with PHPUnit and possibly rewrite Simpletest

msonnabaum’s picture

msonnabaum CreditAttribution: msonnabaum commented
Status: Active » Fixed

Vendoring something is very different than what we did with simpletest. See the other issue for why this is necessary.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Fixed » Active

The only reason I see over there is this:

I chose to vendor phpunit because it enables us to ship with everything you need to run tests, and it also makes it possible to retain support for using the simpletest UI runner.

We decided against that, because

1) Literally every other PHP project that is using PHPUnit requires you to set up PHPUnit on your own, so PHPUnit is expected to exist on your machine already. Due to that, there's also plenty of documentation out there already.

2) A UI-based test runner should not be part of Drupal core in the first place. It may continue to exist in contrib, but there are actually dedicated, non-Drupal PHPUnit UIs available already.

I really do not think we want to include PHPUnit itself in core.

msonnabaum’s picture

msonnabaum CreditAttribution: msonnabaum commented

The main reason is the UI because it's completely out of scope to even discuss the removal of the simpletest UI since there's no way that will happen for D8.

If we don't want to vendor PHPUnit, we need to not ship with any vendored libraries and require a composer install, but that's also way out of scope for this issue.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

Normally, you install and set up PHPUnit via PEAR or their PHAR in a system-wide location on your machine. I've never seen any other PHP project that directs you to install PHPUnit within/into the code base of your PHP application. Why would you want to install it into Drupal?

Also, I don't see why removing the Simpletest UI can't still happen for D8. To run existing tests, we only need the base test classes and test runner code.

msonnabaum’s picture

msonnabaum CreditAttribution: msonnabaum commented

Global installs are common because that's all we had with pear. In the ruby/node world it's not uncommon at all to have your test runner included in your dependencies and run a local version.

You're right in that no other project uses a vendored copy of PHPUnit, but other projects also don't commit their vendor dirs. That's a decision we made, so I'm just rolling with that. I'm all for not doing that, but that's a separate issue.

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia commented

but other projects also don't commit their vendor dirs

That's a red herring. Do any other projects add PHPUnit to their composer.json, regardless of whether they commit their /vendor directory to their codebase?

I don't see why removing the Simpletest UI can't still happen for D8

Wouldn't it make sense then to leave PHPUnit in our codebase until after we remove Simpletest UI, and then remove PHPUnit at that time?

msonnabaum’s picture

msonnabaum CreditAttribution: msonnabaum commented

It's not quite a red herring, I just left out some important details.

The right way to do this would be to add it to require-dev:

http://getcomposer.org/doc/04-schema.md#require-dev

I should have done that, but we can change that when we move back to the official repo.

Here's an example of a php project doing this:

https://github.com/guzzle/guzzle/blob/master/composer.json#L58

And here's official PHPUnit documentation on this method:

http://www.phpunit.de/manual/current/en/installation.html#installation.c...

That said, require-dev doesn't change the fact that we decided to ship with all our dependencies committed to our repository. I'd love to not do that, but as I said before, it's out of scope for this issue.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Assigned: Dries » Unassigned
Status: Active » Fixed

Doesn't look like there's anything left to fix here? And in any case, Dries doesn't need to be assigned anymore.

Status: Fixed » Closed (fixed)

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