diff --git a/js/src/widget.ts b/js/src/widget.ts index a4d8104..62222f8 100644 --- a/js/src/widget.ts +++ b/js/src/widget.ts @@ -273,7 +273,7 @@ export class CelltestsWidget extends Widget { let tests = this.currentActiveCell.model.metadata.get("tests") as string[]; let s = ""; if (tests === undefined || tests.length === 0) { - tests = ["# Use %cell to execute the cell\n"]; + tests = ["# Use %cell to mark where the cell should be inserted, or add a line comment \"# no %cell\" to deliberately skip the cell\n", "%cell\n"]; } // eslint-disable-next-line @typescript-eslint/prefer-for-of for (let i = 0; i < tests.length; i++) { diff --git a/nbcelltests/shared.py b/nbcelltests/shared.py index 2a75424..5a1af3b 100644 --- a/nbcelltests/shared.py +++ b/nbcelltests/shared.py @@ -112,7 +112,7 @@ def extract_extrametadata(notebook, override=None, noqa_regex=None): base['noqa'] = set() for c in notebook.cells: - if c['cell_type'] != 'code' or is_empty(c['source']): + if c['cell_type'] != 'code' or empty_ast(c['source']): continue base['cell_lines'].append(0) @@ -124,7 +124,7 @@ def extract_extrametadata(notebook, override=None, noqa_regex=None): noqa_match = noqa_regex.match(line) if noqa_match: base['noqa'].add(noqa_match.group(1)) - if not is_empty(line): + if not empty_ast(line): base['lines'] += 1 base['cell_lines'][-1] += 1 if cell_injected_into_test(get_test(c)): @@ -142,7 +142,20 @@ def get_test(cell): return lines2source(cell.get('metadata', {}).get('tests', [])) -def is_empty(source): +def empty_ast(source): + """ + Whether the supplied source string has an empty ast. + + >>> empty_ast(" ") + True + + >>> empty_ast("pass") + False + + >>> empty_ast("# hello") + True + + """ try: parsed = ast.parse(source) except SyntaxError: @@ -156,7 +169,25 @@ def is_empty(source): return len(parsed.body) == 0 +def only_whitespace(source): + """ + Whether the supplied source string contains only whitespace. + + >>> only_whitespace(" ") + True + + >>> only_whitespace("pass") + False + + >>> only_whitespace("# hello") + False + """ + return len(source.strip()) == 0 + + +# TODO drop the "token" part CELL_INJ_TOKEN = r"%cell" +CELL_SKIP_TOKEN = r"# no %cell" def source2lines(source): @@ -181,10 +212,23 @@ def get_cell_inj_span(test_line): def cell_injected_into_test(test_source): + """ + Return True if the corresponding cell is injected into the test, + False if the cell is deliberately not injected, or None if there + is no deliberate command either way. + """ + inject = False + run = None for test_line in source2lines(test_source): - if get_cell_inj_span(test_line) is not None: - return True - return False + if inject is False and get_cell_inj_span(test_line) is not None: + inject = True + elif run is None and test_line.strip().startswith(CELL_SKIP_TOKEN): + run = False + + if run is False and inject: + raise ValueError("'%s' and '%s' are mutually exclusive but both were supplied:\n%s" % (CELL_SKIP_TOKEN, CELL_INJ_TOKEN, test_source)) + + return inject or run def get_coverage(metadata): diff --git a/nbcelltests/tests/_broken_magics.ipynb b/nbcelltests/tests/_broken_magics.ipynb new file mode 100644 index 0000000..19029f1 --- /dev/null +++ b/nbcelltests/tests/_broken_magics.ipynb @@ -0,0 +1,39 @@ +{ + "cells": [ + { + "cell_type": "code", + "execution_count": 5, + "metadata": {}, + "outputs": [], + "source": [ + "%magics2\n", + "raise\n", + "class X: pass" + ] + } + ], + "metadata": { + "celltests": { + "cell_coverage": 50 + }, + "kernelspec": { + "display_name": "Python 3", + "language": "python", + "name": "python3" + }, + "language_info": { + "codemirror_mode": { + "name": "ipython", + "version": 3 + }, + "file_extension": ".py", + "mimetype": "text/x-python", + "name": "python", + "nbconvert_exporter": "python", + "pygments_lexer": "ipython3", + "version": "3.7.6" + } + }, + "nbformat": 4, + "nbformat_minor": 4 +} diff --git a/nbcelltests/tests/_cell_counting.ipynb b/nbcelltests/tests/_cell_counting.ipynb index f485a4d..fd6c440 100644 --- a/nbcelltests/tests/_cell_counting.ipynb +++ b/nbcelltests/tests/_cell_counting.ipynb @@ -4,14 +4,10 @@ "cell_type": "code", "execution_count": 1, "metadata": { - "tests": [ - "assert x == 1" - ] + "tests": [] }, "outputs": [], - "source": [ - "x = 1" - ] + "source": [] }, { "cell_type": "markdown", diff --git a/nbcelltests/tests/_cell_coverage.ipynb b/nbcelltests/tests/_cell_coverage.ipynb index 64dc28f..4d1fd6a 100644 --- a/nbcelltests/tests/_cell_coverage.ipynb +++ b/nbcelltests/tests/_cell_coverage.ipynb @@ -3,12 +3,7 @@ { "cell_type": "code", "execution_count": 1, - "metadata": { - "tests": [ - "assert x == 1\n", - "class X: pass" - ] - }, + "metadata": {}, "outputs": [], "source": [ "# nothing" @@ -51,8 +46,10 @@ "execution_count": 3, "metadata": { "tests": [ - "assert x == 3\n", - "%magics1" + "x = 4\n", + "assert x == 4\n", + "%dirs\n", + "# no %cell" ] }, "outputs": [], @@ -77,7 +74,7 @@ "metadata": {}, "outputs": [], "source": [ - "%magics2\n", + "%dirs\n", "class X: pass" ] } diff --git a/nbcelltests/tests/_cell_not_injected_or_mocked.ipynb b/nbcelltests/tests/_cell_not_injected_or_mocked.ipynb new file mode 100644 index 0000000..d10c42b --- /dev/null +++ b/nbcelltests/tests/_cell_not_injected_or_mocked.ipynb @@ -0,0 +1,82 @@ +{ + "cells": [ + { + "cell_type": "code", + "execution_count": 1, + "metadata": { + "tests": [ + "# no %cell" + ] + }, + "outputs": [], + "source": [ + "x = 1\n" + ] + }, + { + "cell_type": "code", + "execution_count": 2, + "metadata": {}, + "outputs": [], + "source": [ + "# nothing to see here" + ] + }, + { + "cell_type": "code", + "execution_count": 3, + "metadata": {}, + "outputs": [], + "source": [ + "x = 1" + ] + }, + { + "cell_type": "code", + "execution_count": 4, + "metadata": { + "tests": [ + "%cell" + ] + }, + "outputs": [], + "source": [ + "x = 1" + ] + }, + { + "cell_type": "code", + "execution_count": 5, + "metadata": { + "tests": [ + "x = 17" + ] + }, + "outputs": [], + "source": [ + "x = 1" + ] + } + ], + "metadata": { + "kernelspec": { + "display_name": "Python 3", + "language": "python", + "name": "python3" + }, + "language_info": { + "codemirror_mode": { + "name": "ipython", + "version": 3 + }, + "file_extension": ".py", + "mimetype": "text/x-python", + "name": "python", + "nbconvert_exporter": "python", + "pygments_lexer": "ipython3", + "version": "3.7.6" + } + }, + "nbformat": 4, + "nbformat_minor": 4 +} diff --git a/nbcelltests/tests/_cumulative_run.ipynb b/nbcelltests/tests/_cumulative_run.ipynb index 7a40117..8030575 100644 --- a/nbcelltests/tests/_cumulative_run.ipynb +++ b/nbcelltests/tests/_cumulative_run.ipynb @@ -5,7 +5,7 @@ "execution_count": 1, "metadata": { "tests": [ - "pass" + "# no %cell" ] }, "outputs": [], @@ -68,7 +68,46 @@ "source": [ "pass" ] - } + }, + { + "cell_type": "code", + "execution_count": 6, + "metadata": { + "tests": [ + "\n" + ] + }, + "outputs": [], + "source": [ + "y = 10" + ] + }, + { + "cell_type": "code", + "execution_count": 7, + "metadata": { + "tests": [ + "# no %cell" + ] + }, + "outputs": [], + "source": [ + "a = 1" + ] + }, + { + "cell_type": "code", + "execution_count": 8, + "metadata": { + "tests": [ + "%cell\n" + ] + }, + "outputs": [], + "source": [ + "z = 1" + ] + } ], "metadata": { "kernelspec": { diff --git a/nbcelltests/tests/_empty_cell_with_test.ipynb b/nbcelltests/tests/_empty_cell_with_test.ipynb new file mode 100644 index 0000000..0602c1e --- /dev/null +++ b/nbcelltests/tests/_empty_cell_with_test.ipynb @@ -0,0 +1,47 @@ +{ + "cells": [ + { + "cell_type": "code", + "execution_count": 1, + "source": [], + "outputs": [], + "metadata": { + "tests": [ + "# empty test" + ] + } + }, + { + "cell_type": "code", + "execution_count": 2, + "source": [], + "outputs": [], + "metadata": { + "tests": [ + "pass # not empty test" + ] + } + } + ], + "metadata": { + "kernelspec": { + "display_name": "Python 3.7.6 64-bit ('celltestsui': conda)", + "language": "python", + "name": "python37664bitcelltestsuiconda549e14be7f784a2fbde278c18c8be829" + }, + "language_info": { + "codemirror_mode": { + "name": "ipython", + "version": 3 + }, + "file_extension": ".py", + "mimetype": "text/x-python", + "name": "python", + "nbconvert_exporter": "python", + "pygments_lexer": "ipython3", + "version": "3.7.6" + } + }, + "nbformat": 4, + "nbformat_minor": 4 +} diff --git a/nbcelltests/tests/_emptyast_cell_with_test.ipynb b/nbcelltests/tests/_emptyast_cell_with_test.ipynb new file mode 100644 index 0000000..318c955 --- /dev/null +++ b/nbcelltests/tests/_emptyast_cell_with_test.ipynb @@ -0,0 +1,49 @@ +{ + "cells": [ + { + "cell_type": "code", + "execution_count": 1, + "source": [], + "outputs": [], + "metadata": { + "tests": [ + "# empty test" + ] + } + }, + { + "cell_type": "code", + "execution_count": 2, + "source": [ + "# nothing to see here" + ], + "outputs": [], + "metadata": { + "tests": [ + "pass # not empty test" + ] + } + } + ], + "metadata": { + "kernelspec": { + "display_name": "Python 3.7.6 64-bit ('celltestsui': conda)", + "language": "python", + "name": "python37664bitcelltestsuiconda549e14be7f784a2fbde278c18c8be829" + }, + "language_info": { + "codemirror_mode": { + "name": "ipython", + "version": 3 + }, + "file_extension": ".py", + "mimetype": "text/x-python", + "name": "python", + "nbconvert_exporter": "python", + "pygments_lexer": "ipython3", + "version": "3.7.6" + } + }, + "nbformat": 4, + "nbformat_minor": 4 +} diff --git a/nbcelltests/tests/_skips.ipynb b/nbcelltests/tests/_skips.ipynb index 9dbf238..5d91e4f 100644 --- a/nbcelltests/tests/_skips.ipynb +++ b/nbcelltests/tests/_skips.ipynb @@ -5,7 +5,7 @@ "execution_count": 1, "metadata": { "tests": [ - "pass" + "\n" ] }, "outputs": [], @@ -26,44 +26,32 @@ "metadata": {}, "outputs": [], "source": [ - "x = 1" + "x = 3" ] }, { "cell_type": "code", "execution_count": 4, - "metadata": { - "tests": [ - "# nothing to see here" - ] - }, - "outputs": [], - "source": [ - "x = 1" - ] - }, - { - "cell_type": "code", - "execution_count": 5, "metadata": { "tests": [] }, "outputs": [], "source": [ - "x = 1" + "x = 4" ] }, { "cell_type": "code", - "execution_count": 6, + "execution_count": 5, "metadata": { "tests": [ - "x = 2" + "# no %cell\n", + "x = 6" ] }, "outputs": [], "source": [ - "x = 1" + "x = 5" ] } ], diff --git a/nbcelltests/tests/test_shared.py b/nbcelltests/tests/test_shared.py index ad08dca..bbad1a5 100644 --- a/nbcelltests/tests/test_shared.py +++ b/nbcelltests/tests/test_shared.py @@ -10,7 +10,7 @@ import pytest import nbformat -from nbcelltests.shared import extract_extrametadata, get_coverage, is_empty +from nbcelltests.shared import extract_extrametadata, get_coverage, empty_ast, only_whitespace, cell_injected_into_test, get_cell_inj_span # TODO: should generate these BASIC_NB = os.path.join(os.path.dirname(__file__), 'basic.ipynb') @@ -19,14 +19,51 @@ COVERAGE_NB = os.path.join(os.path.dirname(__file__), '_cell_coverage.ipynb') LINT_DISABLE_NB = os.path.join(os.path.dirname(__file__), '_lint_disable.ipynb') +# TODO should parameterize test_empty_ast _whitespace -def test_is_empty(): - assert is_empty("import blah\nblah.do_something()") is False - assert is_empty("%matplotlib inline") is False - assert is_empty("pass") is False - assert is_empty("") is True - assert is_empty("#pass") is True - assert is_empty("\n\n\n") is True +# empty ast, empty whitespace +_empty_empty = [ + "", + " ", + "\t", + "\n", + "\r\n" +] + +# empty ast, non-empty whitespace +_empty_nonempty = [ + "#pass", +] + + +# non-empty ast, non-empty whitespace +_nonempty_nonempty = [ + "import blah\nblah.do_something()", + "%matplotlib inline", + "pass" +] + + +def test_empty_ast(): + for x in _empty_empty: + assert empty_ast(x) is True + + for x in _empty_nonempty: + assert empty_ast(x) is True + + for x in _nonempty_nonempty: + assert empty_ast(x) is False + + +def test_only_whitespace(): + for x in _empty_empty: + assert only_whitespace(x) is True + + for x in _empty_nonempty: + assert only_whitespace(x) is False + + for x in _nonempty_nonempty: + assert only_whitespace(x) is False def _metadata(nb, what): @@ -92,7 +129,7 @@ def test_extract_extrametadata_cell_lines_noncode(): def test_extract_extrametadata_magics_noncode(): - assert _metadata(COVERAGE_NB, 'magics') == set(['magics2']) + assert _metadata(COVERAGE_NB, 'magics') == set(['dirs']) # coverage @@ -166,3 +203,84 @@ def test_extract_extrametadata_disable_bad_regex(): assert e.args[0] == "noqa_regex must contain one capture group (specifying the rule)" else: assert False, "should have raised a ValueError" + + +@pytest.mark.parametrize( + "test_line, expected", [ + (r"%cell", (0, 5)), + (r"%cell;x", (0, 5)), + pytest.param(r"%celll", None, marks=pytest.mark.xfail(reason="need to decide rules/really treat as token")), + (r"", None), + (r" %cell", (4, 9)), + (r"# no %cell", None), + (r"# %cell", None) + ] +) +def test_get_cell_inj_span(test_line, expected): + assert get_cell_inj_span(test_line) == expected + + +# TODO could clean up with textwrap dedent etc +@pytest.mark.parametrize( + "test_source, expected, exception", [ + (r"""\ +%cell +""", True, None), + + (r"""\ +%cell +%cell +""", True, None), + + (r"""\ +%cell;x +""", True, None), + + pytest.param(r"""\ +%celll +""", None, None, marks=pytest.mark.xfail(reason="need to decide rules/really treat as token")), + + (r"""\ +""", None, None), + + (r"""\ +%cell l +""", True, None), + + (r"""\ +x = 1 +%cell +""", True, None), + + (r"""\ +x = 1 +if x==1: + %cell +""", True, None), + + (r"""\ +# no %cell +""", False, None), + + (r"""\ +x = 1 +if x==1: + pass + # no %cell +""", False, None), + + (r"""\ +%cell # no %cell +""", True, None), + + (r"""\ +%cell +# no %cell +""", NotImplemented, (ValueError, "mutually exclusive")), + ]) +def test_cell_injected_into_test(test_source, expected, exception): + if exception is None: + assert cell_injected_into_test(test_source) is expected + else: + with pytest.raises(exception[0], match=exception[1]): + cell_injected_into_test(test_source) diff --git a/nbcelltests/tests/test_test.py b/nbcelltests/tests/test_test.py index b5eadb3..c5569c8 100644 --- a/nbcelltests/tests/test_test.py +++ b/nbcelltests/tests/test_test.py @@ -11,19 +11,35 @@ import sys import unittest +import pytest + from nbcelltests.test import run, runWithReturn, runWithReport, runWithHTMLReturn -# TODO: we should generate the notebooks rather than having them as -# files (same for lint ones). Would also allow for simplification of -# test class hierarchy. +# Some straightforward TODOs: +# +# - We should generate the notebooks rather than having them as files +# (same for lint ones). Would also allow for simplification of test +# class hierarchy. +# +# - Should use assert(Regex)Raises rather than manual try/catch +# +# - Some classes are abstract but not declared as such + + +# TODO: This test file's manual use of unittest is brittle + CUMULATIVE_RUN = os.path.join(os.path.dirname(__file__), '_cumulative_run.ipynb') CELL_ERROR = os.path.join(os.path.dirname(__file__), '_cell_error.ipynb') TEST_ERROR = os.path.join(os.path.dirname(__file__), '_test_error.ipynb') TEST_FAIL = os.path.join(os.path.dirname(__file__), '_test_fail.ipynb') COUNTING = os.path.join(os.path.dirname(__file__), '_cell_counting.ipynb') NONCODE = os.path.join(os.path.dirname(__file__), '_non_code_cell.ipynb') +EMPTY_CELL_WITH_TEST = os.path.join(os.path.dirname(__file__), '_empty_cell_with_test.ipynb') +EMPTYAST_CELL_WITH_TEST = os.path.join(os.path.dirname(__file__), '_emptyast_cell_with_test.ipynb') SKIPS = os.path.join(os.path.dirname(__file__), '_skips.ipynb') COVERAGE = os.path.join(os.path.dirname(__file__), '_cell_coverage.ipynb') +CELL_NOT_INJECTED_OR_MOCKED = os.path.join(os.path.dirname(__file__), '_cell_not_injected_or_mocked.ipynb') +BROKEN_MAGICS = os.path.join(os.path.dirname(__file__), '_broken_magics.ipynb') INPUT_CELL_MULTILINE_STRING = os.path.join(os.path.dirname(__file__), '_input_cell_multiline_string.ipynb') INPUT_TEST_MULTILINE_STRING = os.path.join(os.path.dirname(__file__), '_input_test_multiline_string.ipynb') @@ -36,20 +52,18 @@ FORKED = '--forked' in sys.argv -def _assert_x_undefined(t): +def _assert_undefined(t, name='x'): """ Convenience method to assert that x is not already defined in the kernel. """ t._run(""" try: - x + %s except NameError: pass else: - raise Exception('x was already defined') - """) - -# TODO: This test file's manual use of unittest is brittle + raise Exception('%s was already defined') + """ % (name, name)) def _import_from_path(path, module_name): @@ -59,8 +73,6 @@ def _import_from_path(path, module_name): See e.g. https://stackoverflow.com/a/67692. """ - # TODO: need to test over multiple python versions - # (https://github.com/jpmorganchase/nbcelltests/issues/106) import importlib.util spec = importlib.util.spec_from_file_location(module_name, path) mod = importlib.util.module_from_spec(spec) @@ -98,14 +110,6 @@ def setUpClass(cls): assert hasattr(cls, "NBNAME"), "Subclasses must have NBNAME attribute." # TODO: make actually abstract cls.generated_tests = _generate_test_module(notebook=cls.NBNAME, module_name="nbcelltests.tests.%s.%s" % (__name__, cls.__name__)) - def _assert_skipped(self, mthd, reason): - try: - mthd() - except unittest.case.SkipTest as e: - assert e.args[0] == reason - else: - raise ValueError("Should have skipped with reason '%s'" % reason) - def test_coverage(self): """ Subclasses should override this if they want to check coverage. @@ -116,8 +120,6 @@ def test_coverage(self): class TestMethodGenerationError(_TestCellTests): """Tests of things that should fail during test script generation""" - NBNAME = NONCODE - @classmethod def setUpClass(cls): # we're testing what happens in setUpClass @@ -125,15 +127,39 @@ def setUpClass(cls): def test_non_code_cell_with_test_causes_error(self): try: - _generate_test_module(self.NBNAME, "module.name.irrelevant") + _generate_test_module(NONCODE, "module.name.irrelevant") except ValueError as e: assert e.args[0] == 'Cell 1 is not a code cell, but metadata contains test code!' else: raise Exception("Test script should fail to generate") + def test_onlywhitespace_code_cell_with_test_causes_error(self): + try: + _generate_test_module(EMPTY_CELL_WITH_TEST, "module.name.irrelevant") + except ValueError as e: + assert e.args[0] == 'Code cell 2 is empty, but test contains code.' + else: + raise Exception("Test script should fail to generate") + + def test_emptyast_code_cell_with_test_causes_error(self): + try: + _generate_test_module(EMPTYAST_CELL_WITH_TEST, "module.name.irrelevant") + except ValueError as e: + assert e.args[0] == 'Code cell 2 is empty, but test contains code.' + else: + raise Exception("Test script should fail to generate") + + def test_code_not_injected_or_mocked(self): + try: + _generate_test_module(CELL_NOT_INJECTED_OR_MOCKED, "module.name.irrelevant") + except ValueError as e: + assert e.args[0].startswith('Test 5: cell code not injected into test') + else: + raise Exception("Test script should fail to generate") + class TestSkips(_TestCellTests): - """Tests that various conditions result in skipped tests""" + """Tests that various conditions result in test methods being generated or not as expected.""" NBNAME = SKIPS @@ -151,28 +177,27 @@ def tearDown(self): self.t.tearDownClass() def test_skip_completely_empty_code_cell(self): - """Test+cell where cell has nothing at all in should be skipped""" - self._assert_skipped(self.t.test_code_cell_1, "empty code cell") + """Empty cell and test""" + assert not hasattr(self.t, "test_code_cell_1") def test_skip_no_code_code_cell(self): - """Test+cell where cell had e.g. just a comment in should be skipped""" - self._assert_skipped(self.t.test_code_cell_2, "empty code cell") + """Cell's just a comment, and no test.""" + assert not hasattr(self.t, "test_code_cell_2") - def test_skip_no_test_field_in_metadata(self): - """Cell with no test metadata should be skipped""" - self._assert_skipped(self.t.test_code_cell_3, "no test supplied") - - def test_skip_no_code_in_test(self): - """Test+cell where test is just e.g. a comment should be skipped""" - self._assert_skipped(self.t.test_code_cell_4, "no test supplied") + def test_noskip_with_no_test_field_in_metadata(self): + """Test where cell has no test metadata is just cell""" + self.t.test_code_cell_3() + self.t._run("assert x == 3") - def test_skip_completely_empty_test(self): - """Cell where test is completely empty should be skipped""" - self._assert_skipped(self.t.test_code_cell_5, "no test supplied") + def test_noskip_with_completely_empty_test(self): + """Test is just %cell where test is completely empty""" + self.t.test_code_cell_4() + self.t._run("assert x == 4") - def test_skip_if_no_cell_injection(self): - """Test+cell where test does not inject cell should be skipped""" - self._assert_skipped(self.t.test_code_cell_6, "cell code not injected into test") + def test_noskip_with_deliberate_no_cell_injection(self): + """Runs test but not cell""" + self.t.test_code_cell_5() + self.t._run("assert x == 6") # value from test, not cell class TestCumulativeRun(_TestCellTests): @@ -212,12 +237,12 @@ def test_state(self): t.setUpClass() # check cell did not run - # (no %cell in test) + # (deliberate no cell in test, no code in test) + # TODO: should this be no method, as there's only empty ast in the test t.setUp() - _assert_x_undefined(t) - self._assert_skipped(t.test_code_cell_1, "cell code not injected into test") - # t.test_code_cell_1() - # _assert_x_undefined(t) + _assert_undefined(t, 'x') + t.test_code_cell_1() + _assert_undefined(t, 'x') t.tearDown() if FORKED: t.tearDownClass() @@ -268,6 +293,43 @@ def test_state(self): assert x == 3, x """) t.tearDown() + if FORKED: + t.tearDownClass() + + # test defaults to %cell + if FORKED: + t.setUpClass() + t.setUp() + t.test_code_cell_6() + t._run(""" + assert y == 10 + """) + t.tearDown() + if FORKED: + t.tearDownClass() + + # deliberate no %cell; cell 8 will check it's also not subsequently run + # i.e. a will never be defined + if FORKED: + t.setUpClass() + t.setUp() + t.test_code_cell_7() + _assert_undefined(t, 'a') + t.tearDown() + if FORKED: + t.tearDownClass() + + # check cell 7 above did not run + if FORKED: + t.setUpClass() + t.setUp() + t.test_code_cell_8() + t._run(""" + assert z == 1 + assert y == 10 + """) + _assert_undefined(t, 'a') + t.tearDown() t.tearDownClass() @@ -317,7 +379,7 @@ def test_exception_in_test_is_detected(self): t.setUpClass() t.setUp() - _assert_x_undefined(t) + _assert_undefined(t, 'x') # test should error out try: @@ -371,6 +433,30 @@ def test_failure_is_detected(self): t.tearDownClass() +# TODO: see https://github.com/computationalmodelling/nbval/issues/147 +class TestSomeSanity(_TestCellTests): + + NBNAME = BROKEN_MAGICS + + @pytest.mark.xfail(reason="error messages on shell channel are ignored by nbval") + def test_bad_magic_does_let_everything_pass(self): + t = self.generated_tests.TestNotebook() + + t.setUpClass() + t.setUp() + + # cell should error out + try: + t.test_code_cell_1() + except Exception as e: + assert e.args[0].startswith("UsageError: Line magic function `%magics2` not found.") + else: + raise Exception("Cell should have errored out") + finally: + t.tearDown() + t.tearDownClass() + + class TestCellCounting(_TestCellTests): """Check that various things don't throw off cell+test correspondence.""" @@ -391,10 +477,10 @@ def tearDown(self): def test_skips(self): """ - There's a skipped test at the start of the notebook to make sure skips don't + There's an empty cell at the start of the notebook to make sure skips don't affect test/cell correspondence. """ - self._assert_skipped(self.t.test_code_cell_1, "cell code not injected into test") + assert not hasattr(self.t, "test_code_cell_1") def test_still_ok_after_markdown(self): """Correspondence still ok after markdown cell?""" @@ -407,7 +493,7 @@ def test_still_ok_after_raw(self): def test_count(self): """No unexpected extra test methods""" test_methods = [mthd for mthd in dir(self.t) if mthd.startswith("test_code_cell")] - self.assertListEqual(sorted(test_methods), ['test_code_cell_1', 'test_code_cell_2', 'test_code_cell_3']) + self.assertListEqual(sorted(test_methods), ['test_code_cell_2', 'test_code_cell_3']) class TestCellCoverage(_TestCellTests): @@ -649,18 +735,17 @@ def _check(html, coverage_result): tests_ran = False for p in html_soup.find_all("p"): # 1 cell test plus coverage test - if p.text.startswith("2 tests ran in"): + if p.text.startswith("5 tests ran in"): tests_ran = True break assert tests_ran expected_results = { - "test_code_cell_1": "Skipped", "test_code_cell_2": "Passed", - "test_code_cell_3": "Skipped", - "test_code_cell_4": "Skipped", - "test_code_cell_5": "Skipped", + "test_code_cell_3": "Passed", + "test_code_cell_4": "Passed", + "test_code_cell_5": "Passed", "test_cell_coverage": coverage_result, } diff --git a/nbcelltests/tests_vendored.py b/nbcelltests/tests_vendored.py index 61e2903..ed9d744 100644 --- a/nbcelltests/tests_vendored.py +++ b/nbcelltests/tests_vendored.py @@ -51,13 +51,13 @@ from Queue import Empty except ImportError: from queue import Empty - +import logging import unittest import nbformat from nbval.kernel import RunningKernel -from nbcelltests.shared import is_empty, cell_injected_into_test, source2lines, lines2source, get_cell_inj_span, get_test +from nbcelltests.shared import empty_ast, cell_injected_into_test, source2lines, lines2source, get_cell_inj_span, get_test, only_whitespace, CELL_SKIP_TOKEN, CELL_INJ_TOKEN def get_kernel(path_to_notebook): @@ -118,44 +118,51 @@ def _inject_cell_into_test(cell_source, test_source): def get_celltests(path_to_notebook): """ - Return a dictionary of {code cell number: celltest} for all the code - cells in the given notebook. + Return a dictionary of {code cell number: celltest_info} for all + non-empty code cells in the given notebook. + + celltest_info is a dictionary containing: - A celltest is a source code string of the test supplied for a - cell, plus the cell itself wherever (if) injected into the test - using %cell. + * 'source' string of the test supplied for a cell, plus the + cell itself wherever injected into the test using %cell. - (celltest is actually currently a dictionary of skip_reason (if any) - and the source code, but we're unsure if we want to keep the skipping - part. If keeping, we'll need to clean up and doc.) + * 'cell_injected' flag indicating whether the cell was injected + into the test """ notebook = nbformat.read(path_to_notebook, 4) celltests = {} code_cell = 0 for i, cell in enumerate(notebook.cells, start=1): test_source = lines2source(get_test(cell)) - empty_test = is_empty(_inject_cell_into_test("pass", test_source)) + test_ast_empty = empty_ast(_inject_cell_into_test("pass", test_source)) if cell.get('cell_type') != 'code': - if not empty_test: + if not test_ast_empty: raise ValueError("Cell %d is not a code cell, but metadata contains test code!" % i) continue code_cell += 1 - celltest = _inject_cell_into_test(cell['source'], test_source) - # TODO: we are unsure if we want to keep the skipping part! - # If keeping, clean up and document above. - skip_reason = None - if is_empty(cell['source']): - skip_reason = "empty code cell" - elif empty_test: - skip_reason = "no test supplied" - elif not cell_injected_into_test(test_source): - skip_reason = "cell code not injected into test" + if empty_ast(cell['source']): # TODO: maybe this should be only_whitespace? + if not test_ast_empty: + raise ValueError("Code cell %d is empty, but test contains code." % code_cell) + continue + + cell_injected = cell_injected_into_test(test_source) + + if only_whitespace(test_source): + celltest = cell['source'] + cell_injected = True + elif cell_injected is None: + raise ValueError( + r"Test {}: cell code not injected into test; either add '{}' to the test, or add '{}' to deliberately suppress cell execution".format( + code_cell, CELL_INJ_TOKEN, CELL_SKIP_TOKEN)) + elif cell_injected is False: + celltest = test_source + else: + celltest = _inject_cell_into_test(cell['source'], test_source) - celltests[code_cell] = {'skip_reason': skip_reason, - 'source': celltest if not skip_reason else ""} + celltests[code_cell] = {'source': celltest, 'cell_injected': cell_injected} return celltests @@ -194,16 +201,40 @@ class TestNotebookBase(unittest.TestCase): 1. test_code_cell_5: executes cells 1, 2, 3, 4, 5 (test passes) 2. test_code_cell_3: execute nothing (test passes) + + When does a cell execute? + ------------------------- + + A cell's test will execute its corresponding cell wherever %cell + appears at the start of a stripped line. + + If a cell does not have a test defined, or the test is only + whitespace, the cell will be executed (i.e. the test will be + assumed to be just %cell). + + If a cell's test contains a "# no %cell" comment line, the cell + will not be executed (though the test will). Use for mocking out a + whole cell (ideally it's better to avoid this and mock things the + cell uses, but that might not be possible). + + Code cells that have an empty ast are considered empty. An + exception will be raised if attempting to test empty code cells or + non-code cells. + + + Notes + ----- + This is an abstract class; subclasses will supply the source of - code cells and their associated tests in cells_and_tests, plus - test methods as entry points for test runners. + code cells and their associated tests in celltests, plus test + methods as entry points for test runners. + + 'cell' used in this class refers to cell number; 'cell content' + typically refers to code_cell+test (depending what is passed in). - Note: 'cell' used in this class refers to cell number; 'cell - content' typically refers to code_cell+test (depending what is - passed in). """ - # abstract - subclasses will define KERNEL_NAME (TODO: make - # actually abstract...) + # abstract - subclasses will define KERNEL_NAME and celltests + # (TODO: make actually abstract...) @classmethod def setUpClass(cls): @@ -222,13 +253,18 @@ def run_test(self, cell): Run any cells preceding cell (number) that have not already been run, then run cell itself. """ - if self.celltests[cell]['skip_reason']: - raise unittest.SkipTest(self.celltests[cell]['skip_reason']) - - preceding_cells = set(range(1, cell)) + preceding_cells = set(range(1, cell)) & self.celltests.keys() for preceding_cell in sorted(set(preceding_cells) - self.celltests_run): self._run_cell(preceding_cell) self._run_cell(cell) + if not self.celltests[cell]['cell_injected']: + # TODO: this will appear in the html report under the test + # method as captured logging, but it would be better + # reported as a warning by pytest. However, (a) pytest is + # already reporting various spurious warnings + # (e.g. traitlets deprecations), and (b) pytest warnings + # are not being reported in the html we show right now. + logging.warning("Cell %d was not executed as part of the cell test", cell) def _run_cell(self, cell): """Run cell and record its execution"""