-
Notifications
You must be signed in to change notification settings - Fork 73
Conversation
snapshotpl
commented
Mar 4, 2017
•
edited
Loading
edited
- Improve deps testing,
- add php 7.1,
- php7.* must pass tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice PR, I left a few comments but quite like the PR :)
.travis.yml
Outdated
matrix: | ||
include: | ||
- php: 5.4 | ||
env: DEPS=lowest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
COMPOSER_FLAGS='--prefer-lowest'
.travis.yml
Outdated
include: | ||
- php: 5.4 | ||
env: DEPS=lowest | ||
- php: 5.4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can skip the flags here as it's the default behaviour
.travis.yml
Outdated
- php: 5.5 | ||
env: DEPS=lowest | ||
- php: 5.5 | ||
env: DEPS=latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doubling the number of tests looks a bit too much to me. Having a test for the two extrema plus one for each version is enough I think
.travis.yml
Outdated
env: DEPS=latest | ||
- php: 7.1 | ||
env: DEPS=lowest | ||
- php: 7.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we're missing a nightly
build that should be allowed to fail
.travis.yml
Outdated
@@ -28,8 +50,9 @@ before_install: | |||
- travis_retry composer self-update |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is no longer necessary, although it's not related to this PR I think you can take the opportunity to remove it
.travis.yml
Outdated
@@ -28,8 +50,9 @@ before_install: | |||
- travis_retry composer self-update | |||
|
|||
before_script: | |||
- travis_retry composer install --no-interaction | |||
- if [[ $DEPS == 'latest' ]]; then travis_retry composer update --no-interaction --prefer-source ; fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With my first comment, this [the two added lines] can be simplified to composer update --no-interaction --prefer-source $COMPOSER_FLAGS
.travis.yml
Outdated
|
||
script: | ||
- vendor/bin/phpunit --coverage-text | ||
- if [[ $TEST_COVERAGE == 'true' ]]; then vendor/bin/phpunit --coverage-text ; else vendor/bin/phpunit ; fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if [ -n "$TEST_COVERAGE" ]; then
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Single brackets are also enough AFAIK
.travis.yml
Outdated
@@ -1,21 +1,43 @@ | |||
language: php | |||
sudo: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also no part of the goal of this PR, can be removed as well
.travis.yml
Outdated
env: DEPS=lowest | ||
- php: hhvm | ||
env: DEPS=latest | ||
- php: hhvm-nightly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HHVM nightly is no longer supported on TravisCI - travis-ci/travis-ci#3788, so it should be removed
Could you please adapt also those changes in our PR: composer.json
new PHPUnit 6.0 does not support tap parameter, TAP format is not supported any more in PHPUnit 6 - https://github.com/sebastianbergmann/phpunit/blob/master/ChangeLog-6.0.md#removed features/version.feature |
@piotr-zuralski I think this changes what you said should be part of other pull requrest. |
@snapshotpl Those changes are already waiting in other PRs and without them builds will keep failing. |
Ok, so when they will be merged into develop/master I can rebase it here. There's no sense to duplicate work |
.travis.yml
Outdated
fast_finish: true | ||
allow_failures: | ||
- php: 7.0 | ||
- php: hhvm | ||
- php: hhvm-nightly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no hhvm-nightly
build now :)
.travis.yml
Outdated
before_script: | ||
- travis_retry composer install --no-interaction | ||
- composer update --no-interaction --prefer-source $COMPOSER_FLAGS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there some reason to put --prefer-source
here? Installing source packages takes more time than dist ones.
.travis.yml
Outdated
|
||
script: | ||
- vendor/bin/phpunit --coverage-text | ||
- if [[ $TEST_COVERAGE == 'true' ]]; then vendor/bin/phpunit --coverage-text ; else vendor/bin/phpunit ; fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about introducing $PHPUNIT_FLAGS
? This way we can remove if
from this statement.
Closing and reopening to force build on |
Give 10 minutes to fix that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two suggestions from me.
.travis.yml
Outdated
|
||
matrix: | ||
include: | ||
- php: 5.4 | ||
env: COMPOSER_FLAGS='--prefer-lowest' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be simpler to just add an env block later? e.g.
env:
matrix:
- COMPOSER_FLAGS="--prefer-lowest"
- COMPOSER_FLAGS=""
Keeps the list of flags and versions distinct?
.travis.yml
Outdated
|
||
matrix: | ||
include: | ||
- php: 5.4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per #226, we can remove the EOL php versions 5.3 to 5.5.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last round :)
.travis.yml
Outdated
- vendor/bin/phpunit --coverage-text | ||
- vendor/bin/behat | ||
- vendor/bin/phpunit $PHPUNIT_FLAGS | ||
- vendor/bin/behat -vv |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should put it in verbose mode, however we could do -f progress --stop-on-failure
and add a --stop-on-failure
for PHPUnit as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-f progress
- good idea. Verbose can be helpful for debugging without running on local machine.
--stop-on-failure
? I think is better to get all issues in one build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
both non verbose + progress + fail fast are for fast builds detecting as early as possible if things pass or not. Figuring out why should be done locally, and for issues where it's hard to reproduce locally the user can always change the verbosity.
.travis.yml
Outdated
before_script: | ||
- travis_retry composer install --no-interaction | ||
- composer update --no-interaction --prefer-dist $COMPOSER_FLAGS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add --no-suggest
as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make a sense
.travis.yml
Outdated
matrix: | ||
include: | ||
- php: 5.6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
versions should be strings
.travis.yml
Outdated
- php: hhvm | ||
|
||
env: | ||
matrix: | ||
- COMPOSER_FLAGS="--prefer-lowest" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to stay consistent let's keep it at single quotes :)
This PR shows that humbug's tests don't work well with phpunit in version 5.0. Should we remove phpunit >5.0 features or change minimum version of phpunit? |
Yes, there's been a lot of drift over time so it's messy getting the CI just right. The basic migration plan is to do just enough to make Humbug pass in its present state. Once "just enough" is accomplished, we're going straight to 2.0 on Humbug, and we can support PHPUnit 5-6 any way we want. That may require paring back on PHPUnit features for a brief period (e.g. using exception annotations, not the methods, and so on, which are the same across 4,5&6) - also we have hard upper limits (Humbug 1 needs TAP support, for e.g.). |
PHPUnit compatibility improved. However php 7 still fall. But it's problem for another PR, because this scenario alread falls in master https://travis-ci.org/humbug/humbug/jobs/223862748 . disable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍, commits need to be squashed on merge though :)
Thanks @snapshotpl! |
I'm really missing the thanks in the PRs lately... soz about that |
Update README.md markdown fix Update README.md Add migration notice & update clone URL Update Travis CI and Scrutinizer badges in readme Apply fixes from StyleCI Run Travis builds for all branches Unify version displaying format Add a download counter to the readme (humbug#222) Make Behat tests up and running (humbug#224) Forces humbug to use the phpunit downloaded using Composer. Handle mocking objects for both PHPUnit 4 and 5 (humbug#226) Support for optionally configuring phpunit executable (humbug#155) Change PHAR compression from bz2 to gzip (humbug#205) Allow to be run with phpdbg (humbug#184) Remove white spaces Improve Travis matrix (humbug#207) Support adding env variables; by default use this to disable Symfony deprecation notices. Fixes @42