Merge lp:~vila/bzr/625686-selftest-cleanup-deprecation-warnings into lp:bzr

[Vincent Ladeuil]

Cleanup warnings.filters after running selftest

This fixes bug #625686 by cleaning up the warning filter added by cmd_selftest.

See bug description for details but roughly, cmd_selftest wasn't properly isolated.

This is the least intrusive patch, but we may want to revisit either
blackbox/test_selftest.py design (overrideAtttr may replace get_params_passed_to_core)
or re-think the way we play with warnings.filterwarning (we don't use it much
so it may be overkill though).

Finally, having re-read symbol_versioning.py, I notice that almost all
function there start with 'deprecated'. How about renaming the module
itself to deprecated ?

So instead of writing 'symbol_versioning.deprecated_in()' we can use
the simpler 'deprecated.in()' and so on ?

I'm not sure about how to deprecate the 'symbol_versioning' module itself
in that case though ;)

On the other hand, I'm not sure many plugins depend on it either, thoughts ?

[Martin Packman (gz)]

Change looks good to me. One thing, I'd prefer if you passed and closed over the warnings.filters list as well as well as the tuple just added, to defend against that module global changing.

The warnings module upsets me a little more every time I go near it...

review: Approve

[Vincent Ladeuil (vila)]

>>>>> Martin [gz] <email address hidden> writes:

    > Review: Approve

    > Change looks good to me. One thing, I'd prefer if you passed and
    > closed over the warnings.filters list as well as well as the tuple
    > just added, to defend against that module global changing.

Hehe, Yeah, me too, but I refrained. I also refrained making
_check_for_filter a bit more precise...

    > The warnings module upsets me a little more every time I go near it...

I think it comes mostly from the fact that we use it only lightly and
mostly as a global state. Yet, some tests want to... test and modify it.

As I said, I went for the minimal patch, but if we encounter more
problems, it will be good to rework the whole. We have several ways to
catch/ignore/activate deprecation warnings (and we even have our own way
to report warnings and our own way to disable them...) and it would good
to unify this (and take python changes into account,
http://docs.python.org/library/warnings.html mentions changes introduced
for 2.6 and 2.7 that we could use a guide).

May be I should add some comments before landing... :)

[Vincent Ladeuil (vila)]

sent to pqm by email

=== modified file 'NEWS'
--- NEWS	2010-08-27 22:09:35 +0000
+++ NEWS	2010-08-28 15:43:42 +0000
@@ -103,6 +103,9 @@
   directory that was a symlink in the previous commit.
   (Martin Pool, #192859)
 
+* Fix spurious paramiko warning on hardy by ensuring that ``selftest``
+  properly remove its warning filter. (Vincent Ladeuil, #625686)
+
 * ``HTTP/1.1`` test servers now set a ``Content-Length`` header to comply
   with pedantic ``HTTP/1.1`` clients. (Vincent Ladeuil, #568421)
 

=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py	2010-08-20 09:39:20 +0000
+++ bzrlib/builtins.py	2010-08-28 15:43:42 +0000
@@ -3572,9 +3572,6 @@
             parallel=None, lsprof_tests=False):
         from bzrlib.tests import selftest
 
-        # Make deprecation warnings visible, unless -Werror is set
-        symbol_versioning.activate_deprecation_warnings(override=False)
-
         if testspecs_list is not None:
             pattern = '|'.join(testspecs_list)
         else:
@@ -3620,7 +3617,14 @@
                           "starting_with": starting_with
                           }
         selftest_kwargs.update(self.additional_selftest_args)
-        result = selftest(**selftest_kwargs)
+
+        # Make deprecation warnings visible, unless -Werror is set
+        cleanup = symbol_versioning.activate_deprecation_warnings(
+            override=False)
+        try:
+            result = selftest(**selftest_kwargs)
+        finally:
+            cleanup()
         return int(not result)
 
 

=== modified file 'bzrlib/symbol_versioning.py'
--- bzrlib/symbol_versioning.py	2010-06-29 16:21:13 +0000
+++ bzrlib/symbol_versioning.py	2010-08-28 15:43:42 +0000
@@ -29,6 +29,9 @@
            'warn',
            ]
 
+
+import warnings
+# Import the 'warn' symbol so bzrlib can call it even if we redefine it
 from warnings import warn
 
 import bzrlib
@@ -296,7 +299,6 @@
     :param error_only: Only match an 'error' filter
     :return: True if a filter is found, False otherwise
     """
-    import warnings
     for filter in warnings.filters:
         if issubclass(DeprecationWarning, filter[2]):
             # This filter will effect DeprecationWarning
@@ -305,6 +307,19 @@
     return False
 
 
+def _remove_filter_callable(filter):
+    """Build and returns a callable removing filter from the warnings.
+
+    :param filter: The filter to remove (can be None).
+
+    :return: A callable that will remove filter from warnings.filters.
+    """
+    def cleanup():
+        if filter:
+            warnings.filters.remove(filter)
+    return cleanup
+
+
 def suppress_deprecation_warnings(override=True):
     """Call this function to suppress all deprecation warnings.
 
@@ -314,18 +329,17 @@
 
     :param override: If True, always set the ignore, if False, only set the
         ignore if there isn't already a filter.
+
     :return: A callable to remove the new warnings this added.
     """
-    import warnings
     if not override and _check_for_filter(error_only=False):
         # If there is already a filter effecting suppress_deprecation_warnings,
         # then skip it.
-        return
-    warnings.filterwarnings('ignore', category=DeprecationWarning)
-    filter = warnings.filters[0]
-    def cleanup():
-        warnings.filters.remove(filter)
-    return cleanup
+        filter = None
+    else:
+        warnings.filterwarnings('ignore', category=DeprecationWarning)
+        filter = warnings.filters[0]
+    return _remove_filter_callable(filter)
 
 
 def activate_deprecation_warnings(override=True):
@@ -342,10 +356,14 @@
     :param override: If False, only add a filter if there isn't an error filter
         already. (This slightly differs from suppress_deprecation_warnings, in
         because it always overrides everything but -Werror).
+
+    :return: A callable to remove the new warnings this added.
     """
-    import warnings
     if not override and _check_for_filter(error_only=True):
         # DeprecationWarnings are already turned into errors, don't downgrade
         # them to 'default'.
-        return
-    warnings.filterwarnings('default', category=DeprecationWarning)
+        filter = None
+    else:
+        warnings.filterwarnings('default', category=DeprecationWarning)
+        filter = warnings.filters[0]
+    return _remove_filter_callable(filter)

=== modified file 'bzrlib/versionedfile.py'
--- bzrlib/versionedfile.py	2010-08-16 06:46:17 +0000
+++ bzrlib/versionedfile.py	2010-08-28 15:43:42 +0000
@@ -46,7 +46,6 @@
 from bzrlib.transport.memory import MemoryTransport
 """)
 from bzrlib.registry import Registry
-from bzrlib.symbol_versioning import *
 from bzrlib.textmerge import TextMerge
 from bzrlib import bencode