[PhpUnitBridge] add a triggered errors assertion helper #18880

xabbuh · 2016-05-25T21:44:00Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets
License	MIT
Doc PR

In the past, we updated the test code several times that made sure that deprecation messages are triggered. I think we should move this code to a reusable class in the PhpUnitBridge to make it available everywhere and to be able to change a single method in case we need to update the logic.

xabbuh · 2016-05-26T06:44:19Z

@nicolas-grekas Any idea why the deps=low build is failing?

GuilhemN · 2016-05-28T20:16:02Z

@xabbuh the phpunit-bridge is not required in several composer files such the one of the framework bundle (https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/composer.json).

GuilhemN · 2016-05-28T20:17:50Z

Anyway the tests probably won't pass as long as this isn't merged.

OskarStark · 2016-05-31T14:22:47Z

src/Symfony/Bridge/PhpUnit/ErrorAssert.php

+ */
+final class ErrorAssert
+{
+    public static function assertDeprecationsAreTriggered($expectedMessages, callable $testCode)


should we use singular here? assertDeprecationIsTriggered or anything like setExpectedException ?

setExpectedDeprecation ?

Given that the method takes an array of expected messages, plural is fine.

setExpectedDeprecation is not fine for the current API. It would work if the method was register the expectation to check it later after running the test, but this is not what happens in the current API.

Given that the method takes an array of expected messages, plural is fine.

~~Ok fine, but do we have many cases where we assert many deprecations?~~
https://github.com/symfony/symfony/pull/18880/files/b8fcced6cd5b6f324e85c232fbb853f514fe8ea3#diff-ecb644500d31b54f3fa4574298dc6156R537

setExpectedDeprecation is not fine for the current API. It would work if the method was register the expectation to check it later after running the test, but this is not what happens in the current API.

👍

stof · 2016-05-31T14:27:27Z

@Ener-Getick we have a special phpunit launcher performing a local composer-based installation of PHPUnit (to remove unnecessary dependency which would hurt us, for instance symfony/yaml as we don't want to run tests on the version shipped in PHPUnit and we don't use the PHPUnit feature needing it). This special wrapper includes the bridge in the phpunit installation.

stof · 2016-05-31T14:29:44Z

@xabbuh looks like the PHPUnit setup installed the 3.1.0-beta1 version of the bridge, and so does not use the updated version of the bridge: https://travis-ci.org/symfony/symfony/jobs/133035973#L578

GuilhemN · 2016-06-01T17:50:57Z

@stof thanks for the explanation, I didn't know that.

GuilhemN · 2016-06-01T17:54:02Z

src/Symfony/Bridge/PhpUnit/ErrorAssert.php

+            $triggeredMessages[] = $message;
+        });
+
+        $testCode();


What about putting the entire function in a try/finally structure in case an exception is thrown ?

try { // ... } finally { restore_error_handler(); }

fabpot · 2016-06-14T06:07:08Z

ping @nicolas-grekas

nicolas-grekas · 2016-06-14T06:50:18Z

src/Symfony/Bridge/PhpUnit/ErrorAssert.php

+        $triggeredMessages = array();
+
+        set_error_handler(function ($type, $message) use ($expectedType, &$triggeredMessages) {
+            if ($expectedType !== $type) {


shouldn't this be a bitfield?

I don't think that that would be a good idea. I think if you really need to test for different kind of errors, you should probably rethink your code or write your own assertion code.

nicolas-grekas · 2016-06-14T12:11:30Z

👍

GuilhemN · 2016-06-14T12:21:10Z

src/Symfony/Bridge/PhpUnit/ErrorAssert.php

+                $triggeredMessages[] = $message;
+            });
+
+            $testCode();


Isn't it safer to use call_user_func here as well ?

Nope: no code has been written that can't be run. Having it here since the beginning will enforce this forever.

so we won't accept other callables than closure ?

Edit: if yes, then we should change the typehint to closure

@Ener-Getick But that would mean to drop support for other callables that are not anonymous functions.

@xabbuh indeed sorry, let's forget it, I thought the syntax $bar() only worked with closures...

I suggest doing $testCode($this) to ease writing tests in 5.3

@nicolas-grekas the class is static.

Edit: or did you mean adding it to the method signature ?

I forgot it is static :)

GuilhemN · 2016-06-14T13:04:57Z

src/Symfony/Bridge/PhpUnit/ErrorAssert.php

+     */
+    public static function assertDeprecationsAreTriggered($expectedMessages, $testCode)
+    {
+        if (!is_callable($testCode)) {


useless here as will be checked in the second function anyway

That's true but I'd like to keep it here too for two reasons: Do not confuse users by letting them deal with exceptions that have been thrown by methods they did not call if we cannot avoid that. Secondly, mitigate the risk of breaking anything if we refactor the method that we call here.

ok fine for me :)

nicolas-grekas · 2016-06-15T13:36:10Z

Status: reviewed

fabpot · 2016-06-15T16:21:49Z

Thank you @xabbuh.

…r (xabbuh) This PR was merged into the 3.2-dev branch. Discussion ---------- [PhpUnitBridge] add a triggered errors assertion helper | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | In the past, we updated the test code several times that made sure that deprecation messages are triggered. I think we should move this code to a reusable class in the PhpUnitBridge to make it available everywhere and to be able to change a single method in case we need to update the logic. Commits ------- b5c2095 add a triggered errors assertion helper

@xabbuh

…cation` (nicolas-grekas) This PR was merged into the 3.2-dev branch. Discussion ---------- [PhpUnitBridge] Replace ErrorAssert by `@expectedDeprecation` | Q | A | ------------- | --- | Branch? | 3.2 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #18880 | License | MIT | Doc PR | symfony/symfony-docs#7074 For 3.2, that's what the feat. freeze is for in this case. ping @xabbuh See https://github.com/symfony/symfony/pull/20255/files?w=1 Commits ------- c344203 [PhpUnitBridge] Drop ErrorAssert in favor of @expectedDeprecation

xabbuh added the PhpUnitBridge label May 25, 2016

carsonbot added Status: Needs Review Feature labels May 25, 2016

xabbuh force-pushed the error-assert branch 4 times, most recently from 76cfc10 to b8fcced Compare May 26, 2016 06:36

OskarStark reviewed May 31, 2016
View reviewed changes

GuilhemN reviewed Jun 1, 2016
View reviewed changes

nicolas-grekas reviewed Jun 14, 2016
View reviewed changes

xabbuh force-pushed the error-assert branch 5 times, most recently from 6458c99 to ec0464c Compare June 14, 2016 12:09

GuilhemN reviewed Jun 14, 2016
View reviewed changes

add a triggered errors assertion helper

b5c2095

xabbuh force-pushed the error-assert branch from ec0464c to b5c2095 Compare June 14, 2016 12:58

GuilhemN reviewed Jun 14, 2016
View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Jun 15, 2016

fabpot merged commit b5c2095 into symfony:master Jun 15, 2016

xabbuh deleted the error-assert branch June 15, 2016 19:52

nicolas-grekas mentioned this pull request Jun 23, 2016

[DependencyInjection] deprecate access to private shared services. #19146

Merged

nicolas-grekas mentioned this pull request Oct 20, 2016

[PhpUnitBridge] Replace ErrorAssert by @expectedDeprecation #20255

Merged

fabpot mentioned this pull request Oct 27, 2016

Release v3.2.0-BETA1 #20317

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PhpUnitBridge] add a triggered errors assertion helper #18880

[PhpUnitBridge] add a triggered errors assertion helper #18880

xabbuh commented May 25, 2016

xabbuh commented May 26, 2016

GuilhemN commented May 28, 2016

GuilhemN commented May 28, 2016

OskarStark May 31, 2016

stof May 31, 2016

OskarStark May 31, 2016 •

edited

Loading

stof commented May 31, 2016

stof commented May 31, 2016

GuilhemN commented Jun 1, 2016

GuilhemN Jun 1, 2016

xabbuh Jun 14, 2016

fabpot commented Jun 14, 2016

nicolas-grekas Jun 14, 2016

xabbuh Jun 14, 2016

nicolas-grekas commented Jun 14, 2016

GuilhemN Jun 14, 2016

nicolas-grekas Jun 14, 2016

GuilhemN Jun 14, 2016 •

edited

Loading

xabbuh Jun 14, 2016

GuilhemN Jun 14, 2016 •

edited

Loading

nicolas-grekas Jun 14, 2016

GuilhemN Jun 14, 2016 •

edited

Loading

nicolas-grekas Jun 14, 2016

GuilhemN Jun 14, 2016

xabbuh Jun 14, 2016

GuilhemN Jun 14, 2016

nicolas-grekas commented Jun 15, 2016

fabpot commented Jun 15, 2016

[PhpUnitBridge] add a triggered errors assertion helper #18880

[PhpUnitBridge] add a triggered errors assertion helper #18880

Conversation

xabbuh commented May 25, 2016

xabbuh commented May 26, 2016

GuilhemN commented May 28, 2016

GuilhemN commented May 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OskarStark May 31, 2016 • edited Loading

Choose a reason for hiding this comment

stof commented May 31, 2016

stof commented May 31, 2016

GuilhemN commented Jun 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Jun 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Jun 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuilhemN Jun 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuilhemN Jun 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuilhemN Jun 14, 2016 • edited Loading

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

nicolas-grekas commented Jun 15, 2016

fabpot commented Jun 15, 2016

OskarStark May 31, 2016 •

edited

Loading

GuilhemN Jun 14, 2016 •

edited

Loading

GuilhemN Jun 14, 2016 •

edited

Loading

GuilhemN Jun 14, 2016 •

edited

Loading