From fdb8764af8ef087bf4f70bfed98091d92520cd70 2013-07-31 21:36:19 From: Min RK Date: 2013-07-31 21:36:19 Subject: [PATCH] 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. --- diff --git a/IPython/nbconvert/tests/base.py b/IPython/nbconvert/tests/base.py index 0955121..2926590 100644 --- a/IPython/nbconvert/tests/base.py +++ b/IPython/nbconvert/tests/base.py @@ -13,55 +13,25 @@ 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 @@ 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 @@ 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 diff --git a/IPython/nbconvert/tests/test_nbconvertapp.py b/IPython/nbconvert/tests/test_nbconvertapp.py index 355923e..bee17cf 100644 --- a/IPython/nbconvert/tests/test_nbconvertapp.py +++ b/IPython/nbconvert/tests/test_nbconvertapp.py @@ -16,7 +16,6 @@ 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 @@ 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 @@ 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 @@ 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 @@ 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 @@ 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 @@ 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 @@ 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 @@ 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 @@ 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') diff --git a/IPython/testing/tools.py b/IPython/testing/tools.py index c54db89..6b2cc55 100644 --- a/IPython/testing/tools.py +++ b/IPython/testing/tools.py @@ -19,7 +19,6 @@ from __future__ import absolute_import #----------------------------------------------------------------------------- import os -import pipes import re import sys import tempfile @@ -38,7 +37,6 @@ 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 @@ 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 @@ 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] diff --git a/IPython/utils/_process_common.py b/IPython/utils/_process_common.py index 34ca83b..926721c 100644 --- a/IPython/utils/_process_common.py +++ b/IPython/utils/_process_common.py @@ -139,13 +139,31 @@ 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. diff --git a/IPython/utils/process.py b/IPython/utils/process.py index 42c6bf5..b7f71c4 100644 --- a/IPython/utils/process.py +++ b/IPython/utils/process.py @@ -27,7 +27,7 @@ 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 diff --git a/IPython/utils/tempdir.py b/IPython/utils/tempdir.py index 364e119..714c70e 100644 --- a/IPython/utils/tempdir.py +++ b/IPython/utils/tempdir.py @@ -104,3 +104,29 @@ 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() diff --git a/IPython/utils/tests/test_process.py b/IPython/utils/tests/test_process.py index 6e7f282..364dcd2 100644 --- a/IPython/utils/tests/test_process.py +++ b/IPython/utils/tests/test_process.py @@ -21,7 +21,8 @@ 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 @@ 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) diff --git a/IPython/utils/tests/test_tempdir.py b/IPython/utils/tests/test_tempdir.py index 8e919e5..acb6a66 100644 --- a/IPython/utils/tests/test_tempdir.py +++ b/IPython/utils/tests/test_tempdir.py @@ -8,6 +8,7 @@ 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 @@ 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