Add windows and macos python 3.6 unit test runners; make filesystem operations safer on Windows

.github/workflows/tox.yml

@@ -83,3 +83,52 @@ jobs:
         key: ${{ runner.os }}-tox-py${{ matrix.python-version }}-${{ hashFiles('requirements/*.txt') }}
     - name: Test with tox
       run: tox -e postgres
+  macos:
+    name: Python unit tests on Mac OS
+    needs: pre_job
+    if: ${{ needs.pre_job.outputs.should_skip != 'true' }}
+    runs-on: macos-latest
+    strategy:
+      max-parallel: 5
+      matrix:
+        python-version: [3.6]
+
+    steps:
+    - uses: actions/checkout@v2
+    - name: Set up Python ${{ matrix.python-version }}
+      uses: actions/setup-python@v2
+      with:
+        python-version: ${{ matrix.python-version }}
+    - name: Install tox
+      run: |
+        python -m pip install --upgrade pip
+        pip install tox
+    - name: tox env cache
+      uses: actions/cache@v2
+      with:
+        path: ${{ github.workspace }}/.tox/py${{ matrix.python-version }}
+        key: ${{ runner.os }}-tox-py${{ matrix.python-version }}-${{ hashFiles('requirements/*.txt') }}
+    - name: Test with tox
+      run: tox -e py${{ matrix.python-version }}
+  windows:
+    name: Python unit tests on Windows Server
+    needs: pre_job
+    if: ${{ needs.pre_job.outputs.should_skip != 'true' }}
+    runs-on: windows-latest
+    strategy:
+      max-parallel: 5
+      matrix:
+        python-version: [3.6]
+
+    steps:
+    - uses: actions/checkout@v2
+    - name: Set up Python ${{ matrix.python-version }}
+      uses: actions/setup-python@v2
+      with:
+        python-version: ${{ matrix.python-version }}
+    - name: Install tox
+      run: |
+        python -m pip install --upgrade pip
+        pip install tox
+    - name: Test with tox
+      run: tox -e py${{ matrix.python-version }}

kolibri/conftest.py

@@ -15,4 +15,8 @@ def global_fixture():
         os.mkdir(os.path.join(TEMP_KOLIBRI_HOME, "content"))
     yield  # wait until the test ended
     if os.path.exists(TEMP_KOLIBRI_HOME):
-        shutil.rmtree(TEMP_KOLIBRI_HOME)
+        try:
+            shutil.rmtree(TEMP_KOLIBRI_HOME)
+        except OSError:
+            # Don't fail a test just because we failed to cleanup
+            pass

kolibri/core/auth/test/sync_utils.py

@@ -4,12 +4,12 @@
 import os
 import shutil
 import socket
+import subprocess
 import tempfile
 import time
 import uuid
 
 import requests
-import sh
 from django.db import connection
 from django.db import connections
 from django.utils.functional import wraps
@@ -49,8 +49,9 @@ def __init__(
     def start(self, pre_migrate=True):
         if pre_migrate:
             KolibriServer.get_pre_migrated_database(self.env["KOLIBRI_HOME"])
-        self._instance = sh.kolibri.start(
-            port=self.port, foreground=True, _bg=True, _env=self.env
+        self._instance = subprocess.Popen(
+            ["kolibri start", "--port {}".format(self.port), "--foreground"],
+            env=self.env,
         )
         self._wait_for_server_start()
 
@@ -68,20 +69,17 @@ def _wait_for_server_start(self, timeout=20):
 
     def kill(self):
         try:
-            self._instance.process.kill()
+            self._instance.kill()
             shutil.rmtree(self.env["KOLIBRI_HOME"])
         except OSError:
             pass
 
-    def manage(self, *args, **kwargs):
-        sh.kolibri.manage(*args, _env=self.env, **kwargs)
-
     @classmethod
     def get_pre_migrated_database(cls, home_path):
         if not cls._pre_migrated_db_dir:
             # if no pre-migrated db files, then create them
             server = cls(autostart=False)
-            server.manage("migrate")
+            subprocess.run(["kolibri manage migrate"], env=server.env)
             # put pre-migrated files in temp directory
             cls._pre_migrated_db_dir = tempfile.mkdtemp()
             for f in glob.glob(os.path.join(server.env["KOLIBRI_HOME"], "db.sqlite3*")):

kolibri/core/auth/test/test_user_import.py

@@ -142,6 +142,7 @@ def test_device_not_setup(self):
         csvfile, csvpath = tempfile.mkstemp(suffix="csv")
         with self.assertRaisesRegexp(CommandError, "No default facility exists"):
             call_command("importusers", csvpath)
+        os.close(csvfile)
         os.remove(csvpath)
 
 
@@ -160,6 +161,7 @@ def setUp(self):
 
     def tearDown(self):
         FacilityUser.objects.exclude(username=self.superuser.username).delete()
+        os.close(self.csvfile)
         os.remove(self.csvpath)
 
     def importFromRows(self, *args):

kolibri/core/content/test/test_import_export.py

@@ -79,7 +79,8 @@ def test_remote_cancel_during_transfer(
         start_progress_mock,
         import_channel_mock,
     ):
-        local_path = tempfile.mkstemp()[1]
+        fd, local_path = tempfile.mkstemp()
+        os.close(fd)
         local_path_mock.return_value = local_path
         remote_path_mock.return_value = "notest"
         FileDownloadMock.return_value.__iter__.side_effect = TransferCanceled()
@@ -116,8 +117,10 @@ def test_local_cancel_during_transfer(
         start_progress_mock,
         import_channel_mock,
     ):
-        local_dest_path = tempfile.mkstemp()[1]
-        local_src_path = tempfile.mkstemp()[1]
+        fd1, local_dest_path = tempfile.mkstemp()
+        fd2, local_src_path = tempfile.mkstemp()
+        os.close(fd1)
+        os.close(fd2)
         local_path_mock.side_effect = [local_dest_path, local_src_path]
         FileCopyMock.return_value.__iter__.side_effect = TransferCanceled()
         call_command("importchannel", "disk", self.the_channel_id, tempfile.mkdtemp())
@@ -214,7 +217,8 @@ def test_remote_successful_import_clears_stats_cache(
         start_progress_mock,
         import_channel_mock,
     ):
-        local_path = tempfile.mkstemp()[1]
+        fd, local_path = tempfile.mkstemp()
+        os.close(fd)
         local_path_mock.return_value = local_path
         remote_path_mock.return_value = "notest"
         FileDownloadMock.return_value.__iter__.return_value = ["one", "two", "three"]
@@ -295,7 +299,8 @@ def test_remote_cancel_during_transfer(
         channel_list_status_mock,
     ):
         # If transfer is cancelled during transfer of first file
-        local_path = tempfile.mkstemp()[1]
+        fd, local_path = tempfile.mkstemp()
+        os.close(fd)
         local_path_mock.return_value = local_path
         remote_path_mock.return_value = "notest"
         # Mock this __iter__ so that the filetransfer can be looped over
@@ -345,8 +350,10 @@ def test_remote_cancel_after_file_copy_file_not_deleted(
         channel_list_status_mock,
     ):
         # If transfer is cancelled after transfer of first file
-        local_path_1 = tempfile.mkstemp()[1]
-        local_path_2 = tempfile.mkstemp()[1]
+        fd1, local_path_1 = tempfile.mkstemp()
+        fd2, local_path_2 = tempfile.mkstemp()
+        os.close(fd1)
+        os.close(fd2)
         with open(local_path_1, "w") as f:
             f.write("a")
         local_path_mock.side_effect = [local_path_1, local_path_2]
@@ -410,8 +417,10 @@ def test_local_cancel_during_transfer(
         channel_list_status_mock,
     ):
         # Local version of test above
-        local_dest_path = tempfile.mkstemp()[1]
-        local_src_path = tempfile.mkstemp()[1]
+        fd1, local_dest_path = tempfile.mkstemp()
+        fd2, local_src_path = tempfile.mkstemp()
+        os.close(fd1)
+        os.close(fd2)
         local_path_mock.side_effect = [local_dest_path, local_src_path]
         FileCopyMock.return_value.__iter__.side_effect = TransferCanceled()
         get_import_export_mock.return_value = (1, list(LocalFile.objects.all()), 10)
@@ -480,9 +489,12 @@ def test_remote_import_httperror_404(
         get_import_export_mock,
         channel_list_status_mock,
     ):
-        local_dest_path_1 = tempfile.mkstemp()[1]
-        local_dest_path_2 = tempfile.mkstemp()[1]
-        local_dest_path_3 = tempfile.mkstemp()[1]
+        fd1, local_dest_path_1 = tempfile.mkstemp()
+        fd2, local_dest_path_2 = tempfile.mkstemp()
+        fd3, local_dest_path_3 = tempfile.mkstemp()
+        os.close(fd1)
+        os.close(fd2)
+        os.close(fd3)
         path_mock.side_effect = [
             local_dest_path_1,
             local_dest_path_2,
@@ -654,7 +666,8 @@ def test_local_import_oserror_dne(
         get_import_export_mock,
         channel_list_status_mock,
     ):
-        dest_path = tempfile.mkstemp()[1]
+        fd, dest_path = tempfile.mkstemp()
+        os.close(fd)
         path_mock.side_effect = [dest_path, "/test/dne"]
         LocalFile.objects.filter(
             files__contentnode__channel_id=self.the_channel_id
@@ -678,7 +691,8 @@ def test_local_import_oserror_permission_denied(
         get_import_export_mock,
         channel_list_status_mock,
     ):
-        dest_path = tempfile.mkstemp()[1]
+        fd, dest_path = tempfile.mkstemp()
+        os.close(fd)
         path_mock.side_effect = [dest_path, "/test/dne"]
         getsize_mock.side_effect = ["1", OSError("Permission denied")]
         get_import_export_mock.return_value = (1, [LocalFile.objects.first()], 10)
@@ -711,8 +725,10 @@ def test_local_import_source_corrupted(
         get_import_export_mock,
         channel_list_status_mock,
     ):
-        local_src_path = tempfile.mkstemp()[1]
-        local_dest_path = tempfile.mkstemp()[1]
+        fd1, local_dest_path = tempfile.mkstemp()
+        fd2, local_src_path = tempfile.mkstemp()
+        os.close(fd1)
+        os.close(fd2)
         LocalFile.objects.filter(
             files__contentnode="32a941fb77c2576e8f6b294cde4c3b0c"
         ).update(file_size=1)
@@ -770,7 +786,8 @@ def test_local_import_source_corrupted_full_progress(
         with open(local_src_path, "w") as f:
             f.write("This is just a test")
         expected_file_size = 10000
-        local_dest_path = tempfile.mkstemp()[1]
+        fd, local_dest_path = tempfile.mkstemp()
+        os.close(fd)
         os.remove(local_dest_path)
         # Delete all but one file associated with ContentNode to reduce need for mocking
         files = ContentNode.objects.get(
@@ -829,8 +846,10 @@ def test_remote_import_source_corrupted(
         get_import_export_mock,
         channel_list_status_mock,
     ):
-        dest_path_1 = tempfile.mkstemp()[1]
-        dest_path_2 = tempfile.mkstemp()[1]
+        fd1, dest_path_1 = tempfile.mkstemp()
+        fd2, dest_path_2 = tempfile.mkstemp()
+        os.close(fd1)
+        os.close(fd2)
         path_mock.side_effect = [dest_path_1, dest_path_2]
         LocalFile.objects.filter(pk="6bdfea4a01830fdd4a585181c0b8068c").update(
             file_size=2201062
@@ -883,8 +902,10 @@ def test_remote_import_full_import(
         get_import_export_mock,
         channel_list_status_mock,
     ):
-        dest_path_1 = tempfile.mkstemp()[1]
-        dest_path_2 = tempfile.mkstemp()[1]
+        fd1, dest_path_1 = tempfile.mkstemp()
+        fd2, dest_path_2 = tempfile.mkstemp()
+        os.close(fd1)
+        os.close(fd2)
         path_mock.side_effect = [dest_path_1, dest_path_2]
         LocalFile.objects.filter(pk="6bdfea4a01830fdd4a585181c0b8068c").update(
             file_size=2201062
@@ -989,8 +1010,10 @@ def test_cancel_during_transfer(
         start_progress_mock,
     ):
         # Make sure we clean up a database file that is canceled during export
-        local_dest_path = tempfile.mkstemp()[1]
-        local_src_path = tempfile.mkstemp()[1]
+        fd1, local_dest_path = tempfile.mkstemp()
+        fd2, local_src_path = tempfile.mkstemp()
+        os.close(fd1)
+        os.close(fd2)
         local_path_mock.side_effect = [local_src_path, local_dest_path]
         FileCopyMock.return_value.__iter__.side_effect = TransferCanceled()
         call_command("exportchannel", self.the_channel_id, local_dest_path)
@@ -1055,8 +1078,10 @@ def test_local_cancel_during_transfer(
         get_import_export_mock,
     ):
         # Make sure we cancel during transfer
-        local_dest_path = tempfile.mkstemp()[1]
-        local_src_path = tempfile.mkstemp()[1]
+        fd1, local_dest_path = tempfile.mkstemp()
+        fd2, local_src_path = tempfile.mkstemp()
+        os.close(fd1)
+        os.close(fd2)
         local_path_mock.side_effect = [local_src_path, local_dest_path]
         FileCopyMock.return_value.__iter__.side_effect = TransferCanceled()
         get_import_export_mock.return_value = (1, [LocalFile.objects.first()], 10)

kolibri/core/content/test/test_movedirectory.py

@@ -1,9 +1,22 @@
+import os
+import sys
+
 from django.core.management import call_command
 from django.test import TestCase
 from mock import patch
 
 from kolibri.utils.conf import OPTIONS
 
+path_prefix = "C:\\" if sys.platform == "win32" else "/"
+
+success_path = os.path.join(path_prefix, "test", "success")
+
+no_content_path = os.path.join(path_prefix, "test", "content_exists_no")
+
+yes_content_path = os.path.join(path_prefix, "test", "content_exists_yes")
+
+random_content_path = os.path.join(path_prefix, "test", "content_exists_random")
+
 
 @patch("kolibri.core.content.management.commands.content.update_options_file")
 class ContentMoveDirectoryTestCase(TestCase):
@@ -15,16 +28,16 @@ class ContentMoveDirectoryTestCase(TestCase):
     def _path_exists_side_effect(*args):
         if args[0] == OPTIONS["Paths"]["CONTENT_DIR"]:
             return True
-        elif args[0].startswith("/test/success"):
+        elif args[0].startswith(success_path):
             return False
         return True
 
     def _listdir_side_effect(*args):
-        if args[0] == OPTIONS["Paths"]["CONTENT_DIR"] + "/databases":
+        if args[0] == os.path.join(OPTIONS["Paths"]["CONTENT_DIR"], "databases"):
             return ["test.sqlite3"]
-        elif args[0] == OPTIONS["Paths"]["CONTENT_DIR"] + "/storage":
+        elif args[0] == os.path.join(OPTIONS["Paths"]["CONTENT_DIR"], "storage"):
             return ["test.mp3"]
-        elif args[0] == "/test/content_exists_no/databases":
+        elif args[0] == os.path.join(no_content_path, "databases"):
             return ["exists.sqlite3"]
         return []
 
@@ -73,7 +86,7 @@ def test_migrate_while_dest_content_exists_no(
         remove_mock,
         update_mock,
     ):
-        destination = "/test/content_exists_no"
+        destination = no_content_path
         call_command("content", "movedirectory", destination)
         self.assertEqual(copyfile_mock.call_count, 2)
         self.assertEqual(remove_mock.call_count, 2)
@@ -98,7 +111,7 @@ def test_migrate_while_dest_content_exists_yes(
         copy_mock,
         update_mock,
     ):
-        destination = "/test/content_exists_yes"
+        destination = yes_content_path
         call_command("content", "movedirectory", destination)
         copy_mock.assert_called()
         self.assertEqual(remove_mock.call_count, 4)
@@ -149,7 +162,7 @@ def test_migrate_while_dest_dir_dne_success(
         remove_mock,
         update_mock,
     ):
-        destination = "/test/success"
+        destination = success_path
         call_command("content", "movedirectory", destination)
         remove_mock.assert_called()
         mkdir_mock.assert_called()