Skip to content

Test lowest version constraints #114

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.

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

Merged
merged 1 commit into from
Feb 13, 2017
Merged

Conversation

andig
Copy link
Contributor

@andig andig commented Feb 11, 2017

Follow-up of #101 and #111.

See https://travis-ci.org/andig/http/builds/200662157 for sample output. It already shows that ringcentral:1.0 is broken. https://travis-ci.org/andig/http/jobs/200662167 shows some additional errors that should be investigated once ringcentral/psr7 is upgraded to 1.2.

Note: this doubles the build matrix

@WyriHaximus
Copy link
Member

Doubling is a wee bit much tbh. This adds only one build:

env:
  - DEPENDENCIES=default

matrix:
  include:
    - php: 7.0
      env:
        - DEPENDENCIES=lowest

@andig
Copy link
Contributor Author

andig commented Feb 11, 2017

Then for the sake of it lets to the same for the lowest supported version 5.3, too?

@andig
Copy link
Contributor Author

andig commented Feb 11, 2017

What confuses me now is that on php 5.3 composer picks ringcentral/psr7:1.2 (which I wouldn't expect?) while on php 7.0 it chooses 1.0?

@jsor
Copy link
Member

jsor commented Feb 11, 2017

ringcentral/psr7:1.0 requires php: >=5.4.0 (thats why it can be picked by the PHP7 build), since ringcentral/psr7:1.2 it has been lowered to >=5.3, so that's the lowest version the PHP5.3 build can pick.

@andig
Copy link
Contributor Author

andig commented Feb 11, 2017

Then something else is broken in the 7.0 build.

- Updating phpunit/phpunit (4.8.35 => 4.8.0) Downloading: Connecting... Downloading: 100%

$ ./vendor/bin/phpunit --coverage-text
PHPUnit v0.4.3-14-g8d2e2c9 by Sebastian Bergmann and contributors.

Since phpunit 4.8 is installed that might have been bogus- the version number definitely looks borked.

update should use 4.8.10 at least: https://github.com/sebastianbergmann/phpunit/blob/4.8.32/ChangeLog-4.8.md

@clue clue modified the milestone: v0.4.4 Feb 11, 2017
@clue
Copy link
Member

clue commented Feb 13, 2017

I'd like to look into getting the release out :shipit: What is the status here? Do you think this PR is still needed?

IMHO we don't really need this, but I see no harm in adding this anyway if you feel that this adds value. 👍

Technically, each and every minor version of our dependencies could have introduced breaking changes, but I rarely see this being an actual issue and the latest version is regularly tested via CI. Also testing the lowest supported version adds some value, but ultimately if we'd want to be 100% sure we'd have to test every possible combination of our dependencies :-)

@andig
Copy link
Contributor Author

andig commented Feb 13, 2017 via email

Copy link
Member

@clue clue left a comment

Choose a reason for hiding this comment

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

Alright, changes LGTM, can you rebase on current master now that #113 and #115 are in? 👍

@andig
Copy link
Contributor Author

andig commented Feb 13, 2017

Will do, but to learn something: why is this necessary? Targeting different files here?

@clue
Copy link
Member

clue commented Feb 13, 2017

Will do, but to learn something: why is this necessary?

The branch behind this PR is based on an older version of the master branch, one which did not specify the proper minimum version requirements. Hence, trying to test minimum versions on this branch does current fail, see e.g. Travis.

In the meantime, the master branch has been updated and we now specify proper minimum version requirements (#113 and #115). However, we do not currently run any tests on the master branch for older versions (and it's debatable whether this is needed in the first place).

If you rebase this branch on the current master, then this branch will use the proper minimum version requirements.

If we were to accept this PR now without rebasing on master first, then we risk accepting a PR that does technically not build currently, but will will build once it's merged.

I hope this helps 👍

@andig
Copy link
Contributor Author

andig commented Feb 13, 2017

If we were to accept this PR now without rebasing on master first, then we risk accepting a PR that does technically not build currently, but will will build once it's merged.

Actually, I was assuming that retriggering the travis build (which anybody with sufficient privileges can do but not me) would apply it to current master and then test to get valid results. As said, really no problem and will do asap once I'm back at the machine.

@clue
Copy link
Member

clue commented Feb 13, 2017

Thanks for keeping this updated! 👍

retriggering the travis build (which anybody with sufficient privileges can do but not me) would apply it to current master

This may be true from a practical (sane!) view point :-) I'm tempted to disagree from a theoretical view point, but I'm not really that much into discussing theoretical aspects of this, so I appreciate you've rebased this on current master already, which I still believe is the cleanest way of addressing this :-)

@WyriHaximus WyriHaximus requested a review from jsor February 13, 2017 11:30
@WyriHaximus WyriHaximus merged commit acc7a22 into reactphp:master Feb 13, 2017
@WyriHaximus
Copy link
Member

Awesome, thank you 👍

@andig andig deleted the composer branch October 25, 2017 06:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants