added phpunit 6 #47

kokspflanze · 2017-02-24T00:21:27Z

updated some tests with ::class
changed composer.json with phpunit to min 5.7.14 or 6.0.7

updated some tests with ::class changed composer.json with phpunit to min 5.7.14 or 6.0.7

froschdesign · 2017-02-24T11:54:28Z

test/Test/EventListenerIntrospectionTraitTest.php

@@ -193,6 +194,13 @@ public function testAssertListenerAtPriorityFailsWhenListenerIsNotFound()
                    $arguments['event'],
                    $arguments['priority']
                ), $e->getMessage(), sprintf('Assertion failure message was unexpected: %s', $e->getMessage()));
+            // for PHPUnit 5.7


Please no dependencies to a specific version.

thats the fallback for PHPUnit5, thats not just phpunit5.7

that exception class doesnt exist in phpunit 6, so there must be a fallback, other way would be a class_alias or a check if the class exists.

Please have a look how @weierophinney did it in another repo:
zendframework/zend-component-installer@654983e
I'd suggest the same way here.

ohh i missed that, is that workflow is expected, than i will change that sure =)

michalbundyra · 2017-02-24T12:10:14Z

src/Test/EventListenerIntrospectionTrait.php

@@ -9,7 +9,7 @@

 namespace Zend\EventManager\Test;

-use PHPUnit_Framework_Assert as Assert;
+use PHPUnit\Framework\Assert as Assert;


Alias Assert here is redundant.

michalbundyra · 2017-02-24T12:14:01Z

@kokspflanze We need also update travis configuration, to install PHPUnit 5.7 on PHP5.6, please see changes for example here:
https://github.com/zendframework/zend-component-installer/blob/master/.travis.yml#L16
and install section there.

michalbundyra · 2017-02-24T12:20:54Z

composer.json

@@ -32,7 +32,7 @@
        "php": "^5.6 || ^7.0"
    },
    "require-dev": {
-        "phpunit/PHPUnit": "^5.6",
+        "phpunit/PHPUnit": "^5.7.14 || ^6.0.7",


I would suggest also swap order here, please see:
zendframework/zend-expressive-skeleton@39ef689

Alias Assert is redundant update travis configuration swap order of phpunit version

updated doc-block

kokspflanze · 2017-02-24T18:04:19Z

done, i just added in the autoload file a class_exist check, to remove the warning

michalbundyra

LGTM 👍

marc-mabe · 2017-03-15T20:07:21Z

src/Test/EventListenerIntrospectionTrait.php

@@ -9,7 +9,7 @@

 namespace Zend\EventManager\Test;

-use PHPUnit_Framework_Assert as Assert;
+use PHPUnit\Framework\Assert;


Why not using $this->assertTrue() instead of Assert::assertTrue()

why?

in that case we are in a trait class and the class that this trait import can not have that method.

Makes sense

Makes sense

Sure? If we look at the comments of the class:

Trait providing utility methods and assertions for use in PHPUnit test cases.

😉

before the Assert class it was with the TestCase class

be75ec1

from my site i see the Assets class as better solution, with $this it could be possible that it break smth in other tests if someone not use the TestCase as parent-class. also we have a autocomplete and no warning that the method not exist.

@froschdesign In a trait it's only possible to define an abstract method to define that a specific method needs to be available but that does not work with __call() so it gets marked red in good IDEs. Using the direct assert seems to be more clear and the trait itself can be tested much better.

I fine with it. No extra discussion needed. (Look at the smiley!)

froschdesign · 2017-03-16T08:43:33Z

@kokspflanze

The topic of this PR is "added phpunit 6", but you provide an all-in-one-commit:

namespace imports
class names with ::class
expectException and expectExceptionMessage
fallback

This may be necessary, but not in one commit! It's fine to add all this changes to one PR, but please create separate commits for each problem. We work with a version control system and these all-in-one-commits breaks this system. Try a cherry-pick or try to read the history.

Each individual change should be one commit.

(Added benefit: a review becomes easier and faster!)

kokspflanze · 2017-03-16T08:57:05Z

@froschdesign no problem to split it in more commits, but that week is no much space, hope its okay if i do that next week.

didnt know that this should be in more commits, it was for me one little task.

froschdesign · 2017-03-16T09:21:46Z

@kokspflanze

…but that week is no much space, hope its okay if i do that next week.

I can live with this PR.

@marc-mabe What are you saying?

didnt know that this should be in more commits, it was for me one little task.

Which mixes different problems!

Each individual change should be one commit.

That's not a specific feature or requirement of Zend Framework, GitHub or me. That's the principle of a version control system.

added phpunit 6

Close #47

Forward port #47

weierophinney · 2017-07-11T19:08:30Z

Thanks, @kokspflanze

kokspflanze added 2 commits February 24, 2017 01:19

added phpunit 6

7da4a9d

updated some tests with ::class changed composer.json with phpunit to min 5.7.14 or 6.0.7

remove ignore-platform-reqs in travis

0f138e8

froschdesign reviewed Feb 24, 2017

View reviewed changes

michalbundyra reviewed Feb 24, 2017

View reviewed changes

kokspflanze added 3 commits February 24, 2017 18:49

updated phpunit 5.7 fallback

863e308

Alias Assert is redundant update travis configuration swap order of phpunit version

added check for phpunit 5.7 fallback

d8d874a

updated doc-block

cs part fixed

ba3af00

michalbundyra approved these changes Feb 27, 2017

View reviewed changes

kokspflanze mentioned this pull request Feb 27, 2017

phpunit 6 update zendframework/zend-mvc#222

Merged

marc-mabe reviewed Mar 15, 2017

View reviewed changes

marc-mabe mentioned this pull request Mar 16, 2017

PHPUnit6 does not have a class PHPUnit_Framework_Assert #50

Closed

froschdesign added the enhancement label Mar 16, 2017

weierophinney merged commit ba3af00 into zendframework:master Jul 11, 2017

weierophinney added a commit that referenced this pull request Jul 11, 2017

Merge pull request #47 from kokspflanze/master

75175ed

added phpunit 6

weierophinney added a commit that referenced this pull request Jul 11, 2017

Merge branch 'hotfix/47'

1e373e0

Close #47

weierophinney added a commit that referenced this pull request Jul 11, 2017

Merge branch 'hotfix/47' into develop

8ac2f85

Forward port #47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added phpunit 6 #47

added phpunit 6 #47

kokspflanze commented Feb 24, 2017

froschdesign Feb 24, 2017

kokspflanze Feb 24, 2017

michalbundyra Feb 24, 2017

kokspflanze Feb 24, 2017

michalbundyra Feb 24, 2017

michalbundyra commented Feb 24, 2017

michalbundyra Feb 24, 2017

kokspflanze commented Feb 24, 2017

michalbundyra left a comment

marc-mabe Mar 15, 2017

kokspflanze Mar 15, 2017

marc-mabe Mar 16, 2017

froschdesign Mar 16, 2017

kokspflanze Mar 16, 2017

marc-mabe Mar 16, 2017

froschdesign Mar 16, 2017

froschdesign commented Mar 16, 2017

kokspflanze commented Mar 16, 2017

froschdesign commented Mar 16, 2017

weierophinney commented Jul 11, 2017

added phpunit 6 #47

added phpunit 6 #47

Conversation

kokspflanze commented Feb 24, 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

michalbundyra commented Feb 24, 2017

Choose a reason for hiding this comment

kokspflanze commented Feb 24, 2017

michalbundyra 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

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

froschdesign commented Mar 16, 2017

kokspflanze commented Mar 16, 2017

froschdesign commented Mar 16, 2017

weierophinney commented Jul 11, 2017