Merge PR #93 - fix NsJail tests

A test is still broken, but it's due to a bug in the code being
tested rather than in the test itself. It'll be fixed separately.

4 files changed, 39 insertions, 30 deletions

diff --git a/.github/workflows/lint-test-build-push.yaml b/.github/workflows/lint-test-build-push.yaml
index 338e301..e86096f 100644
--- a/.github/workflows/lint-test-build-push.yaml
+++ b/.github/workflows/lint-test-build-push.yaml
@@ -73,6 +73,7 @@ jobs:
           push: false
           load: true
           target: venv
+          build-args: DEV=1
           cache-from: |
             type=local,src=/tmp/.buildx-cache
             ghcr.io/python-discord/snekbox-base:latest
@@ -85,12 +86,6 @@ jobs:
           export IMAGE_SUFFIX='-venv:${{ steps.sha_tag.outputs.tag }}'
           docker-compose up --no-build -d
 
-      # One of the unit tests needs to import numpy.
-      - name: Install dependencies
-        run: >-
-          docker exec snekbox_dev /bin/bash -c
-          'pipenv install --system --deploy --dev && pip install numpy'
-
       # Required by pre-commit.
       - name: Install git
         run: >-
@@ -120,12 +115,15 @@ jobs:
         run: sudo swapoff -a
 
       # Run unittests and generate coverage report in the container
-      - name: Run unit tests and generate coverage report
+      - name: Run unit tests
         id: run_tests
         run: |
           echo '::set-output name=started::true'
-          cmd='coverage run -m unittest; coverage report -m'
-          docker exec snekbox_dev /bin/bash -c "${cmd}"
+          docker exec snekbox_dev /bin/bash -c 'coverage run -m unittest'
+
+      - name: Generate coverage report
+        if: always() && steps.run_tests.outputs.started == 'true'
+        run: docker exec snekbox_dev /bin/bash -c 'coverage report -m'
 
       # Set-up a Python version to process the coverage reports
       # Note: This step runs even if the test step failed to make
diff --git a/Dockerfile b/Dockerfile
index 2aee11b..7e8d4ba 100644
--- a/Dockerfile
+++ b/Dockerfile
@@ -44,13 +44,24 @@ RUN chmod +x /usr/sbin/nsjail
 
 # ------------------------------------------------------------------------------
 FROM base as venv
-ARG DEV
 
 COPY Pipfile Pipfile.lock /snekbox/
 WORKDIR /snekbox
 
-# Install to the default user site since PIP_USER is set.
-RUN pipenv install --deploy --system ${DEV:+--dev}
+# Pipenv installs to the default user site since PIP_USER is set.
+RUN pipenv install --deploy --system
+
+# This must come after the first pipenv command! From the docs:
+# All RUN instructions following an ARG instruction use the ARG variable
+# implicitly (as an environment variable), thus can cause a cache miss.
+ARG DEV
+
+# Install numpy when in dev mode; one of the unit tests needs it.
+RUN if [ -n "${DEV}" ]; \
+    then \
+        pipenv install --deploy --system --dev \
+        && PYTHONUSERBASE=/snekbox/user_base pip install numpy~=1.19; \
+    fi
 
 # At the end to avoid re-installing dependencies when only a config changes.
 # It's in the venv image because the final image is not used during development.
diff --git a/snekbox/nsjail.py b/snekbox/nsjail.py
index 06c3154..c7087ad 100644
--- a/snekbox/nsjail.py
+++ b/snekbox/nsjail.py
@@ -154,20 +154,19 @@ class NsJail:
         output_size = 0
         output = []
 
-        # We'll consume STDOUT as long as the NsJail subprocess is running.
-        while nsjail.poll() is None:
-            chars = nsjail.stdout.read(READ_CHUNK_SIZE)
-            output_size += sys.getsizeof(chars)
-            output.append(chars)
-
-            if output_size > OUTPUT_MAX:
-                # Terminate the NsJail subprocess with SIGKILL.
-                log.info("Output exceeded the output limit, sending SIGKILL to NsJail.")
-                nsjail.kill()
-                break
-
-        # Ensure that we wait for the NsJail subprocess to terminate.
-        nsjail.wait()
+        # Context manager will wait for process to terminate and close file descriptors.
+        with nsjail:
+            # We'll consume STDOUT as long as the NsJail subprocess is running.
+            while nsjail.poll() is None:
+                chars = nsjail.stdout.read(READ_CHUNK_SIZE)
+                output_size += sys.getsizeof(chars)
+                output.append(chars)
+
+                if output_size > OUTPUT_MAX:
+                    # Terminate the NsJail subprocess with SIGKILL.
+                    log.info("Output exceeded the output limit, sending SIGKILL to NsJail.")
+                    nsjail.kill()
+                    break
 
         return "".join(output)
 
diff --git a/tests/test_nsjail.py b/tests/test_nsjail.py
index 40c27f9..c0e43b6 100644
--- a/tests/test_nsjail.py
+++ b/tests/test_nsjail.py
@@ -5,7 +5,7 @@ import unittest
 import unittest.mock
 from textwrap import dedent
 
-from snekbox.nsjail import MEM_MAX, NsJail, OUTPUT_MAX, READ_CHUNK_SIZE
+from snekbox.nsjail import NsJail, OUTPUT_MAX, READ_CHUNK_SIZE
 
 
 class NsJailTests(unittest.TestCase):
@@ -13,8 +13,8 @@ class NsJailTests(unittest.TestCase):
         super().setUp()
 
         self.nsjail = NsJail()
-        self.nsjail.DEBUG = False
         self.logger = logging.getLogger("snekbox.nsjail")
+        self.logger.setLevel(logging.WARNING)
 
     def test_print_returns_0(self):
         result = self.nsjail.python3("print('test')")
@@ -39,7 +39,7 @@ class NsJailTests(unittest.TestCase):
     def test_memory_returns_137(self):
         # Add a kilobyte just to be safe.
         code = dedent(f"""
-            x = ' ' * {MEM_MAX + 1000}
+            x = ' ' * {self.nsjail.config.cgroup_mem_max + 1000}
         """).strip()
 
         result = self.nsjail.python3(code)
@@ -100,6 +100,7 @@ class NsJailTests(unittest.TestCase):
         self.assertEqual(result.stdout, "ValueError: embedded null byte")
         self.assertEqual(result.stderr, None)
 
+    @unittest.mock.patch("snekbox.nsjail.DEBUG", new=False)
     def test_log_parser(self):
         log_lines = (
             "[D][2019-06-22T20:07:00+0000][16] void foo::bar()():100 This is a debug message.",
@@ -191,7 +192,7 @@ class NsJailTests(unittest.TestCase):
         chunk = "a" * READ_CHUNK_SIZE
         expected_chunks = OUTPUT_MAX // sys.getsizeof(chunk) + 1
 
-        nsjail_subprocess = unittest.mock.Mock()
+        nsjail_subprocess = unittest.mock.MagicMock()
 
         # Go 10 chunks over to make sure we exceed the limit
         nsjail_subprocess.stdout = io.StringIO((expected_chunks + 10) * chunk)