Error handler suppresses PHPUnit exception conversion

sdustinh commented 7 years ago

This is a (multiple allowed):

[x] bug
[ ] enhancement
[ ] feature-discussion (RFC)

sdustinh@sdustinh:~/github/cakephp/app$ php --version
PHP 7.0.22-0ubuntu0.16.04.1 (cli) ( NTS )
Copyright (c) 1997-2017 The PHP Group
Zend Engine v3.0.0, Copyright (c) 1998-2017 Zend Technologies
    with Zend OPcache v7.0.22-0ubuntu0.16.04.1, Copyright (c) 1999-2017, by Zend Technologies
    with Xdebug v2.4.0, Copyright (c) 2002-2016, by Derick Rethans
sdustinh@sdustinh:~/github/cakephp/app$ mysql --version
mysql  Ver 14.14 Distrib 5.7.19, for Linux (x86_64) using  EditLine wrapper
sdustinh@sdustinh:~/github/cakephp/app$ composer show cakephp/*
cakephp/bake                1.3.7              Bake plugin for CakePHP 3.0
cakephp/cakephp             3.4.11             The CakePHP framework
cakephp/cakephp-codesniffer dev-master 6a7a46d CakePHP CodeSniffer Standards
cakephp/chronos             1.1.2              A simple API extension for DateTime.
cakephp/debug_kit           3.11.0             CakePHP Debug Kit
cakephp/elastic-search      0.3.5              An Elastic Search datasource and data mapper for CakePHP 3.0
cakephp/migrations          1.6.7              Database Migration plugin for CakePHP 3.0 based on Phinx
cakephp/plugin-installer    1.0.0              A composer installer for CakePHP 3.0+ plugins.

What you did

Ran unit tests with vendor/bin/phpunit. With the following action:

public function index()
{
    $something += '';
}

What happened

sdustinh@sdustinh:~/github/cakephp/app$ vendor/bin/phpunit --no-coverage
PHPUnit 6.2.3 by Sebastian Bergmann and contributors.

Runtime:       PHP 7.0.22-0ubuntu0.16.04.1 with Xdebug 2.4.0
Configuration: /home/sdustinh/github/cakephp/app/phpunit.xml

.................Notice Error: Undefined variable: something in [/home/sdustinh/github/cakephp/app/src/Controller/PagesController.php, line 23]

.Notice Error: Undefined variable: something in [/home/sdustinh/github/cakephp/app/src/Controller/PagesController.php, line 23]

Notice Error: Undefined variable: something in [/home/sdustinh/github/cakephp/app/src/Controller/PagesController.php, line 23]

...........................................^C

What you expected to happen

sdustinh@sdustinh:~/github/cakephp/app$ vendor/bin/phpunit --no-coverage
PHPUnit 6.2.3 by Sebastian Bergmann and contributors.

Runtime:       PHP 7.0.22-0ubuntu0.16.04.1 with Xdebug 2.4.0
Configuration: /home/sdustinh/github/cakephp/app/phpunit.xml

.................FF............................................  63 / 575 ( 10%)
......................^C

So, this has been a problem for a while and I finally had a minute to track down what was causing it. Our CI would report a build as successful despite errors being triggered. I read up on the PHPUnit documentation about converting errors to exceptions so that the tests would fail as expected but I couldn't figure out exactly why a notice would be displayed and the tests would still be flagged as passing. I sifted through my app.php which lead me to the BaseErrorHandler.

I was able to get the expected results by adding the following to my tests/bootstrap.php: restore_error_handler();

Do you guys think this is a safe thing to include in my test bootstrap? Would it make any of my tests unreliable from the framework perspective? If not, should this be something that is included in the skeleton test bootstrap?

markstory commented 7 years ago

Do you guys think this is a safe thing to include in my test bootstrap? Would it make any of my tests unreliable from the framework perspective?

Yes it is safe. It shouldn't make tests unreliable.

If not, should this be something that is included in the skeleton test bootstrap?

It could. We could also change the error handlers to not be registered inside of an environment that is within phpunit.

sdustinh commented 7 years ago

I'm not sure that this is a real consideration or not but I just wanted to note it for any development on this issue. Once this change was done, if there were errors that existed in previously passing builds they will now cause your build to fail. So, if this is incorporated into the error handlers themselves it should probably be noted in the migration notes.

cleptric commented 7 years ago

@sdustinh As this is the skeleton app and normally just the starting point of a new application ,the changes should be fine :)

sdustinh commented 7 years ago

@cleptric Affirm. @markstory just commented that this might be better suited in the error handler classes so I just wanted to note if that was the solution (which I'm :+1: for!) then there should be a note of the consequences on the migration guide.

markstory commented 7 years ago

Updating the app skeleton will certainly be easier than changing the error handlers.

ADmad commented 5 years ago

Closing as the issue has gone stale.

mamchenkov commented 5 years ago

I'm experiencing the very same problem, however the solution of adding restore_error_handler() to tests/bootstrap.php doesn't solve it. Should this issue be reopened or is creating a new one a better option?

ADmad commented 5 years ago

Reopening the issue won't do any good if it's going to stay unresolved for months as earlier. It would be better if someone come's up with a solution and opens a pull request instead.

mamchenkov commented 5 years ago

OK, sure. If I only manage to solve it, I'll send the PR :)

cakephp / app