Bazaar

Merge lp:~jameinel/bzr/2.1.0b4-win32-test-imports into lp:bzr

  1. 2.1.0b4-win32-test-imports
  2. Merge into bzr.dev
Proposed by John A Meinel
Status: Merged
Approved by: Martin Pool
Approved revision: not available
Merged at revision: not available
Proposed branch: lp:~jameinel/bzr/2.1.0b4-win32-test-imports
Merge into: lp:bzr
Diff against target: 120 lines (+59/-11)
3 files modified
bzrlib/tests/__init__.py (+31/-0)
bzrlib/tests/test_osutils.py (+12/-11)
bzrlib/tests/test_selftest.py (+16/-0)
To merge this branch: bzr merge lp:~jameinel/bzr/2.1.0b4-win32-test-imports
Reviewer Review Type Date Requested Status
Martin Pool Needs Fixing
Vincent Ladeuil Approve
Review via email: mp+15829@code.launchpad.net
To post a comment you must log in.
Revision history for this message
John A Meinel (jameinel) wrote :

This patch does 2 things:

1) Adds a trap around importing 'termios' since it doesn't exist on windows. This is the most important part, and could be done in a different way.
2) Adds bzrlib.tests._ModuleFeature. We have a very common pattern of running tests based on whether a given module can be imported. (Think of all the compiled extension tests.) It seemed nicer to me to have the module be part of the Feature, so that you don't end up with:

 self.requireFeature(feature)
 from foo import bar

Given that 'feature' is already doing 'from foo import bar' to test that the module is available.

I haven't updated lots of code paths to use _ModuleFeature, but that would certainly be a reasonable next step.

Revision history for this message
Vincent Ladeuil (vila) wrote :

Nice.

But why a leading '_' for _ModuleFeature ?

Two empty lines will be enough after that class.

'a feature than describes' 'that' ?

From the docstring, I'm really tempted to call it ModuleAvailableFeature no ?

If there is agreement to land it, let's try to deploy it as widely and quickly as possible.

review: Approve
Revision history for this message
John A Meinel (jameinel) wrote :

Because it follows the standard patter for other features. Which is that the class is defined as "_FooFeature" and then the instance is "FooFeature = _FooFeature()".

I'm okay with ModuleAvailableFeature if you are happier with it.

Revision history for this message
Martin Pool (mbp) wrote :

Nice.

ModuleAvailableFeature should be in NEWS, and ideally get a paragraph in the testing guide.

review: Needs Fixing
Revision history for this message
Vincent Ladeuil (vila) wrote :

>>>>> "jam" == John A Meinel <email address hidden> writes:

    jam> Because it follows the standard patter for other
    jam> features. Which is that the class is defined as
    jam> "_FooFeature" and then the instance is "FooFeature =
    jam> _FooFeature()".

Yeah, of course... on the other hand, since there isn't a feature
object in that case...

    jam> I'm okay with ModuleAvailableFeature if you are happier with it.

That was a gut feeling I should have explain a bit better: there
may be other features for a module, I have no idea which so far
but since we are checking the availability here, I thought it was
worth being clear about it.

With that name, there is no doubt left when the feature is *used*
and the probe() code not under the reader eyes .

Revision history for this message
Martin Pool (mbp) wrote :

2009/12/9 Vincent Ladeuil <email address hidden>:
>>>>>> "jam" == John A Meinel <email address hidden> writes:
>
>    jam> Because it follows the standard patter for other
>    jam> features. Which is that the class is defined as
>    jam> "_FooFeature" and then the instance is "FooFeature =
>    jam> _FooFeature()".
>
> Yeah, of course... on the other hand, since there isn't a feature
> object in that case...

I've actually always found this a bit weird because it violates our
standard pattern of ClassName and instance_name. So John's thing is
good with me.

--
Martin <http://launchpad.net/~mbp/>

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bzrlib/tests/__init__.py'
2--- bzrlib/tests/__init__.py 2009-12-04 10:16:00 +0000
3+++ bzrlib/tests/__init__.py 2009-12-08 21:48:11 +0000
4@@ -4247,6 +4247,37 @@
5 UnicodeFilenameFeature = _UnicodeFilenameFeature()
6
7
8+class ModuleAvailableFeature(Feature):
9+ """This is a feature than describes a module we want to be available.
10+
11+ Declare the name of the module in __init__(), and then after probing, the
12+ module will be available as 'self.module'.
13+
14+ :ivar module: The module if it is available, else None.
15+ """
16+
17+ def __init__(self, module_name):
18+ super(ModuleAvailableFeature, self).__init__()
19+ self.module_name = module_name
20+
21+ def _probe(self):
22+ try:
23+ self._module = __import__(self.module_name, {}, {}, [''])
24+ return True
25+ except ImportError:
26+ return False
27+
28+ @property
29+ def module(self):
30+ if self.available(): # Make sure the probe has been done
31+ return self._module
32+ return None
33+
34+ def feature_name(self):
35+ return self.module_name
36+
37+
38+
39 def probe_unicode_in_user_encoding():
40 """Try to encode several unicode strings to use in unicode-aware tests.
41 Return first successfull match.
42
43=== modified file 'bzrlib/tests/test_osutils.py'
44--- bzrlib/tests/test_osutils.py 2009-12-04 10:09:11 +0000
45+++ bzrlib/tests/test_osutils.py 2009-12-08 21:48:11 +0000
46@@ -23,7 +23,6 @@
47 import socket
48 import stat
49 import sys
50-import termios
51 import time
52
53 from bzrlib import (
54@@ -54,6 +53,8 @@
55
56 UTF8DirReaderFeature = _UTF8DirReaderFeature()
57
58+TermIOSFeature = tests.ModuleAvailableFeature('termios')
59+
60
61 def _already_unicode(s):
62 return s
63@@ -1961,20 +1962,20 @@
64 sys.stdout = None
65 self.assertEquals(None, osutils.terminal_width())
66
67- def test_TIOCGWINSZ(self):
68+ def test_no_TIOCGWINSZ(self):
69+ self.requireFeature(TermIOSFeature)
70+ termios = TermIOSFeature.module
71 # bug 63539 is about a termios without TIOCGWINSZ attribute
72- exist = True
73 try:
74 orig = termios.TIOCGWINSZ
75 except AttributeError:
76- exist = False
77-
78- def restore():
79- if exist:
80+ # We won't remove TIOCGWINSZ, because it doesn't exist anyway :)
81+ pass
82+ else:
83+ def restore():
84 termios.TIOCGWINSZ = orig
85- self.addCleanup(restore)
86-
87- del termios.TIOCGWINSZ
88+ self.addCleanup(restore)
89+ del termios.TIOCGWINSZ
90 del os.environ['BZR_COLUMNS']
91 del os.environ['COLUMNS']
92- self.assertEquals(None, osutils.terminal_width())
93+ self.assertIs(None, osutils.terminal_width())
94
95=== modified file 'bzrlib/tests/test_selftest.py'
96--- bzrlib/tests/test_selftest.py 2009-12-04 09:09:18 +0000
97+++ bzrlib/tests/test_selftest.py 2009-12-08 21:48:11 +0000
98@@ -2497,6 +2497,22 @@
99 self.assertIs(feature, exception.args[0])
100
101
102+class TestModuleAvailableFeature(tests.TestCase):
103+
104+ def test_available_module(self):
105+ feature = tests.ModuleAvailableFeature('bzrlib.tests')
106+ self.assertEqual('bzrlib.tests', feature.module_name)
107+ self.assertEqual('bzrlib.tests', str(feature))
108+ self.assertTrue(feature.available())
109+ self.assertIs(tests, feature.module)
110+
111+ def test_unavailable_module(self):
112+ feature = tests.ModuleAvailableFeature('bzrlib.no_such_module_exists')
113+ self.assertEqual('bzrlib.no_such_module_exists', str(feature))
114+ self.assertFalse(feature.available())
115+ self.assertIs(None, feature.module)
116+
117+
118 class TestSelftestFiltering(tests.TestCase):
119
120 def setUp(self):