Clean up the tests per Janet's remarks

apps/cli/executables/delivery/test/test_api.py

@@ -9,6 +9,11 @@ from delivery.deliverer import LocalDestination
 
 @pytest.fixture
 def dest_dir(tmpdir) -> pathlib.Path:
+    """
+    Generate a destination directory.
+    :param tmpdir:  the tmpdir fixture we depend on
+    :return:  path to the destination directory
+    """
     # generate the destination directory
     dest_dir = pathlib.Path(tmpdir) / "after"
     dest_dir.mkdir()
@@ -17,6 +22,11 @@ def dest_dir(tmpdir) -> pathlib.Path:
 
 @pytest.fixture
 def file_to_deliver(tmpdir) -> pathlib.Path:
+    """
+    Generate some files in a test directory for delivery.
+    :param tmpdir: pytest fixture for the temporary directory
+    :return: a file in the test directory
+    """
     # rewrap the tmpdir as a pathlib.Path instance
     tmpdir = pathlib.Path(tmpdir)
 

apps/cli/executables/delivery/test/test_cli.py

 # Testing the CLI
 import filecmp
-import os
 import pathlib
 import shutil
 import tarfile
@@ -10,59 +9,88 @@ from delivery.context import DeliveryContext
 from delivery.delivery import Delivery, main
 
 
-def test_local_rawdata_no_tar(tmpdir_factory):
+def verify_extracted_directory(
+    subdirectory: str,
+    tar_path: pathlib.Path,
+    extraction_target: pathlib.Path,
+    original_data_path: str,
+):
     """
-    Test that a directory in the source is copied to a directory in the destination in the manner expected.
+    Verify that an extracted directory has the same contents as the supplied temporary directory.
+    Useful for testing tar-related functionality
+
+    :param subdirectory:       subdirectory to look for inside extraction area
+    :param tar_path:           path to the tarfile to examine
+    :param extraction_target:  location to extract to
+    :param original_data_path: location of the original files to compare to
     :return:
     """
+
+    # is it actually a tar?
+    assert tarfile.is_tarfile(tar_path)
+
+    # let's unpack it
+    shutil.unpack_archive(tar_path, extraction_target / "extracted")
+
+    # did it output what we expect?
+    assert (extraction_target / "extracted" / subdirectory).exists()
+
+    # compare the extracted results with the source
+    assert_directories_are_same(
+        extraction_target / "extracted" / subdirectory, (original_data_path + subdirectory)
+    )
+
+
+def assert_directories_are_same(left, right):
+    """
+    Check that the contents of two directories are the same as far as we care
+    :param left:
+    :param right:
+    :return:
+    """
+    compare_dirs = filecmp.dircmp(left, right)
+
+    # did the comparison report they are the same
+    assert len(compare_dirs.left_only) == 0
+    assert len(compare_dirs.right_only) == 0
+    assert len(compare_dirs.funny_files) == 0
+
+
+def test_local_rawdata_no_tar(tmpdir_factory):
+    """
+    Test that local delivery works without tar (the simplest case)
+    """
     temp_directory = str(tmpdir_factory.mktemp("test_basic_rawdata_no_tar"))
     test_data_path = "../../../../shared/workspaces/test/test_data/spool/724126739/"
     eb_name = "17A-109.sb33151331.eb33786546.57892.65940042824"
     main(["-r", "-l", temp_directory, test_data_path])
     # compare the source and destination
-    compare_dirs = filecmp.dircmp(temp_directory + "/" + eb_name, (test_data_path + eb_name))
-    # did the comparison report they are the same
-    assert len(compare_dirs.left_only) == 0
-    assert len(compare_dirs.right_only) == 0
-    assert len(compare_dirs.funny_files) == 0
+    assert_directories_are_same(temp_directory + "/" + eb_name, (test_data_path + eb_name))
 
 
 def test_local_rawdata_with_tar(tmpdir_factory):
-    temp_directory = str(tmpdir_factory.mktemp("test_basic_rawdata_with_tar"))
+    """
+    Test that local delivery works with tar
+    """
+    temp_directory = pathlib.Path(tmpdir_factory.mktemp("test_basic_rawdata_with_tar"))
     test_data_path = "../../../../shared/workspaces/test/test_data/spool/724126739/"
-    main(["-r", "-t", "-l", temp_directory, test_data_path])
+    main(["-r", "-t", "-l", str(temp_directory), test_data_path])
     eb_name = "17A-109.sb33151331.eb33786546.57892.65940042824"
-    tar_path = temp_directory + "/17A-109.sb33151331.eb33786546.57892.65940042824.tar"
+    tar_path = temp_directory / "17A-109.sb33151331.eb33786546.57892.65940042824.tar"
 
     # does a tar exist where we think
-    assert os.path.exists(tar_path)
+    assert tar_path.exists()
 
     # do we only have it and the SHA1SUMS
-    assert len(os.listdir(temp_directory)) == 2
-
-    # is it actually a tar
-    assert tarfile.is_tarfile(tar_path)
+    assert len(list(temp_directory.iterdir())) == 2
 
-    # lets unpack it
-    shutil.unpack_archive(tar_path, temp_directory + "/extracted")
-
-    # did it output what we expect
-    assert os.path.exists(temp_directory + "/extracted/" + eb_name)
-
-    # compare the extracted results with the source
-    compare_dirs = filecmp.dircmp(
-        temp_directory + "/extracted/" + eb_name, (test_data_path + eb_name)
-    )
-    # is the source and extracted the same
-    assert len(compare_dirs.left_only) == 0
-    assert len(compare_dirs.right_only) == 0
-    assert len(compare_dirs.funny_files) == 0
+    verify_extracted_directory(eb_name, tar_path, temp_directory, test_data_path)
 
 
 # @pytest.mark.skip(reason="Test needs more dev time")
 def test_web_rawdata_no_tar(tmpdir_factory):
     """
-    Test that a directory in the source is copied to a directory in the destination in the manner expected.
+    Test that delivery works to a web destination without tar
     """
     temp_directory = pathlib.Path(tmpdir_factory.mktemp("test_web_rawdata_no_tar"))
     test_data_path = "../../../../shared/workspaces/test/test_data/spool/724126739/"
@@ -78,15 +106,13 @@ def test_web_rawdata_no_tar(tmpdir_factory):
     actual_delivery_dir = temp_directory / destination_url.lstrip("http://testing")
 
     # compare the source and destination
-    compare_dirs = filecmp.dircmp(actual_delivery_dir / eb_name, f"{test_data_path}{eb_name}")
-
-    # did the comparison report they are the same
-    assert len(compare_dirs.left_only) == 0
-    assert len(compare_dirs.right_only) == 0
-    assert len(compare_dirs.funny_files) == 0
+    assert_directories_are_same(actual_delivery_dir / eb_name, f"{test_data_path}{eb_name}")
 
 
 def test_web_rawdata_with_tar(tmpdir_factory):
+    """
+    Test that delivery works to a web destination with tar
+    """
     temp_directory = pathlib.Path(tmpdir_factory.mktemp("test_web_rawdata_with_tar"))
     test_data_path = "../../../../shared/workspaces/test/test_data/spool/724126739/"
     test_context = DeliveryContext.parse_commandline(["-r", "-t", test_data_path])
@@ -108,21 +134,4 @@ def test_web_rawdata_with_tar(tmpdir_factory):
     # is it the only thing there (did cleanup work)
     assert len(list(actual_delivery_dir.iterdir())) == 3
 
-    # is it actually a tar
-    assert tarfile.is_tarfile(tar_path)
-
-    # lets unpack it
-    shutil.unpack_archive(tar_path, temp_directory / "extracted")
-
-    # did it output what we expect
-    assert (temp_directory / "extracted" / eb_name).exists()
-
-    # compare the extracted results with the source
-    compare_dirs = filecmp.dircmp(
-        temp_directory / "extracted" / eb_name, (test_data_path + eb_name)
-    )
-
-    # is the source and extracted the same
-    assert len(compare_dirs.left_only) == 0
-    assert len(compare_dirs.right_only) == 0
-    assert len(compare_dirs.funny_files) == 0
+    verify_extracted_directory(eb_name, tar_path, temp_directory, test_data_path)