Merge pull request #14 from UBC-DSCI/review-fixes

Review fixes

Author: trevorcampbell

Diff for: nbgrader/apps/quickstartapp.py

@@ -122,10 +122,10 @@ def start(self):
         ignore_html = shutil.ignore_patterns("*.html")
         shutil.copytree(example, os.path.join(course_path, "source"), ignore=ignore_html)
 
-        # copying the tests.yml file to the course directory
+        # copying the autotests.yml file to the course directory
         tests_file_path = os.path.abspath(os.path.join(
-            os.path.dirname(__file__), '..', 'docs', 'source', 'user_guide', 'tests.yml'))
-        shutil.copyfile(tests_file_path, os.path.join(course_path, 'tests.yml'))
+            os.path.dirname(__file__), '..', 'docs', 'source', 'user_guide', 'autotests.yml'))
+        shutil.copyfile(tests_file_path, os.path.join(course_path, 'autotests.yml'))
 
         # create the config file
         self.log.info("Generating example config file...")

Diff for: nbgrader/docs/source/user_guide/tests.yml renamed to nbgrader/docs/source/user_guide/autotests.yml

@@ -33,7 +33,7 @@ class InstantiateTests(NbGraderPreprocessor):
     tests = None
 
     autotest_filename = Unicode(
-        "tests.yml",
+        "autotests.yml",
         help="The filename where automatic testing code is stored"
     ).tag(config=True)
 
@@ -115,7 +115,7 @@ def preprocess(self, nb, resources):
                     raise ValueError(f"Kernel {kernel_name} has not been specified in InstantiateTests.comment_strs")
                 if kernel_name not in self.sanitizers:
                     raise ValueError(f"Kernel {kernel_name} has not been specified in InstantiateTests.sanitizers")
-                self.log.debug(f"Found kernel {kernel_name}")
+                self.log.debug("Found kernel %s", kernel_name)
                 resources["kernel_name"] = kernel_name
 
                 # load the template tests file
@@ -124,10 +124,10 @@ def preprocess(self, nb, resources):
                 self.global_tests_loaded = True
 
                 # set up the sanitizer
-                self.log.debug('Setting sanitizer for kernel {kernel_name}')
+                self.log.debug('Setting sanitizer for kernel %s', kernel_name)
                 self.sanitizer = self.sanitizers[kernel_name]
                 #start the kernel
-                self.log.debug('Starting client for kernel {kernel_name}')
+                self.log.debug('Starting client for kernel %s', kernel_name)
                 km, self.kc = start_new_kernel(kernel_name = kernel_name)
 
                 # run the preprocessor
@@ -214,7 +214,9 @@ def preprocess_cell(self, cell, resources, index):
             # appears in the line before the self.autotest_delimiter token
             use_hash = (self.hashed_delimiter in line[:line.find(self.autotest_delimiter)])
             if use_hash:
-                self.log.debug('Hashing delimiter found, using template: ' + self.hash_template)
+                if self.hash_template is None:
+                    raise ValueError('Found a hashing delimiter, but the hash property has not been set in autotests.yml')
+                self.log.debug('Hashing delimiter found, using template: %s', self.hash_template)
             else:
                 self.log.debug('Hashing delimiter not found')
 
@@ -233,18 +235,18 @@ def preprocess_cell(self, cell, resources, index):
 
             # generate the test for each snippet
             for snippet in snippets:
-                self.log.debug('Running autotest generation for snippet ' + snippet)
+                self.log.debug('Running autotest generation for snippet %s', snippet)
 
                 # create a random salt for this test
                 if use_hash:
                     salt = secrets.token_hex(8)
-                    self.log.debug('Using salt: ' + salt)
+                    self.log.debug('Using salt: %s', salt)
                 else:
                     salt = None
 
                 # get the normalized(/hashed) template tests for this code snippet
                 self.log.debug(
-                    'Instantiating normalized' + ('/hashed ' if use_hash else ' ') + 'test templates based on type')
+                    'Instantiating normalized%s test templates based on type', ' & hashed' if use_hash else '')
                 instantiated_tests, test_values, fail_messages = self._instantiate_tests(snippet, salt)
 
                 # add all the lines to the cell
@@ -253,21 +255,21 @@ def preprocess_cell(self, cell, resources, index):
                 for i in range(len(instantiated_tests)):
                     check_code = template.render(snippet=instantiated_tests[i], value=test_values[i],
                                                  message=fail_messages[i])
-                    self.log.debug('Test: ' + check_code)
+                    self.log.debug('Test: %s', check_code)
                     new_lines.append(check_code)
 
                 # add an empty line after this block of test code
                 new_lines.append('')
 
+        # add the final success code and execute it
+        if is_grade_flag and self.global_tests_loaded and (self.autotest_delimiter in cell.source) and (self.success_code is not None):
+            new_lines.append(self.success_code)
+            non_autotest_code_lines.append(self.success_code)
+
         # run the trailing non-autotest lines, if any remain
         if len(non_autotest_code_lines) > 0:
             self._execute_code_snippet("\n".join(non_autotest_code_lines))
 
-        # add the final success message
-        if is_grade_flag and self.global_tests_loaded:
-            if self.autotest_delimiter in cell.source:
-                new_lines.append(self.success_code)
-
         # replace the cell source
         cell.source = "\n".join(new_lines)
 
@@ -279,36 +281,34 @@ def preprocess_cell(self, cell, resources, index):
     # -------------------------------------------------------------------------------------
     def _load_test_template_file(self, resources):
         """
-        attempts to load the tests.yml file within the assignment directory. In case such file is not found
+        attempts to load the autotests.yml file within the assignment directory. In case such file is not found
         or perhaps cannot be loaded, it will attempt to load the default_tests.yaml file with the course_directory
         """
-        self.log.debug('loading template tests.yml...')
-        self.log.debug(f'kernel_name: {resources["kernel_name"]}')
+        self.log.debug('loading template autotests.yml...')
+        self.log.debug('kernel_name: %s', resources["kernel_name"])
         try:
             with open(os.path.join(resources['metadata']['path'], self.autotest_filename), 'r') as tests_file:
                 tests = yaml.safe_load(tests_file)
             self.log.debug(tests)
 
         except FileNotFoundError:
-            # if there is no tests file, just load a default tests dict
+            # if there is no tests file, try to load default tests dict
             self.log.warning(
-                'No tests.yml file found in the assignment directory. Loading the default tests.yml file in the course root directory')
-            # tests = {}
+                'No autotests.yml file found in the assignment directory. Loading the default autotests.yml file in the course root directory')
             try:
                 with open(os.path.join(self.autotest_filename), 'r') as tests_file:
                     tests = yaml.safe_load(tests_file)
             except FileNotFoundError:
-                # if there is no tests file, just create a default empty tests dict
-                self.log.warning(
-                    'No tests.yml file found. If AUTOTESTS appears in testing cells, an error will be thrown.')
-                tests = {}
+                # if there is not even a default tests file, re-raise the FileNotFound error
+                self.log.error('No autotests.yml file found, but there were autotest directives found in the notebook. ')
+                raise
             except yaml.parser.ParserError as e:
-                self.log.error('tests.yml contains invalid YAML code.')
+                self.log.error('autotests.yml contains invalid YAML code.')
                 self.log.error(e.msg)
                 raise
 
         except yaml.parser.ParserError as e:
-            self.log.error('tests.yml contains invalid YAML code.')
+            self.log.error('autotests.yml contains invalid YAML code.')
             self.log.error(e.msg)
             raise
 
@@ -322,10 +322,10 @@ def _load_test_template_file(self, resources):
         self.dispatch_template = tests['dispatch']
 
         # get the success message template
-        self.success_code = tests['success']
+        self.success_code = tests.get('success', None)
 
         # get the hash code template
-        self.hash_template = tests['hash']
+        self.hash_template = tests.get('hash', None)
 
         # get the hash code template
         self.check_template = tests['check']
@@ -342,19 +342,19 @@ def _instantiate_tests(self, snippet, salt=None):
         template = j2.Environment(loader=j2.BaseLoader).from_string(self.dispatch_template)
         dispatch_code = template.render(snippet=snippet)
         dispatch_result = self._execute_code_snippet(dispatch_code)
-        self.log.debug('Dispatch result returned by kernel: ', dispatch_result)
+        self.log.debug('Dispatch result returned by kernel: %s', dispatch_result)
         # get the test code; if the type isn't in our dict, just default to 'default'
         # if default isn't in the tests code, this will throw an error
         try:
             tests = self.test_templates_by_type.get(dispatch_result, self.test_templates_by_type['default'])
         except KeyError:
-            self.log.error('tests.yml must contain a top-level "default" key with corresponding test code')
+            self.log.error('autotests.yml must contain a top-level "default" key with corresponding test code')
             raise
         try:
             test_templs = [t['test'] for t in tests]
             fail_msgs = [t['fail'] for t in tests]
         except KeyError:
-            self.log.error('each type in tests.yml must have a list of dictionaries with a "test" and "fail" key')
+            self.log.error('each type in autotests.yml must have a list of dictionaries with a "test" and "fail" key')
             self.log.error('the "test" item should store the test template code, '
                            'and the "fail" item should store a failure message')
             raise

Diff for: nbgrader/preprocessors/instantiatetests.py

@@ -102,7 +102,7 @@ def test_grade_autotest(self, db, course_dir):
         run_nbgrader(["db", "student", "add", "bar", "--db", db])
 
         self._copy_file(join("files", "autotest-simple.ipynb"), join(course_dir, "source", "ps1", "p1.ipynb"))
-        self._copy_file(join("files", "tests.yml"), join(course_dir, "tests.yml"))
+        self._copy_file(join("files", "autotests.yml"), join(course_dir, "autotests.yml"))
         run_nbgrader(["generate_assignment", "ps1", "--db", db])
 
         self._copy_file(join("files", "autotest-simple-unchanged.ipynb"), join(course_dir, "submitted", "foo", "ps1", "p1.ipynb"))
@@ -132,7 +132,7 @@ def test_grade_hashed_autotest(self, db, course_dir):
         run_nbgrader(["db", "student", "add", "bar", "--db", db])
 
         self._copy_file(join("files", "autotest-hashed.ipynb"), join(course_dir, "source", "ps1", "p1.ipynb"))
-        self._copy_file(join("files", "tests.yml"), join(course_dir, "tests.yml"))
+        self._copy_file(join("files", "autotests.yml"), join(course_dir, "autotests.yml"))
         run_nbgrader(["generate_assignment", "ps1", "--db", db])
 
         self._copy_file(join("files", "autotest-hashed-unchanged.ipynb"), join(course_dir, "submitted", "foo", "ps1", "p1.ipynb"))
@@ -162,7 +162,7 @@ def test_grade_complex_autotest(self, db, course_dir):
         run_nbgrader(["db", "student", "add", "bar", "--db", db])
 
         self._copy_file(join("files", "autotest-multi.ipynb"), join(course_dir, "source", "ps1", "p1.ipynb"))
-        self._copy_file(join("files", "tests.yml"), join(course_dir, "tests.yml"))
+        self._copy_file(join("files", "autotests.yml"), join(course_dir, "autotests.yml"))
         run_nbgrader(["generate_assignment", "ps1", "--db", db])
 
         self._copy_file(join("files", "autotest-multi-unchanged.ipynb"), join(course_dir, "submitted", "foo", "ps1", "p1.ipynb"))
@@ -944,7 +944,7 @@ def test_hidden_tests_autotest(self, db, course_dir):
             fh.write("""c.ClearSolutions.code_stub=dict(python="# YOUR CODE HERE")""")
 
         self._copy_file(join("files", "autotest-hidden.ipynb"), join(course_dir, "source", "ps1", "p1.ipynb"))
-        self._copy_file(join("files", "tests.yml"), join(course_dir, "tests.yml"))
+        self._copy_file(join("files", "autotests.yml"), join(course_dir, "autotests.yml"))
         run_nbgrader(["generate_assignment", "ps1", "--db", db])
 
         self._copy_file(join("files", "autotest-hidden-unchanged.ipynb"), join(course_dir, "submitted", "foo", "ps1", "p1.ipynb"))

Diff for: nbgrader/tests/apps/files/tests.yml renamed to nbgrader/tests/apps/files/autotests.yml

@@ -50,23 +50,23 @@ def test_single_file(self, course_dir, temp_cwd):
     def test_autotests_simple(self, course_dir, temp_cwd):
         """Can a notebook with simple autotests be generated with a default yaml location, and is autotest code removed?"""
         self._copy_file(join("files", "autotest-simple.ipynb"), join(course_dir, "source", "ps1", "foo.ipynb"))
-        self._copy_file(join("files", "tests.yml"), join(course_dir, "tests.yml"))
+        self._copy_file(join("files", "autotests.yml"), join(course_dir, "autotests.yml"))
         run_nbgrader(["db", "assignment", "add", "ps1"])
         run_nbgrader(["generate_assignment", "ps1"])
         assert os.path.isfile(join(course_dir, "release", "ps1", "foo.ipynb"))
-        assert not os.path.isfile(join(course_dir, "release", "ps1", "tests.yml"))
+        assert not os.path.isfile(join(course_dir, "release", "ps1", "autotests.yml"))
 
         foo = self._file_contents(join(course_dir, "release", "ps1", "foo.ipynb"))
         assert "AUTOTEST" not in foo
 
     def test_autotests_simple(self, course_dir, temp_cwd):
         """Can a notebook with simple autotests be generated with an assignment-specific yaml, and is autotest code removed?"""
         self._copy_file(join("files", "autotest-simple.ipynb"), join(course_dir, "source", "ps1", "foo.ipynb"))
-        self._copy_file(join("files", "tests.yml"), join(course_dir, "source", "ps1", "tests.yml"))
+        self._copy_file(join("files", "autotests.yml"), join(course_dir, "source", "ps1", "autotests.yml"))
         run_nbgrader(["db", "assignment", "add", "ps1"])
         run_nbgrader(["generate_assignment", "ps1"])
         assert os.path.isfile(join(course_dir, "release", "ps1", "foo.ipynb"))
-        assert os.path.isfile(join(course_dir, "release", "ps1", "tests.yml"))
+        assert os.path.isfile(join(course_dir, "release", "ps1", "autotests.yml"))
 
         foo = self._file_contents(join(course_dir, "release", "ps1", "foo.ipynb"))
         assert "AUTOTEST" not in foo
@@ -81,7 +81,7 @@ def test_autotests_needs_yaml(self, course_dir, temp_cwd):
     def test_autotests_fancy(self, course_dir, temp_cwd):
         """Can a more complicated autotests notebook be generated, and is autotest code removed?"""
         self._copy_file(join("files", "autotest-multi.ipynb"), join(course_dir, "source", "ps1", "foo.ipynb"))
-        self._copy_file(join("files", "tests.yml"), join(course_dir, "tests.yml"))
+        self._copy_file(join("files", "autotests.yml"), join(course_dir, "autotests.yml"))
         run_nbgrader(["db", "assignment", "add", "ps1"])
         run_nbgrader(["generate_assignment", "ps1"])
         assert os.path.isfile(join(course_dir, "release", "ps1", "foo.ipynb"))
@@ -92,7 +92,7 @@ def test_autotests_fancy(self, course_dir, temp_cwd):
     def test_autotests_hidden(self, course_dir, temp_cwd):
         """Can a notebook with hidden autotest be generated, and is autotest/hidden sections removed?"""
         self._copy_file(join("files", "autotest-hidden.ipynb"), join(course_dir, "source", "ps1", "foo.ipynb"))
-        self._copy_file(join("files", "tests.yml"), join(course_dir, "tests.yml"))
+        self._copy_file(join("files", "autotests.yml"), join(course_dir, "autotests.yml"))
         run_nbgrader(["db", "assignment", "add", "ps1"])
         run_nbgrader(["generate_assignment", "ps1"])
         assert os.path.isfile(join(course_dir, "release", "ps1", "foo.ipynb"))
@@ -104,7 +104,7 @@ def test_autotests_hidden(self, course_dir, temp_cwd):
     def test_autotests_hashed(self, course_dir, temp_cwd):
         """Can a notebook with hashed autotests be generated, and is hashed autotest code removed?"""
         self._copy_file(join("files", "autotest-hashed.ipynb"), join(course_dir, "source", "ps1", "foo.ipynb"))
-        self._copy_file(join("files", "tests.yml"), join(course_dir, "tests.yml"))
+        self._copy_file(join("files", "autotests.yml"), join(course_dir, "autotests.yml"))
         run_nbgrader(["db", "assignment", "add", "ps1"])
         run_nbgrader(["generate_assignment", "ps1"])
         assert os.path.isfile(join(course_dir, "release", "ps1", "foo.ipynb"))
@@ -117,7 +117,7 @@ def test_generate_source_with_tests_flag(self, course_dir, temp_cwd):
         """Does setting the flag --source_with_tests also create a notebook with solution and tests in the
         source_with_tests directory"""
         self._copy_file(join("files", "autotest-simple.ipynb"), join(course_dir, "source", "ps1", "foo.ipynb"))
-        self._copy_file(join("files", "tests.yml"), join(course_dir, "source", "ps1", "tests.yml"))
+        self._copy_file(join("files", "autotests.yml"), join(course_dir, "source", "ps1", "autotests.yml"))
         run_nbgrader(["db", "assignment", "add", "ps1"])
         run_nbgrader(["generate_assignment", "ps1", "--source_with_tests"])
         assert os.path.isfile(join(course_dir, "release", "ps1", "foo.ipynb"))

Diff for: nbgrader/tests/apps/test_nbgrader_autograde.py

@@ -333,7 +333,7 @@ def test_autotests(self, course_dir):
 
         self._copy_file(join("files", "autotest-simple.ipynb"), join(course_dir, "source", "ps1", "p1.ipynb"))
         self._copy_file(join("files", "autotest-simple.ipynb"), join(course_dir, "source", "ps1", "p2.ipynb"))
-        self._copy_file(join("files", "tests.yml"), join(course_dir, "tests.yml"))
+        self._copy_file(join("files", "autotests.yml"), join(course_dir, "autotests.yml"))
         run_nbgrader(["generate_assignment", "ps1"])
 
         self._copy_file(join("files", "autotest-simple.ipynb"), join(course_dir, "submitted", "foo", "ps1", "p1.ipynb"))