From fb553585663bbc05692f685ae4ab3706ebc12ff8 Mon Sep 17 00:00:00 2001 From: Chris B Date: Tue, 9 Jun 2020 15:04:44 +0100 Subject: [PATCH 1/7] Celltest always generated for non-empty code cell, and always runs. Defaults to %cell if test is empty. --- js/src/widget.ts | 2 +- nbcelltests/shared.py | 56 +++++- nbcelltests/tests/_broken_magics.ipynb | 39 ++++ nbcelltests/tests/_cell_counting.ipynb | 8 +- nbcelltests/tests/_cell_coverage.ipynb | 15 +- .../tests/_cell_not_injected_or_mocked.ipynb | 82 ++++++++ nbcelltests/tests/_cumulative_run.ipynb | 43 ++++- nbcelltests/tests/_empty_cell_with_test.ipynb | 47 +++++ nbcelltests/tests/_skips.ipynb | 26 +-- nbcelltests/tests/test_shared.py | 55 +++++- nbcelltests/tests/test_test.py | 181 ++++++++++++------ nbcelltests/tests_vendored.py | 109 +++++++---- 12 files changed, 522 insertions(+), 141 deletions(-) create mode 100644 nbcelltests/tests/_broken_magics.ipynb create mode 100644 nbcelltests/tests/_cell_not_injected_or_mocked.ipynb create mode 100644 nbcelltests/tests/_empty_cell_with_test.ipynb 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..f4f7263 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_SPAN, 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/_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..cd636ea 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 # 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 diff --git a/nbcelltests/tests/test_test.py b/nbcelltests/tests/test_test.py index b5eadb3..bb185c0 100644 --- a/nbcelltests/tests/test_test.py +++ b/nbcelltests/tests/test_test.py @@ -13,17 +13,30 @@ 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') 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 +49,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 +70,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 +107,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 +117,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 +124,31 @@ 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_non_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_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 +166,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") - - 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") + """Cell's just a comment, and no test.""" + assert not hasattr(self.t, "test_code_cell_2") - 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 +226,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 +282,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 +368,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 +422,29 @@ def test_failure_is_detected(self): t.tearDownClass() +# TODO: this is an nbval bug +class TestSomeSanity(_TestCellTests): + + NBNAME = BROKEN_MAGICS + + 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 +465,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 +481,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 +723,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..83cacfb 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""" @@ -293,6 +329,9 @@ def _run(self, cell_content, description=''): elif msg_type in ('display_data', 'execute_result'): continue elif msg_type == 'stream': + # TODO: temporary/should not be here; needs fix in nbval + if msg['content']['name'] == 'stderr': + raise Exception(msg['content']['text']) continue # if the message type is an error then an error has occurred during From cf2d02d57e20ae51204acdeb73529c48bfdcff85 Mon Sep 17 00:00:00 2001 From: Chris B Date: Wed, 10 Jun 2020 10:49:04 +0100 Subject: [PATCH 2/7] make fix --- nbcelltests/tests_vendored.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nbcelltests/tests_vendored.py b/nbcelltests/tests_vendored.py index 83cacfb..f76fa24 100644 --- a/nbcelltests/tests_vendored.py +++ b/nbcelltests/tests_vendored.py @@ -143,7 +143,7 @@ def get_celltests(path_to_notebook): code_cell += 1 - if empty_ast(cell['source']): # TODO: maybe this should be only_whitespace? + 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 From dd9b38ccdf95a7b2f196d2d52c034a51d0b242f7 Mon Sep 17 00:00:00 2001 From: Chris B Date: Wed, 10 Jun 2020 13:00:22 +0100 Subject: [PATCH 3/7] Tests for %cell finding --- nbcelltests/tests/test_shared.py | 83 +++++++++++++++++++++++++++++++- 1 file changed, 82 insertions(+), 1 deletion(-) diff --git a/nbcelltests/tests/test_shared.py b/nbcelltests/tests/test_shared.py index cd636ea..a1cbe81 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, empty_ast, only_whitespace +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') @@ -203,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) From fff3cff084fab8878011e289440e726af80367cc Mon Sep 17 00:00:00 2001 From: Chris B Date: Wed, 10 Jun 2020 13:00:38 +0100 Subject: [PATCH 4/7] Fix typo in code --- nbcelltests/shared.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nbcelltests/shared.py b/nbcelltests/shared.py index f4f7263..5a1af3b 100644 --- a/nbcelltests/shared.py +++ b/nbcelltests/shared.py @@ -226,7 +226,7 @@ def cell_injected_into_test(test_source): 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_SPAN, test_source)) + 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 From b831a0316249a958b4f56253a5f337c5c9c13759 Mon Sep 17 00:00:00 2001 From: Chris B Date: Wed, 10 Jun 2020 13:01:37 +0100 Subject: [PATCH 5/7] make fix --- nbcelltests/tests/test_shared.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/nbcelltests/tests/test_shared.py b/nbcelltests/tests/test_shared.py index a1cbe81..bbad1a5 100644 --- a/nbcelltests/tests/test_shared.py +++ b/nbcelltests/tests/test_shared.py @@ -207,16 +207,16 @@ def test_extract_extrametadata_disable_bad_regex(): @pytest.mark.parametrize( "test_line, expected", [ - (r"%cell", (0,5)), - (r"%cell;x", (0,5)), + (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" %cell", (4, 9)), (r"# no %cell", None), (r"# %cell", None) ] ) -def test_get_cell_inj_span(test_line,expected): +def test_get_cell_inj_span(test_line, expected): assert get_cell_inj_span(test_line) == expected @@ -277,8 +277,8 @@ def test_get_cell_inj_span(test_line,expected): %cell # no %cell """, NotImplemented, (ValueError, "mutually exclusive")), -]) -def test_cell_injected_into_test(test_source,expected,exception): + ]) +def test_cell_injected_into_test(test_source, expected, exception): if exception is None: assert cell_injected_into_test(test_source) is expected else: From e4a869bbbeb9c74d8ec043d356757e22a90b32bb Mon Sep 17 00:00:00 2001 From: Chris B Date: Wed, 10 Jun 2020 13:22:29 +0100 Subject: [PATCH 6/7] Make empty whitespace/ast explicit in test. --- .../tests/_emptyast_cell_with_test.ipynb | 49 +++++++++++++++++++ nbcelltests/tests/test_test.py | 11 ++++- 2 files changed, 59 insertions(+), 1 deletion(-) create mode 100644 nbcelltests/tests/_emptyast_cell_with_test.ipynb 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/test_test.py b/nbcelltests/tests/test_test.py index bb185c0..ca2c8cc 100644 --- a/nbcelltests/tests/test_test.py +++ b/nbcelltests/tests/test_test.py @@ -33,6 +33,7 @@ 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') @@ -130,7 +131,7 @@ def test_non_code_cell_with_test_causes_error(self): else: raise Exception("Test script should fail to generate") - def test_non_code_cell_with_test_causes_error(self): + 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: @@ -138,6 +139,14 @@ def test_non_code_cell_with_test_causes_error(self): 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") From 9784f7b75f477335e8849d5d1c9a6dbdcc789be4 Mon Sep 17 00:00:00 2001 From: Chris B Date: Fri, 12 Jun 2020 12:29:23 +0100 Subject: [PATCH 7/7] Remove nbval hack so can be formally reviewed. --- nbcelltests/tests/test_test.py | 5 ++++- nbcelltests/tests_vendored.py | 3 --- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/nbcelltests/tests/test_test.py b/nbcelltests/tests/test_test.py index ca2c8cc..c5569c8 100644 --- a/nbcelltests/tests/test_test.py +++ b/nbcelltests/tests/test_test.py @@ -11,6 +11,8 @@ import sys import unittest +import pytest + from nbcelltests.test import run, runWithReturn, runWithReport, runWithHTMLReturn # Some straightforward TODOs: @@ -431,11 +433,12 @@ def test_failure_is_detected(self): t.tearDownClass() -# TODO: this is an nbval bug +# 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() diff --git a/nbcelltests/tests_vendored.py b/nbcelltests/tests_vendored.py index f76fa24..ed9d744 100644 --- a/nbcelltests/tests_vendored.py +++ b/nbcelltests/tests_vendored.py @@ -329,9 +329,6 @@ def _run(self, cell_content, description=''): elif msg_type in ('display_data', 'execute_result'): continue elif msg_type == 'stream': - # TODO: temporary/should not be here; needs fix in nbval - if msg['content']['name'] == 'stderr': - raise Exception(msg['content']['text']) continue # if the message type is an error then an error has occurred during