Allow testing of multiple library versions #203

labbati · 2018-12-21T10:10:15Z

Description

When we integrate with different versions of the same library we need a structured way to define the various test scenarios for the several versions. @inverse mentioned this excellent library from @greg-1-anderson that seems to be the perfect tool for us: https://github.com/g1a/composer-test-scenarios
This PR aims at integrating this tool into our code base!

Readiness checklist

Changelog entry added, if necessary
Tests added for this feature/bug

…ace-php into tests/move-integrations-tests

…llow-testing-multiple-libray-versions

…ific scenarios

SammyK

Great work at tackling this tough problem, @labbati! :) I have one little issue with the scenario command changing a file in the working directory.

Other than that, everything looks great! My biggest question is around running the entire test suite. Right now this PR changes the behavior of $ composer test which now runs only a subset of the tests:

Time: 3.66 seconds, Memory: 10.00MB

OK (155 tests, 221 assertions)

Compared to the entire test suite:

Time: 11.89 seconds, Memory: 22.00MB

OK (345 tests, 1583 assertions)

I think it's really important to be able to run all the tests before submitting a PR, so is there a command we can run that will just run the entire test suite with this change? :)

SammyK · 2018-12-21T13:10:50Z

CONTRIBUTING.md

+As an example, in order to run Guzzle tests with Guzzle v5 library, run:
+
+    # Only the first time, to create all the different scenarios
+    $ composer scenario:update


After running this command, the LICENSE file gets updated. Here's the diff:

diff --git a/LICENSE b/LICENSE index be5bf07..1e187aa 100644 --- a/LICENSE +++ b/LICENSE @@ -27,3 +27,16 @@ SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + +DEPENDENCY LICENSES: + +Name Version License +ircmaxell/password-compat v1.0.4 MIT +opentracing/opentracing 1.0.0-beta5 MIT +paragonie/random_compat v9.99.99 MIT +psr/http-message 1.0.1 MIT +psr/log 1.1.0 MIT +symfony/polyfill-php55 v1.10.0 MIT +symfony/polyfill-php56 v1.10.0 MIT +symfony/polyfill-php70 v1.10.0 MIT +symfony/polyfill-util v1.10.0 MIT

Thanks for noticing it, let me look at it!

…llow-testing-multiple-libray-versions

labbati · 2018-12-31T10:46:04Z

@SammyK

My biggest question is around running the entire test suite. Right now this PR changes the behavior of $ composer test which now runs only a subset of the tests:

The biggest problem here is that while circleci is PHP version aware, composer is not. Some tests, e.g. Mongo, can be executed only on 5.6, while others can only be executed on 7.x, for more recent versions of the libraries. From a "stability" point of view this is not an issue (because of course CI has the last word) but still you are absolutely right about the fact that we should have a way to run the entire suite (that makes sense for a specific version of PHP) locally. Let me see how things are when we move all the definitions to composer. If we decide that it is not clear enough we can always roll back! 😄

…composer

labbati · 2018-12-31T11:17:43Z

@SammyK I tried to move all the various env specific definitions from circleci's config to composer. I think it makes a lot of sense. What do you think about it?

greg-1-anderson · 2018-12-31T17:33:08Z

.gitignore

@@ -1,5 +1,6 @@
 **/vendor/
 composer.lock
+.scenarios.lock/


If you want to use composer-test-scenarios to do highest/lowest testing (I recommend this, even for libraries), then you should commit composer.lock and .scenarios.lock. If you only want to do highest testing, then what you're doing here is fine.

We actually want to test only specific versions that we support. But highest/lowest testing would be a dream for us at a certain point. Thanks for pointing this out.

greg-1-anderson · 2018-12-31T17:35:42Z

composer.json

+            }
+        },
+        "scenario-options": {
+            "dependency-licenses": false


Maybe this should be the default. I like keeping my dependency licenses listed and up-to-date in the LICENSE file, but this feature is orthogonal to the primary purpose of composer-test-scenarios.

Maybe this should be the default.

It probably would make sense, as you suggested this is orthogonal to the purpose of this library. Very nice feature though, and the fact that it is on/off configurable denoted a very forward-thinking approach!

inverse · 2019-01-02T13:43:54Z

composer.json

+            "@composer test -- --testsuite=integration"
+        ],
+        "test-integrations-56": [
+            "@composer scenario:update",


Are these lists of commands going to be the same across each PHP version?

Right now only mongo tests are different, I expect that as we add more and more versions (soon) they will diverge quite a bit.

SammyK · 2019-01-03T13:53:15Z

Great work @labbati! :) I really like how I can run all the tests with one command.

One thing that I think we should consider going forward is how long it takes to run all the tests now. Considering that the tests suite takes several minutes to run and that time will certainly increase with every integration we add, this will eventually become a big issue for development going forward. I'm not sure of an obvious solution right now but I think we should definitely be thinking of some ways to speed this up with caching or something. :)

labbati · 2019-01-03T14:43:17Z

Ops sorry @SammyK I had missed this comment before merging.
I absolutely agree with you. I don't have a precise idea at the moment. If I remember correctly we used https://github.com/paratestphp/paratest with my previous team to run tests in parallel, but be ready to some refactoring because, as an example, tests that uses the same mysql server may be overlapping and cause each other failure, and so on.
But we probably will need to find a solution. It is also true that as we move forward, work will probably shift (in quantity) from core to specific integrations, and for them we have specific command to run test individually. But let's start thinking about it.

chuck · 2019-01-03T15:30:14Z

Setting up individual pipelines for integrations in circleci config is also an option - doesn't the python integration do something like that?

labbati · 2019-01-03T15:37:07Z

@chuck this is absolutely an option. Actually, I thought that the comment from @SammyK was related the local env. In the end I never felt that having "builds" in CI lasting for a few minutes were a big problem. But in that case pipelines are surely a good approach! 👍

labbati added 16 commits December 19, 2018 18:19

Move frameworks test folder to tests root

0a85caf

[tests] move frameworks test to their own directory

84a549c

[tests] move integrations tests folder

767869b

[test] Fix namespaces in integrations tests

57927f9

[tests] Exclude frameworks test apps folder from phpcs analysis

6fe6d7a

[test] avoid linting frameworks test app

ec21641

Merge branch 'tests/move-framework-tests' of github.com:DataDog/dd-tr…

92e5ff4

…ace-php into tests/move-integrations-tests

[test] Bring back mongo tests to its home in Integrations :D

24bd242

[test] Execute mongo integration tests only php <7

527de57

[test] Basic implementation of test scenarios

e08dbd8

Merge branch 'master' of github.com:DataDog/dd-trace-php into tests/a…

54b5322

…llow-testing-multiple-libray-versions

[test] Remove integrations testsuite. They have to be run within spec…

368885b

…ific scenarios

[core] Restore LICENSE file

1cf48fc

[test] Update contributor guideline's to run itnegrations tests

e9fbc3b

[tests] Move guzzle tests to correct folder

f18e84f

[core] update changelog

09bad58

labbati requested a review from SammyK December 21, 2018 10:11

SammyK suggested changes Dec 21, 2018

View reviewed changes

labbati added 2 commits December 31, 2018 10:44

Merge branch 'master' of github.com:DataDog/dd-trace-php into tests/a…

96f27b4

…llow-testing-multiple-libray-versions

[test] Avoid producing a LICENSE file when updating test scenarios

abb26ec

[tests] Move integrations tests definition from circleci's config to …

6160d48

…composer

labbati mentioned this pull request Dec 31, 2018

Guzzle 6 Integration bug #185

Closed

greg-1-anderson reviewed Dec 31, 2018

View reviewed changes

inverse reviewed Jan 2, 2019

View reviewed changes

SammyK approved these changes Jan 3, 2019

View reviewed changes

labbati added the dev/testing label Jan 3, 2019

labbati added this to the 0.9.0 milestone Jan 3, 2019

labbati merged commit 604d134 into master Jan 3, 2019

labbati deleted the tests/allow-testing-multiple-libray-versions branch January 3, 2019 13:47

labbati modified the milestones: 0.10.0, 0.9.0 Jan 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow testing of multiple library versions #203

Allow testing of multiple library versions #203

labbati commented Dec 21, 2018 •

edited

Loading

SammyK left a comment

SammyK Dec 21, 2018

labbati Dec 31, 2018

labbati commented Dec 31, 2018 •

edited

Loading

labbati commented Dec 31, 2018

greg-1-anderson Dec 31, 2018

labbati Jan 2, 2019

greg-1-anderson Dec 31, 2018

labbati Jan 2, 2019

inverse Jan 2, 2019 •

edited

Loading

labbati Jan 2, 2019

inverse Jan 2, 2019

SammyK commented Jan 3, 2019

labbati commented Jan 3, 2019

chuck commented Jan 3, 2019

labbati commented Jan 3, 2019

Allow testing of multiple library versions #203

Allow testing of multiple library versions #203

Conversation

labbati commented Dec 21, 2018 • edited Loading

Description

Readiness checklist

SammyK left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

labbati commented Dec 31, 2018 • edited Loading

labbati commented Dec 31, 2018

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

inverse Jan 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SammyK commented Jan 3, 2019

labbati commented Jan 3, 2019

chuck commented Jan 3, 2019

labbati commented Jan 3, 2019

labbati commented Dec 21, 2018 •

edited

Loading

labbati commented Dec 31, 2018 •

edited

Loading

inverse Jan 2, 2019 •

edited

Loading