patch for review: unittest ImportError handling

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

patch for review: unittest ImportError handling

Chris Jerdonek-3
Hi folks,

I have a patch to the unittest module for review here:

http://bugs.python.org/issue7559#msg102801

(There have already been a couple rounds of discussion on how to best
fix this.)

This is my first patch, so any feedback is appreciated.

Thanks,
--Chris
_______________________________________________
Python-Dev mailing list
[hidden email]
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe: http://mail.python.org/mailman/options/python-dev/lists%40nabble.com
Reply | Threaded
Open this post in threaded view
|

Re: patch for review: unittest ImportError handling

Michael Foord-5
On 14/04/2010 05:49, Chris Jerdonek wrote:

> Hi folks,
>
> I have a patch to the unittest module for review here:
>
> http://bugs.python.org/issue7559#msg102801
>
> (There have already been a couple rounds of discussion on how to best
> fix this.)
>
> This is my first patch, so any feedback is appreciated.
>    

I'm still not convinced that this isn't a backwards incompatible change
- up until now, however horrible it may be, TestLoader.loadTestsFromName
only raised an AttributeError when it failed to load a test. Changing it
to allow it propagate ImportError means that code catching errors by
handling AttributeError will be potentially broken by the fix. I'm
certainly happy to discuss this here though.

An alternative fix would be for a new API and deprecation of
loadTestFromName. A new API could return a placeholder test that raises
the original error when run - that way individual errors don't break the
test collection phase but are still reported. This *could* be added to
loadTestsFromName with an optional argument.

By the way, in general please don't assign unittest bugs *away* from me
on the tracker. I'm maintaining unittest and have been watching this
issue. I haven't given it much attention recently because of getting the
new features ready for the 2.7b1 release. I will certainly want to look
through the patch before it is committed, if the consensus here is that
changing this API to raise an ImportError is not a backwards
incompatible change.

All the best,

Michael Foord

> Thanks,
> --Chris
> _______________________________________________
> Python-Dev mailing list
> [hidden email]
> http://mail.python.org/mailman/listinfo/python-dev
> Unsubscribe: http://mail.python.org/mailman/options/python-dev/fuzzyman%40voidspace.org.uk
>    


--
http://www.ironpythoninaction.com/

_______________________________________________
Python-Dev mailing list
[hidden email]
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe: http://mail.python.org/mailman/options/python-dev/lists%40nabble.com
Reply | Threaded
Open this post in threaded view
|

Re: patch for review: unittest ImportError handling

Robert Collins
I'd really like to see a fix that works with loadTestsFromNames - generating failing tests, for instance, and the failing tests having the full import error string in them. This doesn't preclude raising ImportError from loadTestFromName, and in fact I'd encourage that as a step towards the aforementioned loadTestsFromNames change.

It is a change with the existing misguided behaviour; sometimes thats better to do than preserving compat; as you say a new API might be best overall though. I'd go with a keyword arg rather than a new function, I think.

_______________________________________________
Python-Dev mailing list
[hidden email]
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe: http://mail.python.org/mailman/options/python-dev/lists%40nabble.com
Reply | Threaded
Open this post in threaded view
|

Re: patch for review: unittest ImportError handling

Chris Jerdonek-3
In reply to this post by Michael Foord-5
On Wed, Apr 14, 2010 at 2:07 AM, Michael Foord
<[hidden email]> wrote:
> I'm still not convinced that this isn't a backwards incompatible change - up
> until now, however horrible it may be, TestLoader.loadTestsFromName only
> raised an AttributeError when it failed to load a test. Changing it to allow
> it propagate ImportError means that code catching errors by handling
> AttributeError will be potentially broken by the fix. I'm certainly happy to
> discuss this here though.

Thanks for your response. Michael.  To understand your position
better, would you view changing the AttributeError in *any* way an
incompatible change (e.g. changing just the error message), or is it
only changing the error type that you view as backwards incompatible?

The reason I ask is that I think it's the change in what the error
contains (e.g. the error message and stack trace) that is the more
important part of this change -- rather than whether that information
be wrapped in an ImportError versus an AttributeError.  It is the
information rather than the particular error type that will assist an
end-developer in diagnosing future unit-test failures.

> An alternative fix would be for a new API and deprecation of
> loadTestFromName. A new API could return a placeholder test that raises the
> original error when run - that way individual errors don't break the test
> collection phase but are still reported. This *could* be added to
> loadTestsFromName with an optional argument.

I'm not opposed to adding a new API, but I think it would be valuable
if we could find a way for people to enjoy the benefit of not having
the stack trace swallowed -- without having to change their code.  I'm
currently part of a project where the current behavior makes it harder
to track down unit test failures.  I imagine many developers are in a
similar situation.  This seems to be a bug, and such developers may be
in a position where they can't change how their unit tests are run,
but they're nevertheless stuck diagnosing the failures.

I'm writing on the assumption that there is a way to preserve the
stack trace and error message of an ImportError while re-raising it as
an AttributeError.

> By the way, in general please don't assign unittest bugs *away* from me on
> the tracker.

Will do.  I hadn't come across any guidance re: assigning in the
development workflow documentation.  Maybe we should add a cautionary
note about this in the documentation somewhere.  In general, do all
reports stay assigned to the maintainer (if a maintainer exists), or
is it a per-maintainer preference on how that is handled?

Thanks,
--Chris
_______________________________________________
Python-Dev mailing list
[hidden email]
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe: http://mail.python.org/mailman/options/python-dev/lists%40nabble.com
Reply | Threaded
Open this post in threaded view
|

Re: patch for review: unittest ImportError handling

Michael Foord-5
On 14/04/2010 12:54, Chris Jerdonek wrote:

> On Wed, Apr 14, 2010 at 2:07 AM, Michael Foord
> <[hidden email]>  wrote:
>    
>> I'm still not convinced that this isn't a backwards incompatible change - up
>> until now, however horrible it may be, TestLoader.loadTestsFromName only
>> raised an AttributeError when it failed to load a test. Changing it to allow
>> it propagate ImportError means that code catching errors by handling
>> AttributeError will be potentially broken by the fix. I'm certainly happy to
>> discuss this here though.
>>      
> Thanks for your response. Michael.  To understand your position
> better, would you view changing the AttributeError in *any* way an
> incompatible change (e.g. changing just the error message), or is it
> only changing the error type that you view as backwards incompatible?
>
> The reason I ask is that I think it's the change in what the error
> contains (e.g. the error message and stack trace) that is the more
> important part of this change -- rather than whether that information
> be wrapped in an ImportError versus an AttributeError.  It is the
> information rather than the particular error type that will assist an
> end-developer in diagnosing future unit-test failures.
>    

Changing the error message to provide more useful information, possibly
including the original traceback, would certainly avoid the potential
for incompatibility. I'd be interested in seeing what other folks here
on python-dev think.

>    
>> An alternative fix would be for a new API and deprecation of
>> loadTestFromName. A new API could return a placeholder test that raises the
>> original error when run - that way individual errors don't break the test
>> collection phase but are still reported. This *could* be added to
>> loadTestsFromName with an optional argument.
>>      
> I'm not opposed to adding a new API, but I think it would be valuable
> if we could find a way for people to enjoy the benefit of not having
> the stack trace swallowed -- without having to change their code.

It's a double edged sword though - it means existing users depending on
the fact that this API only raises AttributeError have to change *their*
code.

For a new API I like Rob Collin's suggestion of a keyword argument to
loadTestsFromNames that returns an error placeholder instead of raising
an exception. That couldn't be put into 2.7 now though.

> I'm
> currently part of a project where the current behavior makes it harder
> to track down unit test failures.  I imagine many developers are in a
> similar situation.  This seems to be a bug, and such developers may be
> in a position where they can't change how their unit tests are run,
> but they're nevertheless stuck diagnosing the failures.
>
> I'm writing on the assumption that there is a way to preserve the
> stack trace and error message of an ImportError while re-raising it as
> an AttributeError.
>
>    

The original stacktrace could be included as part of the error message.
Pretty horrible though.


>> By the way, in general please don't assign unittest bugs *away* from me on
>> the tracker.
>>      
> Will do.  I hadn't come across any guidance re: assigning in the
> development workflow documentation.  Maybe we should add a cautionary
> note about this in the documentation somewhere.  In general, do all
> reports stay assigned to the maintainer (if a maintainer exists), or
> is it a per-maintainer preference on how that is handled?
>
>    
Where an issue is assigned to an existing maintainer I would wait for a
response on the issue tracker or raise it here on python-dev rather than
re-assigning the issue. In general issues corresponding to modules that
have an active maintainer should be assigned to the maintainer.

It is particularly an issue for me with unittest because I want to track
changes in unittest in the unittest2 package and need to know about all
changes. I'm often around on #python-dev to discuss things.

All the best,

Michael Foord


> Thanks,
> --Chris
>    


--
http://www.ironpythoninaction.com/

_______________________________________________
Python-Dev mailing list
[hidden email]
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe: http://mail.python.org/mailman/options/python-dev/lists%40nabble.com
Reply | Threaded
Open this post in threaded view
|

Re: patch for review: unittest ImportError handling

Chris Jerdonek-3
On Wed, Apr 14, 2010 at 4:12 AM, Michael Foord
<[hidden email]> wrote:

> On 14/04/2010 12:54, Chris Jerdonek wrote:
>>
>> On Wed, Apr 14, 2010 at 2:07 AM, Michael Foord
>> <[hidden email]>  wrote:
>>
>>>
>>> I'm still not convinced that this isn't a backwards incompatible change -
>>> up
>>> until now, however horrible it may be, TestLoader.loadTestsFromName only
>>> raised an AttributeError when it failed to load a test. Changing it to
>>> allow
>>> it propagate ImportError means that code catching errors by handling
>>> AttributeError will be potentially broken by the fix. I'm certainly happy
>>> to
>>> discuss this here though.
>>>
>>
>> Thanks for your response. Michael.  To understand your position
>> better, would you view changing the AttributeError in *any* way an
>> incompatible change (e.g. changing just the error message), or is it
>> only changing the error type that you view as backwards incompatible?
>>
>> The reason I ask is that I think it's the change in what the error
>> contains (e.g. the error message and stack trace) that is the more
>> important part of this change -- rather than whether that information
>> be wrapped in an ImportError versus an AttributeError.  It is the
>> information rather than the particular error type that will assist an
>> end-developer in diagnosing future unit-test failures.
>>
>
> Changing the error message to provide more useful information, possibly
> including the original traceback, would certainly avoid the potential for
> incompatibility. I'd be interested in seeing what other folks here on
> python-dev think.
>
>>
>>>
>>> An alternative fix would be for a new API and deprecation of
>>> loadTestFromName. A new API could return a placeholder test that raises
>>> the
>>> original error when run - that way individual errors don't break the test
>>> collection phase but are still reported. This *could* be added to
>>> loadTestsFromName with an optional argument.
>>>
>>
>> I'm not opposed to adding a new API, but I think it would be valuable
>> if we could find a way for people to enjoy the benefit of not having
>> the stack trace swallowed -- without having to change their code.
>
> It's a double edged sword though - it means existing users depending on the
> fact that this API only raises AttributeError have to change *their* code.
>
> For a new API I like Rob Collin's suggestion of a keyword argument to
> loadTestsFromNames that returns an error placeholder instead of raising an
> exception. That couldn't be put into 2.7 now though.
>
>> I'm
>> currently part of a project where the current behavior makes it harder
>> to track down unit test failures.  I imagine many developers are in a
>> similar situation.  This seems to be a bug, and such developers may be
>> in a position where they can't change how their unit tests are run,
>> but they're nevertheless stuck diagnosing the failures.
>>
>> I'm writing on the assumption that there is a way to preserve the
>> stack trace and error message of an ImportError while re-raising it as
>> an AttributeError.
>>
>>
>
> The original stacktrace could be included as part of the error message.
> Pretty horrible though.

I just experimented with this myself.  Couldn't we do something like
the following to change the error type and preserve the stack trace --
without including the stack trace as part of the message?

try:
    foo()
except ImportError:
    excType, excValue, excTraceback = sys.exc_info()
    raise AttributeError, excValue, excTraceback

Superficially this seems to have the desired effect.  We could also
adjust the error text to indicate that the AttributeError was
originally an ImportError.

The above is similar to the code you referenced in one of your
comments to issue 7559--

http://twistedmatrix.com/trac/browser/trunk/twisted/python/reflect.py?rev=28196#L384

--Chris
_______________________________________________
Python-Dev mailing list
[hidden email]
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe: http://mail.python.org/mailman/options/python-dev/lists%40nabble.com
Reply | Threaded
Open this post in threaded view
|

Re: patch for review: unittest ImportError handling

Nick Coghlan
In reply to this post by Michael Foord-5
Michael Foord wrote:
> Changing the error message to provide more useful information, possibly
> including the original traceback, would certainly avoid the potential
> for incompatibility. I'd be interested in seeing what other folks here
> on python-dev think.

Without looking at the details, my question is whether this can be
ignored in 2.x and fixed for 3.x by setting __cause__ appropriately
(i.e. by using the "raise from" syntax).

Cheers,
Nick.

--
Nick Coghlan   |   [hidden email]   |   Brisbane, Australia
---------------------------------------------------------------
_______________________________________________
Python-Dev mailing list
[hidden email]
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe: http://mail.python.org/mailman/options/python-dev/lists%40nabble.com
Reply | Threaded
Open this post in threaded view
|

Re: patch for review: unittest ImportError handling

Michael Foord-5
On 14/04/2010 16:53, Nick Coghlan wrote:

> Michael Foord wrote:
>    
>> Changing the error message to provide more useful information, possibly
>> including the original traceback, would certainly avoid the potential
>> for incompatibility. I'd be interested in seeing what other folks here
>> on python-dev think.
>>      
> Without looking at the details, my question is whether this can be
> ignored in 2.x and fixed for 3.x by setting __cause__ appropriately
> (i.e. by using the "raise from" syntax).
>    

Yes, definitely - if maintaining the exception as an AttributeError is
determined to be the correct course of action for that API.

Chris showed an alternative way of achieving the same effect for Python
2, but that is possibly moot given that 2.7b1 is out (unless this is an
acceptable bugfix to include in b2).

All the best,

Michael

> Cheers,
> Nick.
>
>    


--
http://www.ironpythoninaction.com/

_______________________________________________
Python-Dev mailing list
[hidden email]
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe: http://mail.python.org/mailman/options/python-dev/lists%40nabble.com