Remove TODOs, cleanup logic, tests

.gitignore

@@ -22,3 +22,4 @@ docs/_static/notebooks/html
 htmlcov/
 .pytest_cache/
 /sync.sh
+pytest-report.html

Makefile

@@ -5,7 +5,7 @@ COVERAGE         = coverage
 DOCKER           = true
 SLOW             = true
 
-_PYTESTOPTS      := -vv --durations=0
+_PYTESTOPTS      := -vv --durations=0 --html=pytest-report.html --self-contained-html
 
 ifeq ($(DOCKER), false)
 	_PYTESTOPTS := $(_PYTESTOPTS) -m "not docker"

docs/otter_check/index.rst

@@ -62,12 +62,11 @@ variable name collisions, propagating errors, or other things that would cause t
 fail a test they should be passing.
 
 
-.. TODO: fix this section
-
 Exporting Submissions
 +++++++++++++++++++++
 
-Students can also use the ``Notebook`` class to generate their own PDFs for manual grading using the 
+Students can also use the ``Notebook`` class to generate a zip file containing all of their work for
+submission with the 
 method ``Notebook.export``. This function takes an optional argument of the path to the notebook; if 
 unspecified, it will infer the path by trying to read the config file (if present), using the path 
 of the only notebook in the working directory if there is only one, or it will raise an error 

otter/api.py

@@ -3,8 +3,6 @@
 __all__ = ["export_notebook", "grade_submission"]
 
 import os
-import shutil
-import tempfile
 
 from contextlib import redirect_stdout
 
@@ -41,22 +39,17 @@ def grade_submission(submission_path, ag_path="autograder.zip", quiet=False, deb
         ``otter.test_files.GradingResults``: the results object produced during the grading of the
             submission.
     """
-    dp = tempfile.mkdtemp()
-
     if quiet:
         f = open(os.devnull, "w")
         cm = redirect_stdout(f)
     else:
         cm = nullcontext()
 
-    # TODO: is the output_dir argument of run_grader necessary here?
     with cm:
         results = run_grader(
-            submission_path, autograder=ag_path, output_dir=dp, no_logo=True, debug=debug)
+            submission_path, autograder=ag_path, output_dir=None, no_logo=True, debug=debug)
 
     if quiet:
         f.close()
 
-    shutil.rmtree(dp)
-
     return results

otter/assign/__init__.py

@@ -35,15 +35,15 @@ def main(
         no_pdfs (``bool``): whether to ignore any configurations indicating PDF generation for this run
         no_run_tests (``bool``): prevents Otter tests from being automatically run on the solutions 
             notebook
-        username (``str``): a username for Gradescope for generating a token
-        password (``str``): a password for Gradescope for generating a token
+        username (``str | None``): a username for Gradescope for generating a token
+        password (``str | None``): a password for Gradescope for generating a token
         debug (``bool``): whether to run in debug mode (without ignoring errors during testing)
     """
     LOGGER.debug(f"User-specified master path: {master}")
     LOGGER.debug(f"User-specified result path: {result}")
     master, result = pathlib.Path(os.path.abspath(master)), pathlib.Path(os.path.abspath(result))
 
-    assignment = Assignment()
+    assignment = Assignment(require_valid_keys=True)
 
     result = get_relpath(master.parent, result)
 

otter/assign/assignment.py

@@ -13,7 +13,6 @@
 from ..utils import Loggable
 
 
-# TODO: add detection/warnings/errors for when a user provides an invalid key? (to be added to fica)
 class Assignment(fica.Config, Loggable):
     """
     Configurations for the assignment.
@@ -241,8 +240,7 @@ class TestsValue(fica.Config):
     seed_required: bool = False
     """whether a seeding configuration is required for Otter Generate"""
 
-    # TODO: rename this
-    _temp_test_dir: Optional[str] = None
+    generate_tests_dir: Optional[str] = None
     """the path to a directory of test files for Otter Generate"""
 
     notebook_basename: Optional[str] = None
@@ -267,7 +265,7 @@ def __init__(self, user_config: Dict[str, Any] = {}, **kwargs) -> None:
         if self.variables:
             warnings.warn(
                 "The variables key of the assignment config is deprecated and will be removed in " \
-                    "v6.0.0. Please use generate.seed_variables instead.", DeprecationWarning)
+                    "v6.0.0. Please use generate.serialized_variables instead.", DeprecationWarning)
 
     def update(self, user_config: Dict[str, Any]):
         self._logger.debug(f"Updating config: {user_config}")
@@ -309,7 +307,6 @@ def get_otter_config(self):
         if self.is_r:
             self.generate.lang = "r"
 
-        # TODO: move this config out of the assignment metadata and into the generate key
         if self.variables:
             self.generate.serialized_variables = str(self.variables)
 

otter/assign/feature_toggle.py

@@ -11,7 +11,6 @@
 """a function that returns ``True`` if the assignment is not an R Markdown assignment"""
 
 
-# TODO: move other things in
 class FeatureToggle(Enum):
     """
     An enum for managing features that are enabled or disabled depending on the assignment config.

otter/assign/notebook_transformer.py

@@ -1,6 +1,7 @@
 """Master notebook transformer for Otter Assign"""
 
 import copy
+import fica
 import nbformat
 
 from .assignment import Assignment
@@ -21,8 +22,6 @@
 from .tests_manager import AssignmentTestsManager
 from .utils import (
     add_tag,
-    add_assignment_name_to_notebook,
-    add_require_no_pdf_ack_to_notebook,
     AssignNotebookFormatException,
     get_source,
     is_cell_type,
@@ -31,6 +30,9 @@
     remove_cell_ids_if_applicable,
 )
 
+from ..nbmeta_config import NBMetadataConfig
+from ..utils import NOTEBOOK_METADATA_KEY
+
 
 class NotebookTransformer:
     """
@@ -134,10 +136,6 @@ def transform_notebook(self, nb) -> "TransformedNotebookContainer":
         # strip out ignored lines
         transformed_nb = strip_ignored_lines(transformed_nb)
 
-        add_assignment_name_to_notebook(transformed_nb, self.assignment)
-
-        add_require_no_pdf_ack_to_notebook(transformed_nb, self.assignment)
-
         remove_cell_ids_if_applicable(transformed_nb)
 
         return TransformedNotebookContainer(transformed_nb, self)
@@ -353,9 +351,25 @@ class TransformedNotebookContainer:
     nb_transformer: NotebookTransformer
     """the notebook transformer used to create ``transformed_nb``"""
 
+    nbmeta_config: NBMetadataConfig
+    """"""
+
     def __init__(self, transformed_nb, nb_transformer):
         self.transformed_nb = transformed_nb
         self.nb_transformer = nb_transformer
+        self._populate_nbmeta_config(self.nb_transformer.assignment)
+
+    def _populate_nbmeta_config(self, a: Assignment):
+        """
+        """
+        self.nbmeta_config = NBMetadataConfig({})
+        if a.name:
+            self.nbmeta_config.assignment_name = a.name
+        if a.export_cell and a.export_cell.require_no_pdf_ack:
+            self.nbmeta_config.require_no_pdf_confirmation = True
+            if isinstance(a.export_cell.require_no_pdf_ack, fica.Config):
+                self.nbmeta_config.export_pdf_failure_message = \
+                    a.export_cell.require_no_pdf_ack.message
 
     def _get_sanitized_nb(self):
         """
@@ -374,6 +388,13 @@ def _get_sanitized_nb(self):
             )
         return nb
 
+    def _add_nbmeta_config(self, nb):
+        """
+        """
+        uc = self.nbmeta_config.get_user_config()
+        if uc:
+            nb["metadata"][NOTEBOOK_METADATA_KEY] = uc
+
     def write_transformed_nb(self, output_path, sanitize):
         """
         Write the transformed notebook (either as an autograder or student notebook) to the
@@ -384,8 +405,11 @@ def write_transformed_nb(self, output_path, sanitize):
             sanitize (``bool``): whether to sanitize the notebook (i.e. write the student version)
         """
         nb = self._get_sanitized_nb() if sanitize else self.transformed_nb
+        self._add_nbmeta_config(nb)
+
         if self.nb_transformer.assignment.is_rmd:
             rmarkdown_converter.write_as_rmd(nb, str(output_path), not sanitize)
+
         else:
             try:
                 from nbformat.validator import normalize
@@ -407,7 +431,7 @@ def write_tests(self, tests_dir, include_hidden, force_files):
                 assignment config)
         """
         self.nb_transformer.tests_mgr.write_tests(
-            self.transformed_nb,
+            self.nbmeta_config,
             tests_dir,
             include_hidden=include_hidden,
             force_files=force_files,

otter/assign/output.py

@@ -61,8 +61,8 @@ def write_output_dir(
 
     # write a temp dir for otter generate tests
     if not sanitize:
-        assignment._temp_test_dir = pathlib.Path(tempfile.mkdtemp())
-        transformed_nb.write_tests(str(assignment._temp_test_dir), True, True)
+        assignment.generate_tests_dir = tempfile.mkdtemp()
+        transformed_nb.write_tests(assignment.generate_tests_dir, True, True)
 
     transformed_nb.write_transformed_nb(output_path, sanitize)
 
@@ -84,7 +84,7 @@ def write_output_dir(
             shutil.copy(file, str(output_dir / rel_path))
 
 
-def write_output_directories(assignment):
+def write_output_directories(assignment: Assignment):
     """
     Process a master notebook and write the results to the output directories.
 

otter/assign/tests_manager.py

@@ -2,6 +2,7 @@
 
 import os
 import pandas as pd
+import pathlib
 import pprint
 import re
 import yaml
@@ -15,8 +16,8 @@
 from .solutions import remove_ignored_lines
 from .utils import get_source, str_to_doctest
 
-from ..test_files.abstract_test import OK_FORMAT_VARNAME, TestFile
-from ..utils import NOTEBOOK_METADATA_KEY
+from ..nbmeta_config import NBMetadataConfig, OK_FORMAT_VARNAME
+from ..test_files.abstract_test import TestFile
 
 
 BEGIN_TEST_CONFIG_REGEX = r'(?:.\s*=\s*)?""?"?\s*#\s*BEGIN\s*TEST\s*CONFIG'
@@ -319,24 +320,27 @@ def _format_test(self, name, points, test_cases) -> Union[str, Dict[str, Any]]:
 
         return test
 
-    def write_tests(self, nb, test_dir, include_hidden=True, force_files=False):
+    def write_tests(
+            self,
+            nbmeta_config: NBMetadataConfig,
+            test_dir: Union[pathlib.Path, str],
+            include_hidden: bool = True,
+            force_files: bool = False,
+        ):
         """
         Write all test files to a notebook's metadata or a tests directory.
 
         Args:
-            nb (``nbformat.NotebookNode``): the notebook
+            nbmeta_config (``otter.nbmeta_config.NBMetadataConfig``): the notebook metadata config
             test_dir (``pathlib.Path | str``): the tests directory
             include_hidden (``bool``): whether to include hidden test cases
             force_files (``bool``): whether to force writing to test files (overriding the
                 assignment config)
         """
-        # TODO: move this notebook to the notebook metadata test classes
-        if isinstance(nb, dict) and not self.assignment.tests.files and not force_files:
-            if NOTEBOOK_METADATA_KEY not in nb["metadata"]:
-                nb["metadata"][NOTEBOOK_METADATA_KEY] = {}
-            nb["metadata"][NOTEBOOK_METADATA_KEY]["tests"] = {}
-            nb["metadata"][NOTEBOOK_METADATA_KEY][OK_FORMAT_VARNAME] = \
-                self.assignment.tests.ok_format
+        use_files = self.assignment.tests.files or force_files
+        if not use_files:
+            nbmeta_config.tests = {}
+            nbmeta_config.ok_format = self.assignment.tests.ok_format
 
         test_ext = ".py" if self.assignment.is_python else ".R"
         for test_name in self._tests_by_question.keys():
@@ -352,7 +356,7 @@ def write_tests(self, nb, test_dir, include_hidden=True, force_files=False):
             test = \
                 self._format_test(test_info["name"], test_info["points"], test_info["test_cases"])
 
-            if self.assignment.tests.files or force_files:
+            if use_files:
                 with open(test_path, "w+") as f:
                     if isinstance(test, dict):
                         f.write(f"{OK_FORMAT_VARNAME} = True\n\ntest = ")
@@ -362,8 +366,7 @@ def write_tests(self, nb, test_dir, include_hidden=True, force_files=False):
                         f.write(test)
 
             else:
-                # TODO: move this notebook to the notebook metadata test classes
-                nb["metadata"][NOTEBOOK_METADATA_KEY]["tests"][test_info["name"]] = test
+                nbmeta_config.tests[test_info["name"]] = test
 
     def determine_question_point_value(self, question):
         """