Error success interface
John J Lee
I'm going to use "core" to mean non-plugin code.
I think we don't have precise terminology for the various different uses
of "error" and "failure", so I'll make some up:
1. "error" == (almost) any exception raised by test code (this matches
what unittest means by "error"). ("almost" only because KeyboardInterrupt
is not caught by unittest, so is not an error in this sense)
2. "failure" == "failed test assertion" (this matches what unittest means
by "failure")
3. "nose Failure" == "nose core or plugin bug handled as a
nose.failure.Failure instance" (until nose 0.10.2, class Failure lived in
nose.case). Note that these "Failure"s are a kind of unittest *error*.
4. "unsuccessful error" == "error that causes the test run to be
unsuccessful". Note that errorclass errors whose ErrorClass instance has
a true .isfailure are NOT unsuccessful errors. Extending the terminology
a bit, they are "successful errors".
I think that we should add an interface that lets plugins determine
whether errors are successful errors.
In passing -- obviously, the names nose.failure.Failure and
nose.plugins.errorclass.ErrorClass.isfailure are unfortunate. How about
moving towards renaming the former Bug, and the latter .issuccessful (with
its sense reversed, of course)? The former could be introduced as an
alias in 0.11, and the old name deprecated and removed later. The latter
could be done in a 1.0, maybe.
Back to the issue at hand. The underlying problem is that plugin debug
does not know about errorclass errors. It doesn't necessarily need to
know specifically about errorclass errors, just about whether they are
unsuccessful errors.
As a consequence, there's an immediate problem that errorclass has to
return non-None to stop debug from dropping into the debugger on
non-failure errorclass errors like skips. But that can stop other plugins
working. If debug saw errorclass errors and could determine whether the
error was successful or not, it could act appropriately. Obviously, it
should depend on a well-defined interface, not on the specific errorclass
implementation.
In turn, that "return True" in the .addError() in errorclass means that my
third party plugin project rudolf can't print errorclass errors, because
.addError is not called on my plugin. The .score attribute doesn't help,
because .score has to be "low" in order for my plugin's output to appear
after other plugins. My plugin does that because it generates output to
replace core output, and core output normally appears after plugin output.
Also, of course, ANY use of .score is a hack, including these two (so
don't even think about suggesting splitting my plugin into two plugins :-)
Finally, it's a problem for rudolf that, even if this problem were fixed,
it can't print erroclass errors without poking into result.errorClasses
(and ISTR somebody else on the users list needed to do something similar).
This could be solved by exposing that information somehow (which requires
the specifics about errorclass labels, isfail in addition to the list of
errors). Alternatively, it could be solved with the output "markup"
proposal we talked about earlier. The latter would also solve this issue,
which nobody has proposed an alternative fix for:
http://groups.google.co.uk/group/nose-users/browse_frm/thread/4da310266260ab28/ce76f00b153d19b9
Again in passing: An additional maintenance problem -- core code should
not know about any plugins, but it does know about plugin errorclass
(through nose.core.TextTestRunner).
An obvious alternative to providing an interface to check whether errors
are successful errors is to move the errorclass feature (back?) into core,
and adding a new plugin method .addErrorClassError() so that debug can
just explicitly check whether it's an unsuccessful errorclass error. Or
perhaps just add a new method .addSuccessfulError().
Options:
1. Move errorclass into core and add a new plugin method
.addErrorClassError
2. Add new plugin method .addSuccessfulError(), optionally moving
errorclass into core.
3. Implement the "markup" proposal.
4. Implement a wrapper around exceptions that supports an interface for
testing for "successful error"-ness. Something like nose.case.Test does
for test cases, maybe. Sounds like a big change.
5. Any more?
Whatever the solution, I'd like to fix this somehow in 0.11, because it
breaks my plugin :-) Same goes for the issue linked to above.
John
jason pellerin
>
> I have to define some terms first.
>
> I'm going to use "core" to mean non-plugin code.
"error" and "failure", agreed.
> 3. "nose Failure" == "nose core or plugin bug handled as a
> nose.failure.Failure instance" (until nose 0.10.2, class Failure lived in
> nose.case). Note that these "Failure"s are a kind of unittest *error*.
Failures can be raised in a 3rd case: failure to load tests, for
instance a failed import of a test module.
> 4. "unsuccessful error" == "error that causes the test run to be
> unsuccessful". Note that errorclass errors whose ErrorClass instance has
> a true .isfailure are NOT unsuccessful errors. Extending the terminology
> a bit, they are "successful errors".
I'm confused by this. The intent of setting .isfailure to True for an
ErrorClass is that exceptions handled by that class *should* cause the
test run to fail. If that's not the behavior you're seeing, something
is definitely not working correctly.
Also, I find the term "unsuccessful error" hard to parse. I'd rather
that we let those errors fall under the generic "errors" term, since
their behavior is no different from any other error. The fourth
category I think should instead be "ignored errors" (or something
along those lines) -- for exceptions that, when handled by an
ErrorClass, do not cause the test run to fail.
> I think that we should add an interface that lets plugins determine
> whether errors are successful errors.
Yes. The lack of such an interface is a major problem in error
handling, as you've made clear.
> In passing -- obviously, the names nose.failure.Failure and
> nose.plugins.errorclass.ErrorClass.isfailure are unfortunate.
Definitely. I don't like 'Bug' though. I'd actually like to split out
the 3 Failures into a hierarchy something like:
UnexpectedException
LoaderException
PluginException
With UnexpectedException replacing Failure, and used in the more
general cases, and LoaderException and PluginException using
specifically for exceptions raised during test loading or by plugins.
> Back to the issue at hand. The underlying problem is that plugin debug
> does not know about errorclass errors. .... As a consequence, there's an
> immediate problem that errorclass has to return non-None to stop debug
> from dropping into the debugger on non-failure errorclass errors like skips.
> But that can stop other plugins working.
> Options:
>
> 3. Implement the "markup" proposal.
I want to do this anyway, at some point, once we've been able to
address Michal's concerns (which I think we can do by making tokens
stringify to the same strings that plugins see now).
> 5. Any more?
Simple, hacky: we could make addError chainable to allow multiple
plugins to be called, and specify that plugins can return the original
exception wrapped in an "Ignore" or set an "ignore" or "handled "
attribute on the exception to tell core and other plugins that the
exception has been handled.
Less hacky, similar to 2 of your ideas: Add an errorHandled(test,
err) plugin method. For instance, config.plugins.errorHandled(test,
Skip()) would return True when the SkipTest plugin was active. Then
ErrorClass plugins would not need to return True from addError, but
core and other error-aware plugins like debug would need to be updated
to call errorHandled() before taking action on an error.
> Whatever the solution, I'd like to fix this somehow in 0.11, because it
> breaks my plugin :-) Same goes for the issue linked to above.
I'd like to fix it sooner, if we can find and agree on a simple and
backwards-compatible fix. If not, 0.11 for sure.
JP
John J Lee
> On Sat, Feb 9, 2008 at 11:43 AM, John J Lee <j...@pobox.com> wrote:
[...]
>> 4. "unsuccessful error" == "error that causes the test run to be
>> unsuccessful". Note that errorclass errors whose ErrorClass instance has
>> a true .isfailure are NOT unsuccessful errors. Extending the terminology
>> a bit, they are "successful errors".
>
> I'm confused by this.
[...]
Hah. Sorry, I got it backwards there, obviously.
> Also, I find the term "unsuccessful error" hard to parse. I'd rather
> that we let those errors fall under the generic "errors" term, since
> their behavior is no different from any other error. The fourth
> category I think should instead be "ignored errors" (or something
> along those lines) -- for exceptions that, when handled by an
> ErrorClass, do not cause the test run to fail.
"Ignored errors" -- yup, that's better.
>> I think that we should add an interface that lets plugins determine
>> whether errors are successful errors.
>
> Yes. The lack of such an interface is a major problem in error
> handling, as you've made clear.
Good.
>> In passing -- obviously, the names nose.failure.Failure and
>> nose.plugins.errorclass.ErrorClass.isfailure are unfortunate.
>
> Definitely. I don't like 'Bug' though. I'd actually like to split out
> the 3 Failures into a hierarchy something like:
>
> UnexpectedException
> LoaderException
> PluginException
>
> With UnexpectedException replacing Failure, and used in the more
> general cases, and LoaderException and PluginException using
> specifically for exceptions raised during test loading or by plugins.
That's better than my suggestion. But UnexpectedException and
PluginException showing up in a test run report always means a bug in nose
or plugin respectively, right? If so, it seems good to make that clear
somehow in the exception name.
>> Back to the issue at hand. The underlying problem is that plugin debug
>> does not know about errorclass errors. .... As a consequence, there's an
>> immediate problem that errorclass has to return non-None to stop debug
>> from dropping into the debugger on non-failure errorclass errors like skips.
>> But that can stop other plugins working.
[...]
>> 5. Any more?
[...snip suggestions...]
I'll think about those.
John
John J Lee
>> 5. Any more?
>
> Simple, hacky: we could make addError chainable to allow multiple
> plugins to be called, and specify that plugins can return the original
> exception wrapped in an "Ignore" or set an "ignore" or "handled "
> attribute on the exception to tell core and other plugins that the
> exception has been handled.
I'm wary in general of exposing interfaces on exceptions and treating them
as ordinary values, because exceptions have to be instances of a specific
class, which can cause maintenance problems. I can't think of a specific
objection here, though.
> Less hacky, similar to 2 of your ideas: Add an errorHandled(test,
> err) plugin method. For instance, config.plugins.errorHandled(test,
> Skip()) would return True when the SkipTest plugin was active. Then
> ErrorClass plugins would not need to return True from addError, but
> core and other error-aware plugins like debug would need to be updated
> to call errorHandled() before taking action on an error.
The word "handle", though apt, could be confusing: the meaning of "handle"
would be different in .handleError() and .errorHandled(). Returning
non-None from the former means "my plugin is going to handle everything
about this error, taking over core's responsibility; other plugins don't
get a look in after this point in time". The latter means "I've handled
this, it's no longer going to cause the test run to be unsuccessful; other
plugins should behave in an appropriate manner". Maybe
.isIgnoredError(test, err) is better? Not sure.
How would plugins call .errorHandled()? Would the was-it-handled state be
stored in the result?
I wish I had some other requirement simiilar to the errorclass / debug
issue, to help judge the proposed solutions for this problem...
Both changes you suggested might break third-party plugin code. If a
plugin doesn't check for ignored-ness, it won't ignore "ignored errors"
like SkipTest in some cases where they ignored them in the past. On the
other hand, if we were to stop passing ignored errors to .addError(), and
pass them to some other method instead, that would break plugins that
expect to see all errors (I suppose in that case we should call them
something other than "errors"). Either way, it seems like it should be a
0.11 change (needn't be a long time away, I guess). You seem to disagree?
[...]
> I'd like to fix it sooner, if we can find and agree on a simple and
> backwards-compatible fix. If not, 0.11 for sure.
John
John J Lee
> On Sun, 10 Feb 2008, jason pellerin wrote:
[reordering jason's words]
>> I'd like to fix it sooner, if we can find and agree on a simple and
>> backwards-compatible fix. If not, 0.11 for sure.
http://code.google.com/p/python-nose/issues/detail?id=169
It's not a pretty patch. Better patches or suggestions welcome.
> [...]
>>> 5. Any more?
>>
>> Simple, hacky: we could make addError chainable to allow multiple
>> plugins to be called, and specify that plugins can return the original
>> exception wrapped in an "Ignore" or set an "ignore" or "handled "
>> attribute on the exception to tell core and other plugins that the
>> exception has been handled.
Does seem a hack, so didn't implement it.
>> Less hacky, similar to 2 of your ideas: Add an errorHandled(test,
>> err) plugin method. For instance, config.plugins.errorHandled(test,
>> Skip()) would return True when the SkipTest plugin was active. Then
>> ErrorClass plugins would not need to return True from addError, but
>> core and other error-aware plugins like debug would need to be updated
>> to call errorHandled() before taking action on an error.
Didn't really understand how this suggestion would work -- see the post
I'm replying to here.
John