Deep code tests (#97)

* Add some unit-tests for class-methods in ReleaseAssignment, FetchAssignment, Submit, and Collect

* Tweak a couple of the string tests

* Add method tests for the feedback plugins

* Tweak the docs

* Add 'clear_database' to most of the test_handler files, and remove all but one 'skip' flag

* Lint test_handlers_feedback.py

Co-authored-by: Ian Stuart <ian.stuart@ed.ac.uk>

README.md

@@ -37,7 +37,7 @@ Following the lead of other Jupyter services, it is a `tornado` application.
 
 ## Compatibility
 
-This version is compatible with `nbgrader` 0.5
+This version is compatible with `nbgrader` >= 0.6.2
 
 # Documentation
 
@@ -72,19 +72,19 @@ Nbexchange can be installed as a Helm chart
 
 ## Configuration
 
-| Parameter                                                     | Description                                                                                                                                                                  | Default                                                                                                   |
-| ------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------- |
-| `replicaCount`                                            | Replica count                                                                                                                                                             | 1                                                                                |
-| `image.repository`                                            | Image repository                                                                                                                                                             | `quay.io/noteable/nbexchange`                                                                                |
-| `image.tag`                                                   | Image tag                                                                                                                                                                    | `latest`                                                                                                  |
-| `image.pullPolicy`                                            | Image pull policy                                                                                                                                                            | `IfNotPresent`                                                                                            |
-| `environment`                                              | Environment variables for the application                                                                                                                                          | `{}`                                                                                                      |
-| `service.type`                                                 | Type of Service                                                                                                                                     | `ClusterIP`                                                                                                    |
-| `service.port`                           | Port to expose service under                                                                                                | `9000`                                                                                                    |
-| `resources.requests.cpu`                             | CPU resource requests                      | `200m`                                                                                                        |
-| `resources.requests.memory`                             | Memory resource requests                      | `256Mi`                                                                                                        |
-| `tolerations`                                                 | Pod taint tolerations for deployment                                                                                                                                              | `[]`                                                                                                      |
-| `nodeSelector`                                                | Pod node selector for deployment                                                                                                                                               | `{}`                                                                                                      |
+| Parameter  | Description    | Default |
+| ---------- | -------------- | ------- |
+| `replicaCount` | Replica count | 1  |
+| `image.repository` | Image repository | `quay.io/noteable/nbexchange` |
+| `image.tag` | Image tag | `latest` |
+| `image.pullPolicy` | Image pull policy | `IfNotPresent` |
+| `environment` | Environment variables for the application | `{}` |
+| `service.type` | Type of Service | `ClusterIP` |
+| `service.port` | Port to expose service under | `9000` |
+| `resources.requests.cpu` | CPU resource requests | `200m` |
+| `resources.requests.memory` | Memory resource requests | `256Mi` |
+| `tolerations` | Pod taint tolerations for deployment| `[]` |
+| `nodeSelector` | Pod node selector for deployment | `{}` |
 
 
 ## How to

docs/index.rst

@@ -0,0 +1,18 @@
+Documentation relating to the NBExchange service
+================================================
+
+NbExchange API
+--------------
+
+The documentation for the various API calls are given in ` <api.html>`_ 
+
+It gives the URL calls, the HTTP Methods, and the expected responses
+
+
+Exchange API
+------------
+
+Th `Exchange API calls <nbgrader-exchange-calls.html>`_ documentation describes how
+nbgrader expects to interact with an exchange service.
+
+This is the original documentation for `nbgrader and its exchange service <https://nbgrader.readthedocs.io/en/master/exchange/index.html>`_ page

nbexchange/plugin/fetch_feedback.py

@@ -20,7 +20,7 @@ class ExchangeFetchFeedback(abc.ExchangeFetchFeedback, Exchange):
 
     # where the downloaded files are placed
     def init_src(self):
-        self.src_path = ""
+        pass
 
     # where in the user tree
     def init_dest(self):
@@ -74,17 +74,8 @@ def download(self):
         else:
             self.fail(content.get("note", "could not get feedback"))
 
-    def do_copy(self, src, dest):
-        """Copy the src dir to the dest dir omitting the self.coursedir.ignore globs."""
-        self.download()
-        # shutil.copy(src, dest)
-        # # clear tmp having downloaded file
-        # os.remove(self.src_path)
-
     def copy_files(self):
 
-        self.log.debug(f"Source: {self.src_path}")
         self.log.debug(f"Destination: {self.dest_path}")
-        # self.do_copy(self.src_path, self.dest_path)
         self.download()
         self.log.debug(f"Fetched as: {self.coursedir.notebook_id}")

nbexchange/plugin/release_feedback.py

@@ -18,8 +18,6 @@
 class ExchangeReleaseFeedback(abc.ExchangeReleaseFeedback, Exchange):
 
     src_path = None
-    dest_path = None
-    notebooks = None
 
     # where the downloaded files are placed
     def init_src(self):

nbexchange/tests/test_handlers_base.py

@@ -22,3 +22,7 @@ def test_main_page(self, app):
         r = yield async_requests.get(app.url + "/")
         assert r.status_code == 200
         assert re.search(r"NbExchange", r.text)
+
+    def test_base_location_story(self, app):
+        # Not "/services/nbexchange/", the tests move it
+        assert app.base_storage_location in ["/tmp/exchange/", "/tmp/courses"]

nbexchange/tests/test_handlers_collection.py

@@ -8,6 +8,7 @@
 from nbexchange.handlers.base import BaseHandler
 from nbexchange.tests.utils import (
     async_requests,
+    clear_database,
     get_files_dict,
     user_brobbere_student,
     user_kiz_instructor,
@@ -31,7 +32,7 @@ def test_post_collection_is_501(app):
 
 # subscribed user makes no difference (501, because we've hard-coded it)
 @pytest.mark.gen_test
-def test_post_collection_is_501_even_authenticaated(app):
+def test_post_collection_is_501_even_authenticaated(app, clear_database):
     with patch.object(
         BaseHandler, "get_current_user", return_value=user_kiz_instructor
     ):
@@ -43,7 +44,7 @@ def test_post_collection_is_501_even_authenticaated(app):
 
 # require authenticated user
 @pytest.mark.gen_test
-def test_get_collection_requires_authentication(app):
+def test_get_collection_requires_authentication(app, clear_database):
     r = yield async_requests.get(app.url + "/collection")
     assert r.status_code == 403
 
@@ -69,7 +70,7 @@ def test_get_collection_requires_parameters(app):
 # (needs to be fetched before it can be submitted )
 # (needs to be released before it can be fetched )
 @pytest.mark.gen_test
-def test_get_collection_catches_missing_path(app):
+def test_get_collection_catches_missing_path(app, clear_database):
     with patch.object(
         BaseHandler, "get_current_user", return_value=user_kiz_instructor
     ):
@@ -113,7 +114,7 @@ def test_get_collection_catches_missing_path(app):
 # (needs to be fetched before it can be submitted )
 # (needs to be released before it can be fetched )
 @pytest.mark.gen_test
-def test_get_collection_catches_missing_assignment(app):
+def test_get_collection_catches_missing_assignment(app, clear_database):
     with patch.object(
         BaseHandler, "get_current_user", return_value=user_kiz_instructor
     ):
@@ -157,7 +158,7 @@ def test_get_collection_catches_missing_assignment(app):
 # (needs to be fetched before it can be submitted )
 # (needs to be released before it can be fetched )
 @pytest.mark.gen_test
-def test_get_collection_catches_missing_course(app):
+def test_get_collection_catches_missing_course(app, clear_database):
     with patch.object(
         BaseHandler, "get_current_user", return_value=user_kiz_instructor
     ):
@@ -201,7 +202,7 @@ def test_get_collection_catches_missing_course(app):
 # (needs to be fetched before it can be submitted )
 # (needs to be released before it can be fetched )
 @pytest.mark.gen_test
-def test_get_collection_checks_for_user_subscription(app):
+def test_get_collection_checks_for_user_subscription(app, clear_database):
     with patch.object(
         BaseHandler, "get_current_user", return_value=user_kiz_instructor
     ):
@@ -240,7 +241,7 @@ def test_get_collection_checks_for_user_subscription(app):
 # Has all three params, student can't collect (note this is hard-coded params, as students can list items available for collection)
 # (needs to be released to register the assignment )
 @pytest.mark.gen_test
-def test_get_collection_check_catches_student_role(app):
+def test_get_collection_check_catches_student_role(app, clear_database):
     with patch.object(
         BaseHandler, "get_current_user", return_value=user_kiz_instructor
     ):
@@ -266,7 +267,7 @@ def test_get_collection_check_catches_student_role(app):
 # (needs to be fetched before it can be submitted )
 # (needs to be released before it can be fetched )
 @pytest.mark.gen_test
-def test_get_collection_confirm_instructor_does_download(app):
+def test_get_collection_confirm_instructor_does_download(app, clear_database):
     with patch.object(
         BaseHandler, "get_current_user", return_value=user_kiz_instructor
     ):
@@ -303,7 +304,7 @@ def test_get_collection_confirm_instructor_does_download(app):
 
 # Confirm that multiple submissions are listed
 @pytest.mark.gen_test
-async def test_collection_actions_show_correctly(app):
+async def test_collection_actions_show_correctly(app, clear_database):
     with patch.object(
         BaseHandler, "get_current_user", return_value=user_kiz_instructor
     ):
@@ -409,7 +410,7 @@ async def test_collection_actions_show_correctly(app):
 # (needs to be fetched before it can be submitted )
 # (needs to be released before it can be fetched )
 @pytest.mark.gen_test
-def test_get_collection_path_is_incorrect(app):
+def test_get_collection_path_is_incorrect(app, clear_database):
     with patch.object(
         BaseHandler, "get_current_user", return_value=user_kiz_instructor
     ):
@@ -449,7 +450,7 @@ def test_get_collection_path_is_incorrect(app):
 # (needs to be fetched before it can be submitted )
 # (needs to be released before it can be fetched )
 @pytest.mark.gen_test
-def test_get_collection_with_a_blank_feedback_path_injected(app):
+def test_get_collection_with_a_blank_feedback_path_injected(app, clear_database):
     with patch.object(
         BaseHandler, "get_current_user", return_value=user_kiz_instructor
     ):

nbexchange/tests/test_handlers_delete.py

@@ -7,6 +7,7 @@
 from nbexchange.handlers.base import BaseHandler
 from nbexchange.tests.utils import (
     async_requests,
+    clear_database,
     get_files_dict,
     user_kiz_instructor,
     user_kiz_student,
@@ -16,6 +17,18 @@
 logger = logging.getLogger(__file__)
 logger.setLevel(logging.ERROR)
 
+#################################
+#
+# Very Important Note
+#
+# The `clear_database` fixture removed all database records.
+# In this suite of tests, we do that FOR EVERY TEST
+# This means that every single test is run in isolation, and therefore will need to have the full Release, Fetch,
+#   Submit steps done before the collection can be tested.
+# (On the plus side, adding or changing a test will no longer affect those below)
+#
+#################################
+
 ##### DELETE /assignment (delete or purge assignment) ######
 
 # require authenticated user (404 because the bounce to login fails)
@@ -31,7 +44,7 @@ def test_delete_needs_user(app):
 
 # Requires both params (none)
 @pytest.mark.gen_test
-def test_delete_needs_both_params(app):
+def test_delete_needs_both_params(app, clear_database):
     with patch.object(
         BaseHandler, "get_current_user", return_value=user_kiz_instructor
     ):
@@ -46,7 +59,7 @@ def test_delete_needs_both_params(app):
 
 # Requires both params (just course)
 @pytest.mark.gen_test
-def test_delete_needs_assignment(app):
+def test_delete_needs_assignment(app, clear_database):
     with patch.object(
         BaseHandler, "get_current_user", return_value=user_kiz_instructor
     ):
@@ -62,7 +75,7 @@ def test_delete_needs_assignment(app):
 
 # Requires both params (just assignment)
 @pytest.mark.gen_test
-def test_delete_needs_course(app):
+def test_delete_needs_course(app, clear_database):
     with patch.object(
         BaseHandler, "get_current_user", return_value=user_kiz_instructor
     ):
@@ -79,7 +92,7 @@ def test_delete_needs_course(app):
 # Student cannot release
 # Note we have to use a user who's NEVER been an instructor on the course
 @pytest.mark.gen_test
-def test_delete_student_blocked(app):
+def test_delete_student_blocked(app, clear_database):
     with patch.object(BaseHandler, "get_current_user", return_value=user_zik_student):
         r = yield async_requests.get(app.url + "/assignments?course_id=course_2")
         r = yield async_requests.delete(
@@ -93,7 +106,7 @@ def test_delete_student_blocked(app):
 
 # Instructor, wrong course, cannot release
 @pytest.mark.gen_test
-def test_delete_wrong_course_blocked(app):
+def test_delete_wrong_course_blocked(app, clear_database):
     with patch.object(
         BaseHandler, "get_current_user", return_value=user_kiz_instructor
     ):
@@ -108,7 +121,7 @@ def test_delete_wrong_course_blocked(app):
 
 # instructor can delete
 @pytest.mark.gen_test
-def test_delete_instructor_delete(app):
+def test_delete_instructor_delete(app, clear_database):
     with patch.object(
         BaseHandler, "get_current_user", return_value=user_kiz_instructor
     ):
@@ -131,7 +144,7 @@ def test_delete_instructor_delete(app):
 
 # instructor can purge
 @pytest.mark.gen_test
-def test_delete_instructor_purge(app):
+def test_delete_instructor_purge(app, clear_database):
     with patch.object(
         BaseHandler, "get_current_user", return_value=user_kiz_instructor
     ):
@@ -155,7 +168,7 @@ def test_delete_instructor_purge(app):
 
 # Instructor, wrong course, cannot delete
 @pytest.mark.gen_test
-def test_delete_wrong_course_blocked(app):
+def test_delete_wrong_course_blocked(app, clear_database):
     with patch.object(
         BaseHandler, "get_current_user", return_value=user_kiz_instructor
     ):
@@ -174,7 +187,7 @@ def test_delete_wrong_course_blocked(app):
 
 # instructor releasing - Picks up the first attribute if more than 1 (wrong course)
 @pytest.mark.gen_test
-def test_delete_multiple_courses_listed_first_wrong_blocked(app):
+def test_delete_multiple_courses_listed_first_wrong_blocked(app, clear_database):
     with patch.object(
         BaseHandler, "get_current_user", return_value=user_kiz_instructor
     ):
@@ -195,7 +208,7 @@ def test_delete_multiple_courses_listed_first_wrong_blocked(app):
 
 # instructor releasing - Picks up the first attribute if more than 1 (wrong course)
 @pytest.mark.gen_test
-def test_delete_multiple_courses_listed_first_right_passes(app):
+def test_delete_multiple_courses_listed_first_right_passes(app, clear_database):
     with patch.object(
         BaseHandler, "get_current_user", return_value=user_kiz_instructor
     ):
@@ -218,10 +231,8 @@ def test_delete_multiple_courses_listed_first_right_passes(app):
 
 
 # confirm unreleased does not show in list
-# Skipping because it fails in the group test, but fine when just the 1 file is run
-@pytest.mark.skip
 @pytest.mark.gen_test
-def test_delete_assignment10(app):
+def test_delete_assignment10(app, clear_database):
     with patch.object(
         BaseHandler, "get_current_user", return_value=user_kiz_instructor
     ):