diff --git a/CHANGELOG.md b/CHANGELOG.md index cde72868..b881f0b1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,7 @@ * Fixed OK-test support for the `all_or_nothing` config per [#751](https://github.com/ucbds-infra/otter-grader/issues/751) * Added exception-based test support for the `all_or_nothing` config per [#751](https://github.com/ucbds-infra/otter-grader/issues/751) * Added Otter Assign support for the `all_or_nothing` config per [#751](https://github.com/ucbds-infra/otter-grader/issues/751) +* Updated Otter Assign to only allow a function definition and statement to call the test function in exception-based test cells and automatically ignore the latter statement instead of requiring an explicit `# IGNORE` comment per [#516](https://github.com/ucbds-infra/otter-grader/issues/516) **v5.7.1:** diff --git a/docs/otter_assign/notebook_format.rst b/docs/otter_assign/notebook_format.rst index 1b9ff153..ba9823cc 100644 --- a/docs/otter_assign/notebook_format.rst +++ b/docs/otter_assign/notebook_format.rst @@ -462,10 +462,11 @@ Exception-Based Test Cells To use Otter's exception-based tests, you must set ``tests: ok_format: false`` in your assignment config. Your test cells should define -a test case function as described :ref:`here `. You can run the -test in the master notebook by calling the function, but you should make sure that this call is -"ignored" by Otter Assign so that it's not included in the test file by appending ``# IGNORE`` to the -end of line. You should *not* add the ``test_case`` decorator; Otter Assign will do this for you. +a test case function as described :ref:`here `. You can also run the +test in the master notebook by calling the function. However, no other statements besides a single +``def`` and (optionally) a call to the test function may be included in the cell; Otter enforces +this by parsing the cell's code into an AST, which it uses to identify the test case function. +You should *not* add the ``test_case`` decorator; Otter Assign will do this for you. For example, @@ -478,7 +479,7 @@ For example, assert len(arr) == 10 assert (0 <= arr <= 1).all() - test_validity(arr) # IGNORE + test_validity(arr) It is important to note that the exception-based test files are executed before the student's global environment is provided, so no work should be performed outside the test case function that relies @@ -585,13 +586,13 @@ Ignoring Cells -------------- For any cells that you don't want to be included in *either* of the output notebooks that are -present in the master notebook, include a line at the top of the cell with the ``## Ignore ##`` +present in the master notebook, include a line at the top of the cell with the ``# IGNORE`` comment (case insensitive) just like with test cells. Note that this also works for Markdown cells with the same syntax. .. code-block:: python - ## Ignore ## + # IGNORE print("This cell won't appear in the output.") diff --git a/otter/assign/tests_manager.py b/otter/assign/tests_manager.py index 26f44843..2168d86b 100644 --- a/otter/assign/tests_manager.py +++ b/otter/assign/tests_manager.py @@ -14,7 +14,6 @@ from .assignment import Assignment from .question_config import QuestionConfig -from .solutions import remove_ignored_lines from .utils import str_to_doctest from ..nbmeta_config import NBMetadataConfig, OK_FORMAT_VARNAME from ..test_files.abstract_test import TestCase, TestFile @@ -38,6 +37,7 @@ success_message="{{ tc.success_message }}"{% endif %}{% if tc.failure_message is not none %}, failure_message="{{ tc.failure_message }}"{% endif %}) {{ tc.body }} + {% endfor %} """ ) @@ -151,12 +151,22 @@ def read_test(self, cell: nbf.NotebookNode, question: QuestionConfig): success_message = config.get("success_message", None) failure_message = config.get("failure_message", None) - body: str = "\n".join(remove_ignored_lines(get_source(cell)[test_start_line + 1 :])) + test_source = get_source(cell)[test_start_line + 1 :] + test_ast = ast.parse("\n".join(test_source)) + body: str = "" if self.assignment.tests.ok_format: - inp = ast.unparse(_AnnotationRemover().visit(ast.parse(body))) + inp = ast.unparse(_AnnotationRemover().visit(test_ast)) body_lines = str_to_doctest(inp.split("\n"), []) body_lines.extend(output.split("\n")) body = "\n".join(body_lines) + else: + if len(test_ast.body) > 2 or not isinstance(test_ast.body[0], ast.FunctionDef): + raise ValueError( + f"Error in question {question.name}: an exception-based test cell may have at " + "most a function definition and a call to that function definition but a test " + f"cell with {len(test_ast.body)} nodes in its AST was encountered" + ) + body = ast.unparse(test_ast.body[0]) self._add_test_case( question, diff --git a/test/test_assign/files/exception-correct/autograder/tests/q8.py b/test/test_assign/files/exception-correct/autograder/tests/q8.py index 278fc804..2bdaa611 100644 --- a/test/test_assign/files/exception-correct/autograder/tests/q8.py +++ b/test/test_assign/files/exception-correct/autograder/tests/q8.py @@ -11,6 +11,5 @@ def test_len(z): @test_case(points=None, hidden=True) def test_values(np, z): - assert np.allclose(z, [4.99342831, 3.7234714 , 5.29537708, 7.04605971, 3.53169325, - 3.53172609, 7.15842563, 5.53486946, 3.06105123, 5.08512009]) + assert np.allclose(z, [4.99342831, 3.7234714, 5.29537708, 7.04605971, 3.53169325, 3.53172609, 7.15842563, 5.53486946, 3.06105123, 5.08512009]) diff --git a/test/test_assign/files/exception-example.ipynb b/test/test_assign/files/exception-example.ipynb index 54cc0ead..454b2e6b 100644 --- a/test/test_assign/files/exception-example.ipynb +++ b/test/test_assign/files/exception-example.ipynb @@ -90,7 +90,7 @@ " assert isinstance(x, int)\n", " assert 0 < x < 100\n", "\n", - "test_type(x) # IGNORE" + "test_type(x)" ] }, { @@ -107,7 +107,7 @@ "def test_value(x):\n", " assert x == 2\n", "\n", - "test_value(x) # IGNORE" + "test_value(x)" ] }, { @@ -277,7 +277,7 @@ " assert nine == 9\n", " assert square(16) == 256\n", "\n", - "test_public(nine, square) # IGNORE" + "test_public(nine, square)" ] }, { @@ -290,7 +290,7 @@ "def test_hidden(square):\n", " assert square(1) == 1\n", "\n", - "test_hidden(square) # IGNORE" + "test_hidden(square)" ] }, { @@ -602,7 +602,7 @@ "def test_len(z):\n", " assert len(z) == 10\n", "\n", - "test_len(z) # IGNORE" + "test_len(z)" ] }, { @@ -616,7 +616,7 @@ " assert np.allclose(z, [4.99342831, 3.7234714 , 5.29537708, 7.04605971, 3.53169325,\n", " 3.53172609, 7.15842563, 5.53486946, 3.06105123, 5.08512009])\n", "\n", - "test_values(np, z) # IGNORE" + "test_values(np, z)" ] }, {