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.

Sign up for GitHub

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

Jump to bottom

Rely on own phpunit, not one from CI service #1995

Closed
wants to merge 3 commits into from
Closed

Rely on own phpunit, not one from CI service #1995

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d5832f9
Rely on own phpunit, not one from CI service
keradus Jun 15, 2016
509c4dc
Update .travis.yml
keradus Jun 15, 2016
236dc72
Update composer.json
keradus Jun 16, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 4 additions & 1 deletion .travis.yml
View file
Open in desktop
Expand Up @@ -21,13 +21,16 @@ cache:
- $HOME/.composer/cache

before_install:
- mv $HOME/.phpenv/versions/$(phpenv version-name)/etc/conf.d/xdebug.ini $HOME/xdebug.ini || return 0
- 'if [ "$SYMFONY_VERSION" != "" ]; then sed -i "s/\"symfony\/\([^\"]*\)\": \"[^\"]*\"/\"symfony\/\1\": \"$SYMFONY_VERSION\"/g" composer.json; fi'

install:
- travis_retry composer global require hirak/prestissimo
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This actually ends up slower surely? It takes just as long to install this globally as it does to install the rest.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean about the time to insstall this library?

Dependencies for this one is very light, it should not be a problem.

@keradus I would ever say that the travis_retry should not be here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see no reason to drop it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

travis_retry is good for long with no output jobs.

This command would not take more than 10 minutes to execute.

Plus, travis_retry drop the original output of the command, harder to read.

This is as you want, but I suggest you to remove this. 👍

- travis_retry composer update ${COMPOSER_FLAGS} --no-interaction

script:
- phpunit --verbose --coverage-clover build/logs/clover.xml
- cp $HOME/xdebug.ini $HOME/.phpenv/versions/$(phpenv version-name)/etc/conf.d/xdebug.ini || return 0
- vendor/bin/phpunit --verbose --coverage-clover build/logs/clover.xml
- phpenv config-rm xdebug.ini || return 0
- php php-cs-fixer --diff --dry-run -v fix

Expand Down
3 changes: 2 additions & 1 deletion composer.json
View file
Open in desktop
Expand Up @@ -25,7 +25,8 @@
"sebastian/diff": "~1.1"
},
"require-dev": {
"satooshi/php-coveralls": "0.7.*@dev"
"phpunit/phpunit": "^4.5|^5",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👎

If we are able to not make it dependents, don't do it.

This will install a lot of dependencies, making composer longer to resolve.

Plus, all developer does not work with vendor/bin/phpunit.

What about directly download the .phar on travis?

Example: https://github.com/sonata-project/dev-kit/blob/a4574f4ff3428bdd89ecf1c2f4f0190227abb250/project/.travis/install_test.sh#L4-L13

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like fetching some deps by Composer and some not. You don't like it? Cool. You are not enforced to download dev deps.
Composer is nicer way to describe required version. like not supporting 4.1

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like fetching some deps by Composer and some not.

As you want but IMHO, it's more a tool than a "dependency".

You don't like it? Cool. You are not enforced to download dev deps.

Not really true for all projects. Some can require dev packages other than phpunit and we can select which one we want to install.

By installing with composer, you get dependencies described by phpunit, not locked by composer.lock like for .phar. This mean you increase the issue risk to get PHPUnit not working because of a sub-dependency update.

So this is not only a matter of taste for me. 😉

Composer is nicer way to describe required version. like not supporting 4.1

Well, if you not support it, tests will fail. :-)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. tool could be a dependency
  2. to develop a project you should have a phpunit. you may have global one - cool, but we don't promise that our project is compatible with it.
  3. theoretically, but such an edge case...
  4. it;s better to say "this one will work, you may use it" than "some phpunit could work, we dont know, try, and if it fails - well... we dont know which version works"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it;s better to say "this one will work, you may use it" than "some phpunit could work, we dont know, try, and if it fails - well... we dont know which version works"

Or simply rely on the last stable version of PHPUnit. 😉

theoretically, but such an edge case...

Are you talking about the risk to get PHPUnit not working because of a sub-dependency update?

Not such an edge case as you think. 😉

Anyway, at least I have an answer. :-)

"satooshi/php-coveralls": "^0.7.1"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, you can get the .phar: https://github.com/sonata-project/dev-kit/blob/a4574f4ff3428bdd89ecf1c2f4f0190227abb250/project/.travis/install_test.sh#L15-L17

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disclaimer: I'm one of those who release php-coveralls

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh OK. 👍

And then? Don't you recommend to use .phar?

I'm curious to know why. 😉

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#1995 (comment)

use whatever you prefer ;)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because the dependencies (and conflicts) are not managed using phar's as these are using composer (for ref.: https://ci.appveyor.com/project/keradus/php-cs-fixer/build/dev-2529)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SpacePossum This could be indeed an issue. But could be.

This is why I said on #1995 (comment):

If we are able to not make it dependents, don't do it.

BTW, a WIP is here concerning this kind of issue: sebastianbergmann/phpunit#2015

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SpacePossum BTW, concerning your ref, this is unrelated.

Related issue: sebastianbergmann/phpunit#2219

},
"autoload": {
"psr-4": { "Symfony\\CS\\": "Symfony/CS/" }
Expand Down