Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

throw exceptions for notices #178

Closed
akleiber opened this issue Feb 22, 2013 · 11 comments

Comments

Projects
None yet
5 participants
@akleiber
Copy link

commented Feb 22, 2013

From phpunit we now:

By default, PHPUnit converts PHP errors, warnings, and notices that are triggered during the execution of a test to an exception.

But with codeception notices are not converted to exceptions as we can see here:

https://github.com/Codeception/Codeception/blob/master/src/Codeception/Subscriber/ErrorHandler.php#L22

https://github.com/Codeception/Codeception/blob/master/src/Codeception/Subscriber/ErrorHandler.php#L31

http://www.php.net/manual/en/errorfunc.constants.php

Notices == 8 and the error handler here only throws exceptions for errors higher than notices.

To be sure to have good code you should change the default error handler to throw exceptions for notices as well.

@artyfarty

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2013

I pull-requested the mentioned code to actually get rid of non-fatal php errors because I really don't care about PHP strict warnings for example.

I think that "Fatal error level" should be a setting actually.

@omares

This comment has been minimized.

Copy link

commented Feb 25, 2013

What the hell?

@artyfarty So you dont like proper written code? Sorry but that statement is stupid. Everyone should care about php warnings and errors - every minor error can break your code as much as exceptions. Please revert that pull request, or make the desired error level asap configurable.

@artyfarty

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2013

@omares yet not everybody care. For example, some library developers. There are many reasons why app can overflow with ignored warnings, when the source of those warnings is out of developers control. The reason can be simply legacy code.

When I'm integrating codeception into some old app i'm certainly not interesting in it saying "you app sucks, because 9001 warnings, we are not talking".

What's worse is that new warnings are introduced with new versions of PHP. For example, recently E_STRICT warnings, which were ignored previously, suddenly became 'on' by default. I see no problem in child class changing parent method's signature, PHPStorm sees no problem with it, interpreter sees no problem with it, yet codeception has problems with it.

Why something that's defined as non-fatal in php should be fatal for test execution? I see it as a poorly-written error handler, not as a feature.

Codeception is a tool. Testing tool shouldn't enforce somebody's ideology about coding standarts on developer.

@DavertMik

This comment has been minimized.

Copy link
Member

commented Feb 25, 2013

I merged the Pull request but I didn't get a chance to test it. I'm doing complete reviews before releases, because I can't judge the code before seeing it in action.

The good part of this commit was: fixing issues with Yii module, the bad part of it: we shouldn't hide potential problems.

I must admit I agree with both sides here.
The solution can be: a config option, or guide to set an error level in bootstrap.

And for default behavior... yes, we should promote best practices, i.e. convert notices to exceptions.

@akleiber

This comment has been minimized.

Copy link
Author

commented Feb 26, 2013

The solution can be: a config option, or guide to set an error level in bootstrap

👍

@akleiber akleiber closed this Feb 26, 2013

@akleiber akleiber reopened this Feb 26, 2013

@DavertMik

This comment has been minimized.

Copy link
Member

commented Feb 28, 2013

I had a talk with @artyfarty and we got to conclusion that we should reveal all errors except that one that are blocked with @. That was done in 1.5.5.

So warnings, errors, notices, are exceptions now.

@DavertMik DavertMik closed this Feb 28, 2013

@DavertMik

This comment has been minimized.

Copy link
Member

commented Feb 28, 2013

sorry, guys. It was half done for 1.5.5.
@artyfarty pushed a pull #194 with
So we will get configurable error level and E_ALL => Exception by default.

@DavertMik DavertMik reopened this Feb 28, 2013

@akleiber

This comment has been minimized.

Copy link
Author

commented Feb 28, 2013

Sounds good 👍 Thank you

@akleiber

This comment has been minimized.

Copy link
Author

commented Mar 12, 2013

closed with e0fb21b

@akleiber akleiber closed this Mar 12, 2013

@delboy1978uk

This comment has been minimized.

Copy link

commented Feb 5, 2018

Is it possible to configure this on and off? for instance, we have a passing test suite so long as they aren't thrown. We are working through our error_log removing these notices, but previously using phpunit we just had to add a property in the xml, so we could deploy without problems, and re-throw them in dev time, so we could slowly work to get rid of them all.

@delboy1978uk

This comment has been minimized.

Copy link

commented Feb 5, 2018

ah cool, just found this 76f1bac

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.