Reduced the number of travis builds. #149

Nyholm · 2017-04-14T08:51:48Z

Currently, all three listed Symfony versions will be run against every configured PHP versions. Thus, the build matrix does not only get one but three additional jobs. As the Travis CI quota for the number of jobs run in parallel applies to all repositories in the php-http organisation what do you think about reducing the number of jobs here and run tests with different Symfony versions only with one PHP version?

xabbuh · 2017-04-14T10:28:20Z

.travis.yml

-        - SYMFONY_VERSION=3.2.*
-        - SYMFONY_VERSION=3.1.*
-        - SYMFONY_VERSION=2.8.*
+    - SYMFONY_VERSION=2.8.* TEST_COMMAND="composer test"


I think I would by default not enforce any Symfony version at all. And by the way, 2.8 should be handled fine by the job using the --prefer-lowest flag.

We have to do one by default. Otherwise composer require symfony/symfony: will fail, right?

I used 2.8 because it is the only LTS we support.

Oh well, I would run composer require symfony/symfony only if $SYMFONY_VERSION is defined and not empty.

okey, but that would be the same as setting SYMFONY_VERSION=3.2.*

I would maybe also set minimum-stability in the composer.json file to dev so that you would get a build with the latest Symfony development version for free ("normal" builds could use the --prefer-stable flag).

Testing against Symfony 3.3 would be nice. We just have to allow it to fail.

@Nyholm it is not the same than setting SYMFONY_VERSION=3.2.*:

it runs tests using the unmodified composer.json, ensuring that your composer.json has the necessary requirement (remove the symfony requirement currently, and you won't see any failure on Travis as it will always re-add one)

it runs tests against the latest stable version of Symfony, without requiring you to update the version every 6 months.

My own rule for the Travis matrix is this one:

unmodified composer.json for normal jobs (on different PHP version)

extra jobs for supported LTS branches of Symfony

an extra job checking lowest deps (and without changing the Symfony requirement, otherwise this one is not validated)

an extra job changing the minimum-stability to dev, to ensure compatibility with the upcoming dev version (this one may be allowed to fail depending on your policy)

With such config, I only need to change the build matrix when the list of supported LTS versions is changing (every 2 years)

See https://github.com/Incenteev/IncenteevTranslationCheckerBundle/blob/169c90a9bfc9c7475883521a0edfee30c1c141a0/.travis.yml for an example of such setup (with too much supported PHP versions)

I like the approach @stof recommends.

Nyholm · 2017-04-18T19:36:30Z

Thank you for your feedback. I've updated the PR.

dbu · 2017-04-18T19:52:23Z

if i understand correctly, we don't test with symfony 3.1 anymore like this. i would consider that useful even if its not an LTS

dbu

thanks @Nyholm !

stof · 2017-04-19T07:38:29Z

@dbu 3.1 would be tested by normal jobs (unmodified vendors), as it is the latest stable version. And this would switch to 3.2 automatically on release (I don't care about testing 3.1 during the 2 months of support overlap after the 3.2 release, as this again involves changing the Travis matrix all the time).
Note that my proposal implies uses the stable stability by default (which is the default in composer), which is why there is a dedicated job changing it to dev.

Nyholm · 2017-04-19T07:46:34Z

I think you are mixing versions. 3.2 will be tested in normal jobs since it is the latest stable version.

Shouldn't we always test for the lowest and the highest 3.x versions?
If we not are testing with 3.1 (but we support it) we risk of using a feature/class that was introduced in 3.2.

stof · 2017-04-19T07:48:55Z

@Nyholm this is what there is a prefer-lowest job too, to test the lower supported version.

and btw, 3.1 is EOLed. And yeah, in my message above, shift things for different versions in time.

Nyholm · 2017-04-19T07:58:01Z

this is what there is a prefer-lowest job too, to test the lower supported version.

Yeah, but prefer-lowest goes down to 2.8 in this case. Hm, but that is fine.

If we consider 2.8:

We cannot use features introduced in versions greater 2.8.0 => That is covered by the 2.8 test and prefer-lowest
We cannot use features removed in 3.0.0 => that is covered in any 3.x test.

So if we test on 2.8 and the latest stable sf version we know we support all versions in between.

fbourigault · 2017-04-19T08:05:51Z

If we not are testing with 3.1 (but we support it) we risk of using a feature/class that was introduced in 3.2.

I don't think so, if it works with the latest LTS (2.8), it should work for all 3.x except that we may trigger deprecation notices. Thanks to Symfony BC promise!

Testing LTSs, latest stable and dev should be enough (may require something special to handle 3.4 et 4.0 as dev).

stof · 2017-04-19T08:10:33Z

@fbourigault install the symfony/phpunit-bridge: ^3.2, and any deprecation in tests not marked as legacy will trigger a failure.
And you read it the wrong way. If it works in 3.0, it will work in 3.x thanks to BC promises (unless you relied on a Symfony bug). But working in 3.2 does not ensure it works in 3.0 (new 3.2 features are not working in 3.0).

fbourigault · 2017-04-19T08:12:00Z

Yes, but 2.8 without deprecations = 3.0 = 3.1 = 3.2 = 3.3 = 3.4, no?

stof · 2017-04-19T08:12:57Z

@fbourigault without deprecation, yes. But currently, the testsuite does not ensure it

fbourigault · 2017-04-19T08:27:30Z

@Nyholm what about testing against 2.8 without deprecations using symfony/phpunit-bridge?

stof · 2017-04-19T08:56:03Z

IMO, adding the bridge is a must-have (it is included in the example I linked previously btw). Your users expect to be able to run without deprecations, and so will complain if your bundle has some.

dbu · 2017-04-20T10:53:43Z

composer.json

@@ -37,7 +37,7 @@
        "php-http/guzzle6-adapter": "^1.1.1",
        "php-http/react-adapter": "^0.2.1",
        "php-http/buzz-adapter": "^0.3",
-        "symfony/symfony": "^2.8 || ^3.0",


according to travis-ci, we have either a hard dependency on TwigBundle or its at least optional and needed for testing. if its optional we should add it to the (not yet existing) suggest section along with an explanation what its needed for.

Twig is required for the profiler. I think we should depends on the WebProfilerBundle, but I think this will not be enough.

The WebProfilerBundle only depends on the twig-bridge. But it really require the twig service (https://github.com/symfony/web-profiler-bundle/blob/master/Resources/config/profiler.xml#L11).
We probably could suggest WebProfilerBundle and TwigBundle both for using the profiler.

yeah, WebProfilerBundle does not introduce a hard requirement on TwigBundle to make it usable in Silex. But using it in the fullstack context actually needs TwigBundle too (as it is the one configuring Twig). There is an issue about that on Symfony

Sounds fair to me to suggest both packages and adding them to require-dev. Looks better than depending on symfony/symfony.

yeah, much better

Thank you. I've required symfony/twig-bundle, symfony/twig-bridge and symfony/web-profiler-bundle

fbourigault · 2017-04-24T08:50:59Z

Discovery/ConfiguredClientsStrategy.php

@@ -77,8 +75,8 @@ public function onEvent(Event $e)
    public static function getSubscribedEvents()
    {
        return [
-            KernelEvents::REQUEST => ['onEvent', 1024],
-            ConsoleEvents::COMMAND => ['onEvent', 1024],
+            'kernel.request' => ['onEvent', 1024],


Why do you use strings over class constants?

Because I want to avoid dependency on Symfony/console and Symfony/http-kernel.

fbourigault

Nice :) An other question, you marked tests with @legacy annotation. I guess it's to avoid test failure because of deprecated functions. Can those deprecations be fixed in the code to be ready for Symfony 4.0?

fbourigault · 2017-04-24T14:21:17Z

.travis.yml

+          env: SYMFONY_VERSION=2.8.*
+
+before_install:
+    - if [ "$SYMFONY_VERSION" != "" ]; then composer require "symfony/symfony:${SYMFONY_VERSION}" --no-update; fi;


Maybe we should change this to require only useful bundles instead of the whole symfony?

too hard to maintain (has to be edited each time the composer.json changes).

the normal jobs already take care of using the normal deps with only relevant packages. These jobs are about testing the LTS support.

fbourigault · 2017-04-24T14:21:57Z

Discovery/ConfiguredClientsStrategy.php

@@ -77,8 +75,8 @@ public function onEvent(Event $e)
    public static function getSubscribedEvents()
    {
        return [
-            KernelEvents::REQUEST => ['onEvent', 1024],
-            ConsoleEvents::COMMAND => ['onEvent', 1024],
+            'kernel.request' => ['onEvent', 1024],


stof · 2017-04-24T14:32:39Z

@fbourigault the legacy tests are not about Symfony 4 deprecations (if some of them are, they should not be legacy tests) but about deprecations of the bundle itself (to keep covering the BC layers with tests)

Nyholm · 2017-04-24T15:35:00Z

Thank you for all the comments and feedback.

Reduced the number of travis builds.

2e00824

Nyholm requested a review from sagikazarmark April 14, 2017 08:51

sagikazarmark added the in progress label Apr 14, 2017

Nyholm added 3 commits April 14, 2017 10:53

Syntax fix

a5e90fd

Update .travis.yml

a530f76

Be clearer in Travis GUI

339b82c

xabbuh reviewed Apr 14, 2017

View reviewed changes

updated according to feedback

5176010

Add tests for sf 3.1

145e754

dbu approved these changes Apr 18, 2017

View reviewed changes

Re lved

ca2dd68

Nyholm modified the milestone: Version 1.5.0 Apr 19, 2017

Use the symfony phpunit bridge

32b6f43

dbu reviewed Apr 20, 2017

View reviewed changes

Nyholm added 3 commits April 24, 2017 10:24

Added missing deps

de5fd63

Use strings instead of constants

d9b5985

Added legacy annotation

d52663f

fbourigault reviewed Apr 24, 2017

View reviewed changes

Nyholm added 4 commits April 24, 2017 10:53

Merge branch 'master' into Nyholm-patch-1

55c9f81

merge fix

23a64a6

Fix

01fc201

Require finder

e076a32

fbourigault reviewed Apr 24, 2017

View reviewed changes

dbu approved these changes Apr 24, 2017

View reviewed changes

Nyholm merged commit 1b3bec5 into master Apr 24, 2017

Nyholm deleted the Nyholm-patch-1 branch April 24, 2017 15:35

sagikazarmark removed the in progress label Apr 24, 2017

fbourigault mentioned this pull request Jun 27, 2017

Use simple-phpunit to run tests on travis-ci #178

Closed

Reduced the number of travis builds. #149

Reduced the number of travis builds. #149

Conversation

Nyholm commented Apr 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nyholm commented Apr 18, 2017

dbu commented Apr 18, 2017

dbu left a comment

Choose a reason for hiding this comment

stof commented Apr 19, 2017

Nyholm commented Apr 19, 2017

stof commented Apr 19, 2017

Nyholm commented Apr 19, 2017

fbourigault commented Apr 19, 2017

stof commented Apr 19, 2017

fbourigault commented Apr 19, 2017

stof commented Apr 19, 2017

fbourigault commented Apr 19, 2017

stof commented Apr 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fbourigault left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stof commented Apr 24, 2017

Nyholm commented Apr 24, 2017