Merge pull request #3834 from ivanov/nbconvert-better-tests

This PR fixes a few issues with nbconvert tests

The code for testing 'ipython nbconvert' prior to this PR did not work as
intended, and simply swallowed errors when pandoc wasn't installed, for example.

This PR adds a new get_output_error_code utility for easier checking of error
(looking at return code as opposed to the contents of stdout for the word
'error').

This new machinery is leveraged when calling nbconvert during tests.

IPython/nbconvert/tests/base.py

@@ -13,55 +13,25 b' Contains base test class for nbconvert'
 # Imports
 #-----------------------------------------------------------------------------
 
-import subprocess
 import os
 import glob
 import shutil
-import sys
 
 import IPython
-from IPython.utils.tempdir import TemporaryDirectory
-from IPython.utils import py3compat
+from IPython.utils.tempdir import TemporaryWorkingDirectory
+from IPython.utils.process import get_output_error_code
+from IPython.testing.tools import get_ipython_cmd
+
+# a trailing space allows for simpler concatenation with the other arguments
+ipy_cmd = get_ipython_cmd(as_string=True) + " "
 
 #-----------------------------------------------------------------------------
 # Classes and functions
 #-----------------------------------------------------------------------------
 
-class TemporaryWorkingDirectory(TemporaryDirectory):
-    """
-    Creates a temporary directory and sets the cwd to that directory.
-    Automatically reverts to previous cwd upon cleanup.
-    Usage example:
-
-        with TemporaryWorakingDirectory() as tmpdir:
-            ...
-    """
-
-    def __init__(self, **kw):
-        """
-        Constructor
-        """
-        super(TemporaryWorkingDirectory, self).__init__(**kw)
-
-        #Change cwd to new temp dir.  Remember old cwd.
-        self.old_wd = os.getcwd()
-        os.chdir(self.name)
-
-
-    def cleanup(self):
-        """
-        Destructor
-        """
-
-        #Revert to old cwd.
-        os.chdir(self.old_wd)
-
-        #Cleanup
-        super(TemporaryWorkingDirectory, self).cleanup()
-
 
 class TestsBase(object):
-    """Base tests class.  Contains usefull fuzzy comparison and nbconvert
+    """Base tests class.  Contains useful fuzzy comparison and nbconvert
     functions."""
 
 
@@ -126,30 +96,25 b' class TestsBase(object):'
             text = text.replace(search, replacement)
         return text
 
-            
     def create_temp_cwd(self, copy_filenames=None):
         temp_dir = TemporaryWorkingDirectory()
 
         #Copy the files if requested.
-        if not copy_filenames is None:
+        if copy_filenames is not None:
             self.copy_files_to(copy_filenames)
 
         #Return directory handler
         return temp_dir
 
 
-    def copy_files_to(self, copy_filenames=None, destination=None):
-        
-        #Copy test files into the destination directory.
-        if copy_filenames:
-            for pattern in copy_filenames:
-                for match in glob.glob(os.path.join(self._get_files_path(), pattern)):
-                    if destination is None:
-                        shutil.copyfile(match, os.path.basename(match))
-                    else:
-                        if not os.path.isdir(destination):
-                            os.makedirs(destination)
-                        shutil.copyfile(match, os.path.join(destination, os.path.basename(match)))
+    def copy_files_to(self, copy_filenames, dest='.'):
+        "Copy test files into the destination directory"
+        if not os.path.isdir(dest):
+            os.makedirs(dest)
+        files_path = self._get_files_path()
+        for pattern in copy_filenames:
+            for match in glob.glob(os.path.join(files_path, pattern)):
+                shutil.copyfile(match, os.path.join(dest, os.path.basename(match)))
 
 
     def _get_files_path(self):
@@ -166,12 +131,8 b' class TestsBase(object):'
         return path
 
 
-    def call(self, parameters):
-        output = subprocess.Popen(parameters, stdout=subprocess.PIPE).communicate()[0]
-        
-        #Convert the output to a string if running Python3
-        if py3compat.PY3:
-            return output.decode('utf-8')
-        else:
-            return output
-     
  No newline at end of file
+    def call(self, parameters, raise_on_error=True):
+        stdout, stderr, retcode = get_output_error_code(ipy_cmd + parameters)
+        if retcode != 0 and raise_on_error:
+            raise OSError(stderr)
+        return stdout, stderr

IPython/nbconvert/tests/test_nbconvertapp.py

@@ -16,7 +16,6 b' Contains tests for the nbconvertapp'
 import os
 from .base import TestsBase
 
-from IPython.utils import py3compat
 from IPython.testing import decorators as dec
 
     
@@ -24,12 +23,6 b' from IPython.testing import decorators as dec'
 # Constants
 #-----------------------------------------------------------------------------
 
-# Define ipython commandline name
-if py3compat.PY3:
-    IPYTHON = 'ipython3'
-else:
-    IPYTHON = 'ipython'
-
 
 #-----------------------------------------------------------------------------
 # Classes and functions
@@ -44,7 +37,8 b' class TestNbConvertApp(TestsBase):'
         Will help show if no notebooks are specified?
         """
         with self.create_temp_cwd():
-            assert "see '--help-all'" in self.call([IPYTHON, 'nbconvert'])
+            out, err = self.call('nbconvert', raise_on_error=False)
+            assert "see '--help-all'" in out
 
 
     def test_glob(self):
@@ -52,8 +46,7 b' class TestNbConvertApp(TestsBase):'
         Do search patterns work for notebook names?
         """
         with self.create_temp_cwd(['notebook*.ipynb']):
-            assert not 'error' in self.call([IPYTHON, 'nbconvert', 
-                '--to="python"', '--notebooks=["*.ipynb"]']).lower()
+            self.call('nbconvert --to="python" --notebooks=\'["*.ipynb"]\'')
             assert os.path.isfile('notebook1.py')
             assert os.path.isfile('notebook2.py')
 
@@ -62,10 +55,10 b' class TestNbConvertApp(TestsBase):'
         """
         Do search patterns work for subdirectory notebook names?
         """
-        with self.create_temp_cwd() as cwd:
+        with self.create_temp_cwd():
             self.copy_files_to(['notebook*.ipynb'], 'subdir/')
-            assert not 'error' in self.call([IPYTHON, 'nbconvert', '--to="python"', 
-                '--notebooks=["%s"]' % os.path.join('subdir', '*.ipynb')]).lower()
+            self.call('nbconvert --to="python" --notebooks='
+                      '\'["%s"]\'' % os.path.join('subdir', '*.ipynb'))
             assert os.path.isfile('notebook1.py')
             assert os.path.isfile('notebook2.py')
 
@@ -75,32 +68,34 b' class TestNbConvertApp(TestsBase):'
         Do explicit notebook names work?
         """
         with self.create_temp_cwd(['notebook*.ipynb']):
-            assert not 'error' in self.call([IPYTHON, 'nbconvert', '--to="python"', 
-                '--notebooks=["notebook2.ipynb"]']).lower()
+            self.call('nbconvert --to="python" --notebooks='
+                      '\'["notebook2.ipynb"]\'')
             assert not os.path.isfile('notebook1.py')
             assert os.path.isfile('notebook2.py')
 
 
     @dec.onlyif_cmds_exist('pdflatex')
+    @dec.onlyif_cmds_exist('pandoc')
     def test_post_processor(self):
         """
         Do post processors work?
         """
         with self.create_temp_cwd(['notebook1.ipynb']):
-            assert not 'error' in self.call([IPYTHON, 'nbconvert', '--to="latex"', 
-                'notebook1', '--post="PDF"', '--PDFPostProcessor.verbose=True']).lower()
+            self.call('nbconvert --to="latex" notebook1'
+                      ' --post="PDF" --PDFPostProcessor.verbose=True')
             assert os.path.isfile('notebook1.tex')
             print("\n\n\t" + "\n\t".join([f for f in os.listdir('.') if os.path.isfile(f)]) + "\n\n")
             assert os.path.isfile('notebook1.pdf')
 
 
+    @dec.onlyif_cmds_exist('pandoc')
     def test_template(self):
         """
         Do export templates work?
         """
         with self.create_temp_cwd(['notebook2.ipynb']):
-            assert not 'error' in self.call([IPYTHON, 'nbconvert', '--to=slides', 
-                '--notebooks=["notebook2.ipynb"]', '--template=reveal']).lower()
+            self.call('nbconvert --to=slides --notebooks='
+                      '\'["notebook2.ipynb"]\' --template=reveal')
             assert os.path.isfile('notebook2.slides.html')
             with open('notebook2.slides.html') as f:
                 assert '/reveal.css' in f.read()
@@ -111,8 +106,8 b' class TestNbConvertApp(TestsBase):'
         Can a search pattern be used along with matching explicit notebook names?
         """
         with self.create_temp_cwd(['notebook*.ipynb']):
-            assert not 'error' in self.call([IPYTHON, 'nbconvert', '--to="python"', 
-                '--notebooks=["*.ipynb", "notebook1.ipynb", "notebook2.ipynb"]']).lower()
+            self.call('nbconvert --to="python" --notebooks='
+                      '\'["*.ipynb","notebook1.ipynb","notebook2.ipynb"]\'')
             assert os.path.isfile('notebook1.py')
             assert os.path.isfile('notebook2.py')
 
@@ -122,8 +117,8 b' class TestNbConvertApp(TestsBase):'
         Can explicit notebook names be used and then a matching search pattern?
         """
         with self.create_temp_cwd(['notebook*.ipynb']):
-            assert not 'error' in self.call([IPYTHON, 'nbconvert', '--to="python"', 
-                '--notebooks=["notebook1.ipynb", "notebook2.ipynb", "*.ipynb"]']).lower()
+            self.call('nbconvert --to="python" --notebooks='
+                      '\'["notebook1.ipynb","notebook2.ipynb","*.ipynb"]\'')
             assert os.path.isfile('notebook1.py')
             assert os.path.isfile('notebook2.py')
 
@@ -133,7 +128,7 b' class TestNbConvertApp(TestsBase):'
         Does the default config work?
         """
         with self.create_temp_cwd(['notebook*.ipynb', 'ipython_nbconvert_config.py']):
-            assert not 'error' in self.call([IPYTHON, 'nbconvert']).lower()
+            self.call('nbconvert')
             assert os.path.isfile('notebook1.py')
             assert not os.path.isfile('notebook2.py')
 
@@ -142,8 +137,9 b' class TestNbConvertApp(TestsBase):'
         """
         Can the default config be overriden?
         """
-        with self.create_temp_cwd(['notebook*.ipynb', 'ipython_nbconvert_config.py', 
+        with self.create_temp_cwd(['notebook*.ipynb',
+                                   'ipython_nbconvert_config.py',
                                    'override.py']):
-            assert not 'error' in self.call([IPYTHON, 'nbconvert', '--config="override.py"']).lower()
+            self.call('nbconvert --config="override.py"')
             assert not os.path.isfile('notebook1.py')
             assert os.path.isfile('notebook2.py')

IPython/testing/tools.py

@@ -19,7 +19,6 b' from __future__ import absolute_import'
 #-----------------------------------------------------------------------------
 
 import os
-import pipes
 import re
 import sys
 import tempfile
@@ -38,7 +37,6 b' except ImportError:'
     has_nose = False
 
 from IPython.config.loader import Config
-from IPython.utils.process import getoutputerror
 from IPython.utils.text import list_strings
 from IPython.utils.io import temp_pyfile, Tee
 from IPython.utils import py3compat
@@ -156,6 +154,28 b' def default_config():'
     return config
 
 
+def get_ipython_cmd(as_string=False):
+    """
+    Return appropriate IPython command line name. By default, this will return
+    a list that can be used with subprocess.Popen, for example, but passing
+    `as_string=True` allows for returning the IPython command as a string.
+
+    Parameters
+    ----------
+    as_string: bool
+        Flag to allow to return the command as a string.
+    """
+    # FIXME: remove workaround for 2.6 support
+    if sys.version_info[:2] > (2,6):
+        ipython_cmd = [sys.executable, "-m", "IPython"]
+    else:
+        ipython_cmd = ["ipython"]
+
+    if as_string:
+        ipython_cmd = " ".join(ipython_cmd)
+
+    return ipython_cmd
+
 def ipexec(fname, options=None):
     """Utility to call 'ipython filename'.
 
@@ -186,14 +206,9 b' def ipexec(fname, options=None):'
     ]
     cmdargs = default_argv() + prompt_opts + options
 
-    _ip = get_ipython()
     test_dir = os.path.dirname(__file__)
-    
-    # FIXME: remove workaround for 2.6 support
-    if sys.version_info[:2] > (2,6):
-        ipython_cmd = [sys.executable, "-m", "IPython"]
-    else:
-        ipython_cmd = ["ipython"]
+
+    ipython_cmd = get_ipython_cmd()
     # Absolute path for filename
     full_fname = os.path.join(test_dir, fname)
     full_cmd = ipython_cmd + cmdargs + [full_fname]

IPython/utils/_process_common.py

@@ -139,13 +139,31 b' def getoutputerror(cmd):'
     stdout : str
     stderr : str
     """
+    return get_output_error_code(cmd)[:2]
 
-    out_err = process_handler(cmd, lambda p: p.communicate())
+def get_output_error_code(cmd):
+    """Return (standard output, standard error, return code) of executing cmd
+    in a shell.
+
+    Accepts the same arguments as os.system().
+
+    Parameters
+    ----------
+    cmd : str
+      A command to be executed in the system shell.
+
+    Returns
+    -------
+    stdout : str
+    stderr : str
+    returncode: int
+    """
+
+    out_err, p = process_handler(cmd, lambda p: (p.communicate(), p))
     if out_err is None:
-        return '', ''
+        return '', '', p.returncode
     out, err = out_err
-    return py3compat.bytes_to_str(out), py3compat.bytes_to_str(err)
-
+    return py3compat.bytes_to_str(out), py3compat.bytes_to_str(err), p.returncode
 
 def arg_split(s, posix=False, strict=True):
     """Split a command line's arguments in a shell-like manner.

IPython/utils/process.py

@@ -27,7 +27,7 b' else:'
     from ._process_posix import _find_cmd, system, getoutput, arg_split
 
 
-from ._process_common import getoutputerror
+from ._process_common import getoutputerror, get_output_error_code
 
 #-----------------------------------------------------------------------------
 # Code

IPython/utils/tempdir.py

@@ -104,3 +104,29 b' class NamedFileInTemporaryDirectory(object):'
 
     def __exit__(self, type, value, traceback):
         self.cleanup()
+
+
+class TemporaryWorkingDirectory(TemporaryDirectory):
+    """
+    Creates a temporary directory and sets the cwd to that directory.
+    Automatically reverts to previous cwd upon cleanup.
+    Usage example:
+
+        with TemporaryWorakingDirectory() as tmpdir:
+            ...
+    """
+
+    def __init__(self, **kw):
+        super(TemporaryWorkingDirectory, self).__init__(**kw)
+
+        #Change cwd to new temp dir.  Remember old cwd.
+        self.old_wd = _os.getcwd()
+        _os.chdir(self.name)
+
+
+    def cleanup(self):
+        #Revert to old cwd.
+        _os.chdir(self.old_wd)
+
+        #Cleanup
+        super(TemporaryWorkingDirectory, self).cleanup()

IPython/utils/tests/test_process.py

@@ -21,7 +21,8 b' from unittest import TestCase'
 import nose.tools as nt
 
 from IPython.utils.process import (find_cmd, FindCmdError, arg_split,
-                                   system, getoutput, getoutputerror)
+                                   system, getoutput, getoutputerror,
+                                   get_output_error_code)
 from IPython.testing import decorators as dec
 from IPython.testing import tools as tt
 
@@ -132,3 +133,14 b' class SubProcessTestCase(TestCase, tt.TempFileMixin):'
         out, err = getoutputerror('%s "%s"' % (python, self.fname))
         self.assertEqual(out, 'on stdout')
         self.assertEqual(err, 'on stderr')
+    
+    def test_get_output_error_code(self):
+        quiet_exit = '%s -c "import sys; sys.exit(1)"' % python
+        out, err, code = get_output_error_code(quiet_exit)
+        self.assertEqual(out, '')
+        self.assertEqual(err, '')
+        self.assertEqual(code, 1)
+        out, err, code = get_output_error_code('%s "%s"' % (python, self.fname))
+        self.assertEqual(out, 'on stdout')
+        self.assertEqual(err, 'on stderr')
+        self.assertEqual(code, 0)

IPython/utils/tests/test_tempdir.py

@@ -8,6 +8,7 b''
 import os
 
 from IPython.utils.tempdir import NamedFileInTemporaryDirectory
+from IPython.utils.tempdir import TemporaryWorkingDirectory
 
 
 def test_named_file_in_temporary_directory():
@@ -18,3 +19,10 b' def test_named_file_in_temporary_directory():'
         file.write(b'test')
     assert file.closed
     assert not os.path.exists(name)
+
+def test_temporary_working_directory():
+    with TemporaryWorkingDirectory() as dir:
+        assert os.path.exists(dir)
+        assert os.path.abspath(os.curdir) == dir
+    assert not os.path.exists(dir)
+    assert os.path.abspath(os.curdir) != dir