-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Adding Failed exception to manage maxfail behavior #2845
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good initial work 👍
due to the nature of the bugfix here i believe its better to target the features branch here
please also add a test that demonstrates the behaviour in the testsuite as well as a changelog entry
f690161
to
00d3abe
Compare
Added a changelog entry, changed the branch to merge to feature, and the current tests are changed to validate the new behavior. The existing test already test the behavior. |
I agree that's enough, unless @RonnyPfannschmidt has something else to test in mind. The two failing tests are unrelated (#2843). |
As I explained in the issue #2832, I'm not sure that the behavior is incorrect by itself, but have sense both options, interrupt return, and failed return. Anyway, I prefer to have failed return and leave "interrupt" to explicit user interruptions, like Ctrl-C.