This repository has been archived by the owner. It is now read-only.

Fix: Require phpunit/phpunit as dev dependency #1075

Closed
wants to merge 5 commits into
base: 1.2
from

Conversation

Projects
None yet
@localheinz
Contributor

localheinz commented Dec 14, 2014

The section running tests in about https://github.com/silexphp/Silex/blob/1.2/README.rst#tests is currently lying, and there are two options

  • require users to install phpunit globally, remove the instructions on how to install dev depencencies, and change the line for running the tests to $ phpunit or
  • require phpunit/phpunit as a dev depenceny
@henrikbjorn

This comment has been minimized.

Contributor

henrikbjorn commented Dec 14, 2014

phpunit should not be required, the doc should be updated instead.

The resoning for this is that it is a tool that is installed globally most of the time, just as phpunit. You wouldnt require composer/composer for every project, right?

@igorw

This comment has been minimized.

Contributor

igorw commented Dec 14, 2014

👍 to making phpunit a dev dependency

@GrahamCampbell

This comment has been minimized.

GrahamCampbell commented Dec 14, 2014

👎 from me.

@henrikbjorn

This comment has been minimized.

Contributor

henrikbjorn commented Dec 14, 2014

@igorw why ? We should add composer aswell then

@GrahamCampbell

This comment has been minimized.

GrahamCampbell commented Dec 14, 2014

@henrikbjorn Are you serious? Composer would be installing composer!

@GrahamCampbell

This comment has been minimized.

GrahamCampbell commented Dec 14, 2014

Oh, just read your earlier comment. That was your point...

@localheinz

This comment has been minimized.

Owner

localheinz commented on d47031f Dec 14, 2014

Probably makes sense to use the same version on Travis then, too.

@localheinz

This comment has been minimized.

Contributor

localheinz commented Dec 14, 2014

@henrikbjorn

Why would it make sense to require composer/composer when you need Composer to install it?

But, it makes sense to require a certain version of PHPUnit because your test suite may depend on features that only are available with that certain version, but not with one before that. You may want to see a test fail on Travis for the same reason it failed locally, as you're requiring the same version, wouldn't you?

@GrahamCampbell

This comment has been minimized.

GrahamCampbell commented Dec 14, 2014

@localheinz It doesn't. It was sarcasm I assume...

@davedevelopment

This comment has been minimized.

Contributor

davedevelopment commented Dec 14, 2014

Just for the record, I don't care either way, let's just get it right.

@localheinz

This comment has been minimized.

Contributor

localheinz commented Dec 14, 2014

Just to make it clear:

If you

  • clone this project
  • install dependencies with composer

you're apparently interested to make changes to it. If you make changes, you need tests - because we all do TDD, right? So it's clear that phpunit/phpunit is a development requirement. End of story to me.

@stof

This comment has been minimized.

Contributor

stof commented Dec 15, 2014

This has already been rejected previously, and here is the reason: installing PHPUnit through composer requires installing 14 or 15 packages. Given Travis already has PHPUnit available, this means wasting some time for each build with no benefit. And Travis is the main place where the testsuite is running.
The recommended way to get PHPUnit is through the phar

@localheinz

This comment has been minimized.

Contributor

localheinz commented Dec 15, 2014

@stof

How much time?

@localheinz

This comment has been minimized.

Contributor

localheinz commented Dec 15, 2014

@stof

Seriously, if you're waiting for your build to pass and haven't got anything to do in the meantime, you're doing it wrong.

@davedevelopment

This comment has been minimized.

Contributor

davedevelopment commented Dec 15, 2014

I think the consensus is that it's not all that friendly for travis (and ultimately everyone else using travis) or for the environment.

@localheinz

This comment has been minimized.

Contributor

localheinz commented Dec 15, 2014

Using PHPUnit provided by Travis

screen shot 2014-12-15 at 15 47 20

Using PHPUnit installed with Composer

screen shot 2014-12-15 at 15 47 00

@henrikbjorn

This comment has been minimized.

Contributor

henrikbjorn commented Dec 15, 2014

I dont want it installed locally by EVERY project, even if it would install it, i would run the tests with the binary already in my path.

It is used by almost every project and therefor you should already have it installed.

@igorw

This comment has been minimized.

Contributor

igorw commented Dec 15, 2014

So basically the silex dependencies are adjusted to the global environment of a third party that is a build service. And must upgrade whenever it chooses to upgrade. And will only get new versions once travis provides them.

We do not do this for any other dependency because it is completely insane. Why are we doing it for PHPUnit?

@dcousineau

This comment has been minimized.

dcousineau commented Dec 15, 2014

👍 to adding it as a dev dependency. There are multiple reasons why this a good idea:

  1. The age of composer is an age of completely self contained dependencies in a project. This means everything including dev tools, of which PHPUnit is a dev tool. This is the same path being taking by npm and other language workflows (case in point: socket.io includes its test runners as devDependencies and does not expect any global dependencies other than node/npm)
  2. This will allow Silex to peg a specific version of PHPUnit and reduce confusion in ticket reporting by eliminating bugs due to PHPUnit version discrepancies
  3. This will reduce new contributor friction and development environment pains

With composer's caching system and most projects in PHP moving in the direction of including PHPUnit via composer.json this shouldn't significantly add to setup/composer install time.

As for the "you should already have it installed" argument, ignoring other concerns, you should already have what version installed? If random developer A can't upgrade past a certain version because of an internal project they get paid to work on and Silex expects behaviors introduced in a later version, is said developer SOL for contributing to silex?

@blongden

This comment has been minimized.

Contributor

blongden commented Dec 15, 2014

Agree with everything @dcousineau just said. 👍 from me, fwiw.

@hkdobrev

This comment has been minimized.

Contributor

hkdobrev commented Dec 15, 2014

I have always been in favour of keeping PHPUnit as a dev dependency. But the download/install time has been preventing me to persuade my colleagues to require PHPUnit in Composer for our company projects.

The differences in the build times provided in the screenshot above are totally negligible. Especially for an open-source library.

Big +1 for everything @igorw said. PHPUnit is now rapidly releasing new versions and Travis CI is keeping up with that. Only a small amount of tests are PHPUnit-version-specific. But when there is a problem with your tests, because of the PHPUnit version and you change your tests just to keep up with new versions of dependencies which are not locked or even specified anywhere is not nice at all.

Also don't underestimate the problem when one contributor could have one version of PHPUnit locally, the reviewer of a pull request another and Travis CI an even different version. This could take a lot of time to solve. It would be better if everyone could be on the same page with vendor/bin/phpunit and know if the tests pass or not. We could even use the new composer test which I hope would soon be the most popular way to define how a library is tested much like in the npm world.

Just my ¢2


P.S.

Don't forget that Travis CI would always be right to ship (and test) with PHPUnit by default:

  1. PHPUnit is the most popular testing tool in the PHP world and they are including one per language.
  2. They want to support really easy onboarding even for people not familiar with Composer.

hkdobrev added a commit to estaty/estaty that referenced this pull request Dec 15, 2014

Consolidate testing environment
- Remove `tests/bootstrap.php` by:
  * Using `autoload-dev` in Composer instead of
    manually adding namespaces to its autoloader on the fly
  * Using PHPUnit `<ini>` XML configuration to set up
    `display_errors`, `error_reporting` and `date.timezone`
- Require `phpunit/phpunit` as a dev dependency instead of depending
  on the platform installation.
  See discussion about that here: silexphp/Silex#1075
- Use new Composer custom commands with `scripts`.
  `composer run-script test` is now just `composer test`.

So you could now just `composer install` and `composer test`. Everything
else is taked care of in a minimalistic, but still well-defined way.
@localheinz

This comment has been minimized.

Contributor

localheinz commented Dec 16, 2014

@hkdobrev

Thanks for pointing out composer custom scripts, wasn't aware of that great feature.

@alcohol

This comment has been minimized.

Contributor

alcohol commented Dec 16, 2014

Why is this is even a debate? A cached install takes only a second or two. Insignificant compared to the benefits it delivers.

@henrikbjorn

This comment has been minimized.

Contributor

henrikbjorn commented Dec 16, 2014

It does not deliver any benefit for me in any project, neither would it give any benefit to my team at work as all have it installed globally.

@davedevelopment

This comment has been minimized.

Contributor

davedevelopment commented Dec 17, 2014

Travis have announced caching for open source projects in their recent infrastructure announcement, so maybe the "travis having to download things" reasons against can be discounted? I think we'd just need to add a key or two to the .travis.yml

@localheinz localheinz force-pushed the localheinz:fix/phpunit branch from 7a809c1 to b3bb1c6 Dec 17, 2014

@localheinz

This comment has been minimized.

Contributor

localheinz commented Dec 17, 2014

@davedevelopment

Thanks for pointing that out, seems like this could be a win for everyone, almost.

@hkdobrev

This comment has been minimized.

Contributor

hkdobrev commented Dec 17, 2014

@localheinz You need to explicitly enable caching as per the docs:

cache:
  directories:
    - $HOME/.composer/cache
@localheinz

This comment has been minimized.

Contributor

localheinz commented Dec 17, 2014

@hkdobrev

So I guess b3bb1c6 just covers part 1 of 3 2, no? Documentation says this should already make the build start up faster.

@hkdobrev

This comment has been minimized.

Contributor

hkdobrev commented Dec 17, 2014

@localheinz This would start the build faster, but would not cache PHPUnit with the Composer cache between builds.

@localheinz

This comment has been minimized.

Contributor

localheinz commented Dec 17, 2014

Not really sure if these numbers can be compared in any way (it's just two)

but to me it seems like it's a lot faster, doesn't it?

Of course, it would probably be even faster if only the last two commits were cherry-picked and phpunit/phpunit were not installed as a dev dependency.

@davedevelopment

This comment has been minimized.

Contributor

davedevelopment commented Dec 17, 2014

🎆 Big 👍 from me now 🎆

:shipit:

@hkdobrev

This comment has been minimized.

Contributor

hkdobrev commented Dec 17, 2014

@localheinz Actually the caching will take effect in the next build. In the last build, it only cached the ~/.composer/cache folder, it will be able to re-use it the next time. As you can see it did a lot of cloning: https://api.travis-ci.org/jobs/44383980/log.txt?deansi=true

@davedevelopment

This comment has been minimized.

Contributor

davedevelopment commented Dec 31, 2014

Just realised we --prefer-source, so the cache wont have any effect?

@alcohol

This comment has been minimized.

Contributor

alcohol commented Dec 31, 2014

True. Though I am not sure why one would want to use --prefer-source on travis to be honest.. does anyone recall why this is the case?

@hkdobrev

This comment has been minimized.

Contributor

hkdobrev commented Dec 31, 2014

Composer uses the GitHub API for finding and downloading tarballs. When the GitHub API rate limit is reached the composer install will fail. The --prefer-source option fixes that by always using Git cloning. There is a better solution to that though: You use the --no-interaction flag which would automatically try to Git clone a dependency if the tarball option fails.

@stof

This comment has been minimized.

Contributor

stof commented Dec 31, 2014

When the GitHub API rate limit is reached the composer install will fail.

This is not true anymore since at least 3 months (even more than that I thinkk but I'm not sure). If it cannot download the archive, Composer will now automatically fallback to a source install.
So IMO, it makes sense to remove the --prefer-source now

@GrahamCampbell

This comment has been minimized.

GrahamCampbell commented Dec 31, 2014

@stof Yeh, but it's still selfish to use the api thus using the limited number calls for that vm for the hour when we don't need them, when someone else might need to use the api on the same vm later.

@stof

This comment has been minimized.

Contributor

stof commented Dec 31, 2014

@GrahamCampbell removing --prefer-source should be done at the same time you start using the build cache for the Composer cache folder. this way, most dist downloads will be loaded from cache, not from the API

@localheinz

This comment has been minimized.

Contributor

localheinz commented Jan 7, 2015

@stof

If you like, I can go ahead and make these changes right here.

@fabpot

This comment has been minimized.

Member

fabpot commented Mar 6, 2015

Closing in favor of #1078

@fabpot fabpot closed this Mar 6, 2015

fabpot added a commit that referenced this pull request Mar 6, 2015

minor #1078 [1.2] Fix doc about running unit tests (GromNaN)
This PR was merged into the 1.2 branch.

Discussion
----------

[1.2] Fix doc about running unit tests

Alternative to #1075.

I've kept the syntax using phpunit.phar as it must be the most portable way to run PHPUnit (like it is for Composer).

Commits
-------

7bb4398 Fix README doc about running unit tests

@localheinz localheinz deleted the localheinz:fix/phpunit branch Mar 6, 2015

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.