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

Do not suppress errors #175

Merged
merged 1 commit into from Mar 7, 2016
Merged

Do not suppress errors #175

merged 1 commit into from Mar 7, 2016

Conversation

ezzatron
Copy link
Contributor

@ezzatron ezzatron commented Mar 2, 2016

I use Asplode in my peridot.php to set up an error handler that converts errors into error exceptions. I only recently realised that this was not working, and that my tests were passing silently despite errors.

I see two problems here:

  1. Peridot is currently overwriting and ignoring any existing error handler.
  2. Peridot is currently silencing errors, because the error handler doesn't actually do anything except to raise a Peridot "error" event. That seems like a dangerous choice. Shouldn't a test fail if an error occurs during its execution, and shouldn't that happen without having to write a custom "error" event handler?

This PR addresses these two problems in the following ways:

  1. If there is an existing error handler, Peridot will now "forward on" the error to the existing handler. This includes using the existing error handler's return value, which is important (see below).
  2. If there is no existing error handler, Peridot's error handler will now return false to indicate that the error should propagate to PHP's inbuilt error handler. This means that the error will at least be displayed to the user, instead of being silenced completely.

This does not address the issue of whether tests should fail due to errors, but that's probably another discussion / PR.

@brianium
Copy link
Member

brianium commented Mar 7, 2016

@ezzatron Good game again. Definitely an improvement. Thanks for you work on this!

@brianium
Copy link
Member

brianium commented Mar 7, 2016

I will tag a minor release with this enhancement shortly.

brianium added a commit that referenced this pull request Mar 7, 2016
@brianium brianium merged commit 1945fe9 into peridot-php:master Mar 7, 2016
@ezzatron
Copy link
Contributor Author

ezzatron commented Mar 8, 2016

Thanks :)

@brianium
Copy link
Member

brianium commented Mar 8, 2016

@ezzatron do you have any opinion on suppressing the output generated by the $behavesLikeErrorEmitter behavior?

The spec in question:

Runner->run() when configured to bail on failure should emit an error event with error information

Is printing out the notice when the suite runs. I understand that this is generally the behavior we want. Was wondering if you had thoughts on keeping that out of the Peridot suite's output.

@ezzatron
Copy link
Contributor Author

ezzatron commented Mar 9, 2016

I agree, it's not great having that output displayed. Maybe we can solve another problem at the same time, but I wanted to discuss it with you.

Would you agree that when an error is raised in a test, Peridot should mark that test as failed?

It seems logical to me; and if that's the case, Peridot would actually be "handling" the error, and the need to return false from the error handler (which is causing the output in question) goes away.

I'll submit a PR shortly to clarify what I mean.

@brianium
Copy link
Member

brianium commented Mar 9, 2016

Awesome. My time in here lately has been limited, so I am definitely grateful for your work on Peridot lately.

I am wondering if some of this error related work might fix #158 as well

@ezzatron
Copy link
Contributor Author

ezzatron commented Mar 9, 2016

Thanks man, it's nice to actually get some PRs accepted without drama for once!

See #177 for the PR, and yes, I think it will solve #158 as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants