Merge pull request #1 from oesteban/fix/xvfb_and_noise-patch-1

Old xvfbwrapper patch

Author: effigies

nipype/utils/config.py

@@ -266,7 +266,7 @@ def get_display(self):
         #                    shell=True, stdout=sp.DEVNULL))
 
         if self._display is not None:
-            return ':%d' % self._display.vdisplay_num
+            return ':%d' % self._display.new_display
 
         sysdisplay = None
         if self._config.has_option('execution', 'display_variable'):
@@ -281,7 +281,7 @@ def _mock():
 
             # Store a fake Xvfb object
             ndisp = int(sysdisplay.split(':')[-1])
-            Xvfb = namedtuple('Xvfb', ['vdisplay_num', 'stop'])
+            Xvfb = namedtuple('Xvfb', ['new_display', 'stop'])
             self._display = Xvfb(ndisp, _mock)
             return sysdisplay
         else:
@@ -306,12 +306,12 @@ def _mock():
             self._display = Xvfb(nolisten='tcp')
             self._display.start()
 
-            # Older versions of Xvfb used vdisplay_num
-            if hasattr(self._display, 'vdisplay_num'):
-                return ':%d' % self._display.vdisplay_num
+            # Older versions of xvfbwrapper used vdisplay_num
+            if not hasattr(self._display, 'new_display'):
+                setattr(self._display, 'new_display',
+                        self._display.vdisplay_num)
 
-            if hasattr(self._display, 'new_display'):
-                return ':%d' % self._display.new_display
+            return ':%d' % self._display.new_display
 
     def stop_display(self):
         """Closes the display if started"""

nipype/utils/tests/test_config.py

@@ -15,8 +15,16 @@
 except ImportError:
     has_Xvfb = False
 
-xvfbpatch = MagicMock()
-xvfbpatch.Xvfb.return_value = MagicMock(vdisplay_num=2010)
+# Define mocks for xvfbwrapper. Do not forget the spec to ensure that
+# hasattr() checks return False with missing attributes.
+xvfbpatch = MagicMock(spec=['Xvfb'])
+xvfbpatch.Xvfb.return_value = MagicMock(spec=['new_display', 'start', 'stop'],
+                                        new_display=2010)
+
+# Mock the legacy xvfbwrapper.Xvfb class (changed display attribute name)
+xvfbpatch_old = MagicMock(spec=['Xvfb'])
+xvfbpatch_old.Xvfb.return_value = MagicMock(spec=['vdisplay_num', 'start', 'stop'],
+                                            vdisplay_num=2010)
 
 
 @pytest.mark.parametrize('dispnum', range(5))
@@ -27,6 +35,8 @@ def test_display_config(monkeypatch, dispnum):
     config.set('execution', 'display_variable', dispstr)
     monkeypatch.delitem(os.environ, 'DISPLAY', raising=False)
     assert config.get_display() == config.get('execution', 'display_variable')
+    # Test that it was correctly cached
+    assert config.get_display() == config.get('execution', 'display_variable')
 
 
 @pytest.mark.parametrize('dispnum', range(5))
@@ -37,6 +47,8 @@ def test_display_system(monkeypatch, dispnum):
     dispstr = ':%d' % dispnum
     monkeypatch.setitem(os.environ, 'DISPLAY', dispstr)
     assert config.get_display() == dispstr
+    # Test that it was correctly cached
+    assert config.get_display() == dispstr
 
 
 def test_display_config_and_system(monkeypatch):
@@ -46,6 +58,8 @@ def test_display_config_and_system(monkeypatch):
     config.set('execution', 'display_variable', dispstr)
     monkeypatch.setitem(os.environ, 'DISPLAY', ':0')
     assert config.get_display() == dispstr
+    # Test that it was correctly cached
+    assert config.get_display() == dispstr
 
 
 def test_display_noconfig_nosystem_patched(monkeypatch):
@@ -56,6 +70,8 @@ def test_display_noconfig_nosystem_patched(monkeypatch):
     monkeypatch.delitem(os.environ, 'DISPLAY', raising=False)
     monkeypatch.setitem(sys.modules, 'xvfbwrapper', xvfbpatch)
     assert config.get_display() == ":2010"
+    # Test that it was correctly cached
+    assert config.get_display() == ':2010'
 
 
 def test_display_empty_patched(monkeypatch):
@@ -69,6 +85,38 @@ def test_display_empty_patched(monkeypatch):
     monkeypatch.setitem(os.environ, 'DISPLAY', '')
     monkeypatch.setitem(sys.modules, 'xvfbwrapper', xvfbpatch)
     assert config.get_display() == ':2010'
+    # Test that it was correctly cached
+    assert config.get_display() == ':2010'
+
+
+def test_display_noconfig_nosystem_patched_oldxvfbwrapper(monkeypatch):
+    """
+    Check that when no $DISPLAY nor option are specified,
+    a virtual Xvfb is used (with a legacy version of xvfbwrapper).
+    """
+    config._display = None
+    if config.has_option('execution', 'display_variable'):
+        config._config.remove_option('execution', 'display_variable')
+    monkeypatch.delitem(os.environ, 'DISPLAY', raising=False)
+    monkeypatch.setitem(sys.modules, 'xvfbwrapper', xvfbpatch_old)
+    assert config.get_display() == ":2010"
+    # Test that it was correctly cached
+    assert config.get_display() == ':2010'
+
+
+def test_display_empty_patched_oldxvfbwrapper(monkeypatch):
+    """
+    Check that when $DISPLAY is empty string and no option is specified,
+    a virtual Xvfb is used (with a legacy version of xvfbwrapper).
+    """
+    config._display = None
+    if config.has_option('execution', 'display_variable'):
+        config._config.remove_option('execution', 'display_variable')
+    monkeypatch.setitem(os.environ, 'DISPLAY', '')
+    monkeypatch.setitem(sys.modules, 'xvfbwrapper', xvfbpatch_old)
+    assert config.get_display() == ':2010'
+    # Test that it was correctly cached
+    assert config.get_display() == ':2010'
 
 
 def test_display_noconfig_nosystem_notinstalled(monkeypatch):
@@ -109,7 +157,10 @@ def test_display_noconfig_nosystem_installed(monkeypatch):
     if config.has_option('execution', 'display_variable'):
         config._config.remove_option('execution', 'display_variable')
     monkeypatch.delitem(os.environ, 'DISPLAY', raising=False)
-    assert int(config.get_display().split(':')[-1]) > 1000
+    newdisp = config.get_display()
+    assert int(newdisp.split(':')[-1]) > 1000
+    # Test that it was correctly cached
+    assert config.get_display() == newdisp
 
 
 @pytest.mark.skipif(not has_Xvfb, reason='xvfbwrapper not installed')
@@ -122,7 +173,10 @@ def test_display_empty_installed(monkeypatch):
     if config.has_option('execution', 'display_variable'):
         config._config.remove_option('execution', 'display_variable')
     monkeypatch.setitem(os.environ, 'DISPLAY', '')
-    assert int(config.get_display().split(':')[-1]) > 1000
+    newdisp = config.get_display()
+    assert int(newdisp.split(':')[-1]) > 1000
+    # Test that it was correctly cached
+    assert config.get_display() == newdisp
 
 
 def test_display_empty_macosx(monkeypatch):