Improved comments and tests for shed related stuff.

With spelling fixes from @nsoranzo.

Author: jmchilton

planemo/shed/__init__.py

@@ -1,3 +1,4 @@
+"""Abstractions for shed related interactions used by the rest of planemo."""
 from collections import namedtuple
 import contextlib
 import copy
@@ -63,6 +64,8 @@
                                "repositories.")
 INCORRECT_OWNER_MESSAGE = ("Attempting to create a repository with configured "
                            "owner [%s] that does not matche API user [%s].")
+PROBLEM_PROCESSING_REPOSITORY_MESSAGE = "Problem processing repositories, exiting."
+
 # Planemo generated or consumed files that do not need to be uploaded to the
 # tool shed.
 PLANEMO_FILES = [
@@ -153,6 +156,7 @@ def _shed_context_owner(self):
 
 
 def shed_init(ctx, path, **kwds):
+    """Intialize a new shed repository."""
     if not os.path.exists(path):
         os.makedirs(path)
     shed_config_path = os.path.join(path, SHED_CONFIG_NAME)
@@ -185,8 +189,7 @@ def shed_init(ctx, path, **kwds):
 
 
 def install_arg_lists(ctx, paths, **kwds):
-    """ Build a list of install args for resolved repositories.
-    """
+    """Build a list of install args for resolved repositories."""
     shed_context = get_shed_context(ctx, **kwds)
     install_args_list = []
 
@@ -196,7 +199,7 @@ def process_repo(realized_repository):
 
     exit_code = for_each_repository(ctx, process_repo, paths, **kwds)
     if exit_code:
-        raise RuntimeError("Problem processing repositories, exiting.")
+        raise RuntimeError(PROBLEM_PROCESSING_REPOSITORY_MESSAGE)
 
     return install_args_list
 
@@ -241,8 +244,7 @@ def report_non_existent_repository(realized_repository):
 
 
 def upload_repository(ctx, realized_repository, **kwds):
-    """Upload a tool directory as a tarball to a tool shed.
-    """
+    """Upload a tool directory as a tarball to a tool shed."""
     path = realized_repository.path
     tar_path = kwds.get("tar", None)
     if not tar_path:
@@ -261,7 +263,7 @@ def upload_repository(ctx, realized_repository, **kwds):
         return report_non_existent_repository(realized_repository)
 
     if kwds.get("check_diff", False):
-        is_diff = diff_repo(ctx, realized_repository, **kwds)
+        is_diff = diff_repo(ctx, realized_repository, **kwds) != 0
         if not is_diff:
             name = realized_repository.name
             info("Repository [%s] not different, skipping upload." % name)
@@ -295,6 +297,11 @@ def _update_commit_message(ctx, realized_repository, update_kwds, **kwds):
 
 
 def diff_repo(ctx, realized_repository, **kwds):
+    """Compare two repositories (local or remote) and check for differences.
+
+    Returns 0 if and only the repositories are effectively the same
+    given supplied kwds for comparison description.
+    """
     with temp_directory("tool_shed_diff_") as working:
         return _diff_in(ctx, working, realized_repository, **kwds)
 
@@ -321,7 +328,7 @@ def _diff_in(ctx, working, realized_repository, **kwds):
         # $ diff README.rst not_a_file 2&>1 /dev/null; echo $?
         # 2
         return 2
-    info("Diffing repository %s " % realized_repository.name)
+    info("Diffing repository [%s]" % realized_repository.name)
     download_tarball(
         ctx,
         shed_context,

planemo/shed/diff.py

@@ -1,3 +1,8 @@
+"""Utilities for calculating effective repository diffs.
+
+Some intelligence is required because the tool shed updates attributes that it
+is beneficial to ignore.
+"""
 from __future__ import print_function
 
 import os
@@ -8,6 +13,11 @@
 
 
 def diff_and_remove(working, label_a, label_b, f):
+    """Remove tool shed XML files and use a smart XML diff on them.
+
+    Return 0 if and only if the XML content is the sam after stripping
+    attirbutes the tool shed updates.
+    """
     assert label_a != label_b
     special = ["tool_dependencies.xml", "repository_dependencies.xml"]
     deps_diff = 0
@@ -26,6 +36,11 @@ def diff_and_remove(working, label_a, label_b, f):
 
 
 def _shed_diff(file_a, file_b, f=sys.stdout):
+    """Strip attributes the tool shed writes and do smart XML diff.
+
+    Returns 0 if and only if the XML content is the same after stripping
+    ``tool_shed`` and ``changeset_revision`` attributes.
+    """
     xml_a = ElementTree.parse(file_a).getroot()
     xml_b = ElementTree.parse(file_b).getroot()
     _strip_shed_attributes(xml_a)
@@ -46,3 +61,6 @@ def _remove_attribs(xml_element):
     for attrib in ["changeset_revision", "toolshed"]:
         if attrib in xml_element.attrib:
             del xml_element.attrib[attrib]
+
+
+__all__ = ["diff_and_remove"]

planemo/xml/diff.py

@@ -1,5 +1,6 @@
 
 def diff(x1, x2, reporter=None):
+    """Return 0 if and only if the XML has the same content."""
     compare = xml_compare(x1, x2, reporter)
     return_val = 0 if compare else 1
     return return_val

tests/test_shed_diff.py

@@ -4,15 +4,16 @@
 from os.path import join
 from .test_utils import (
     CliShedTestCase,
+    TEST_REPOS_DIR,
 )
 from planemo import io
 import tempfile
-import shutil
 import sys
 import os
 from xml.etree import ElementTree
 from planemo.xml.diff import diff
-from .test_utils import TEST_REPOS_DIR
+
+from .test_shed_upload import update_package_1
 
 DIFF_LINES = [
     "diff -r _workingdir_/related_file _custom_shed_/related_file",
@@ -43,11 +44,7 @@ def test_diff_doesnt_exist(self):
 
     def test_diff_recursive(self):
         with self._isolate_repo("multi_repos_nested") as f:
-            upload_command = [
-                "shed_upload", "-r", "--force_repository_creation"
-            ]
-            upload_command.extend(self._shed_args())
-            self._check_exit_code(upload_command)
+            self._shed_create(recursive=True)
 
             diff_command = ["shed_diff", "-r"]
             diff_command.extend(self._shed_args(read_only=True))
@@ -61,11 +58,7 @@ def test_diff_recursive(self):
 
     def test_shed_diff_raw(self):
         with self._isolate_repo("suite_auto"):
-            upload_command = [
-                "shed_upload", "--force_repository_creation",
-            ]
-            upload_command.extend(self._shed_args())
-            self._check_exit_code(upload_command)
+            self._shed_create()
 
             diff_command = [
                 "shed_diff", "-o", "diff"
@@ -82,37 +75,25 @@ def test_shed_diff_raw(self):
 
     def test_shed_diff_xml_no_diff(self):
         with self._isolate_repo("package_1"):
-            upload_command = [
-                "shed_upload", "--force_repository_creation",
-            ]
-            upload_command.extend(self._shed_args())
-            self._check_exit_code(upload_command)
+            self._shed_create()
+
             diff_command = ["shed_diff"]
             diff_command.extend(self._shed_args(read_only=True))
             self._check_exit_code(diff_command, exit_code=0)
 
     def test_shed_diff_xml_diff(self):
         with self._isolate_repo("package_1") as f:
-            upload_command = [
-                "shed_upload", "--force_repository_creation",
-            ]
-            upload_command.extend(self._shed_args())
-            self._check_exit_code(upload_command)
-            changed_xml = os.path.join(TEST_REPOS_DIR,
-                                       "package_1_changed",
-                                       "tool_dependencies.xml")
-            shutil.copyfile(changed_xml, join(f, "tool_dependencies.xml"))
+            self._shed_create()
+
+            update_package_1(f)
+
             diff_command = ["shed_diff"]
             diff_command.extend(self._shed_args(read_only=True))
             self._check_exit_code(diff_command, exit_code=1)
 
     def test_diff_xunit(self):
         with self._isolate_repo("multi_repos_nested") as f:
-            upload_command = [
-                "shed_upload", "-r", "--force_repository_creation"
-            ]
-            upload_command.extend(self._shed_args())
-            self._check_exit_code(upload_command)
+            self._shed_create(recursive=True)
 
             xunit_report = tempfile.NamedTemporaryFile(delete=False)
             xunit_report.flush()

tests/test_shed_upload.py

@@ -1,12 +1,17 @@
-""" Integration tests for shed_upload, shed_download, and shed_create
+"""Integration tests for shed contents commands.
+
+Specifically, tests for shed_upload, shed_download, and shed_create.
 commands.
 """
 from os.path import exists, join
 import os
 import tarfile
 import shutil
 
-from .test_utils import CliShedTestCase
+from .test_utils import (
+    CliShedTestCase,
+    TEST_REPOS_DIR,
+)
 from planemo.io import shell
 from planemo import git
 
@@ -40,12 +45,15 @@ def test_update_not_exists_update_only(self):
 
     def test_update_with_check_diff(self):
         with self._isolate_repo("single_tool") as f:
+            self._shed_create()
+
+            self._assert_shed_diff(diff=0)
+
             upload_command = [
                 "shed_update", "--force_repository_creation", "--check_diff"
             ]
             upload_command.extend(self._shed_args())
             self._check_exit_code(upload_command)
-            upload_command.append("--check_diff")
 
             # First time no difference.
             r = self._check_exit_code(upload_command)
@@ -55,15 +63,44 @@ def test_update_with_check_diff(self):
             with open(join(f, "related_file"), "w") as rf:
                 rf.write("new_contents")
 
+            self._assert_shed_diff(diff=1)
+
             # No assert there is no difference again.
             r = self._check_exit_code(upload_command)
             assert "not different, skipping upload." not in r.output
 
-    def test_update_metadata_only(self):
-        with self._isolate_repo("single_tool"):
+            self._assert_shed_diff(diff=0)
+
+    def test_update_with_check_diff_package(self):
+        with self._isolate_repo("package_1") as f:
+            self._shed_create()
+
+            self._assert_shed_diff(diff=0)
+            upload_command = [
+                "shed_update", "--force_repository_creation", "--check_diff"
+            ]
+            upload_command.extend(self._shed_args())
+            self._check_exit_code(upload_command)
+
+            # First time no difference.
+            r = self._check_exit_code(upload_command)
+            assert "not different, skipping upload." in r.output
+
+            update_package_1(f)
+            self._assert_shed_diff(diff=1)
+
+            # No assert there is no difference again.
+            r = self._check_exit_code(upload_command)
+            assert "not different, skipping upload." not in r.output
+
+            self._assert_shed_diff(diff=0)
+
+    def test_update_with_force_create_metadata_only(self):
+        with self._isolate_repo("single_tool") as f:
             upload_command = ["shed_update", "--force_repository_creation", "--skip_upload"]
             upload_command.extend(self._shed_args())
             self._check_exit_code(upload_command)
+            self._verify_empty_repository(f)
 
     def test_update_with_force_create(self):
         with self._isolate_repo("single_tool") as f:
@@ -290,6 +327,11 @@ def test_upload_with_double_dot(self):
                 ],
                 not_contains=[])
 
+    def _assert_shed_diff(self, diff=1):
+        shed_diff_command = ["shed_diff"]
+        shed_diff_command.extend(self._shed_args())
+        self._check_exit_code(shed_diff_command, exit_code=diff)
+
     def _verify_expansion(self, f, name=None):
         upload_command = ["shed_upload", "--tar_only"]
         upload_command.extend(self._shed_args())
@@ -320,6 +362,10 @@ def _verify_single_uploaded(self, f, download_args=[]):
             f, ["cat.xml", "related_file", "test-data/1.bed"], download_args
         )
 
+    def _verify_empty_repository(self, f, download_args=[]):
+        target = self._download_repo(f, download_args)
+        assert len(os.listdir(target)) == 0
+
     def _verify_upload(self, f, download_files=[], download_args=[]):
         target = self._download_repo(f, download_args)
         for download_file in download_files:
@@ -371,7 +417,21 @@ def _untar(self, f, path, tarbomb=True):
             return target
 
 
+def update_package_1(f):
+    """Update tool dependencies file for package_1."""
+    changed_xml = join(
+        TEST_REPOS_DIR,
+        "package_1_changed",
+        "tool_dependencies.xml"
+    )
+    shutil.copyfile(changed_xml, join(f, "tool_dependencies.xml"))
+
+
 def assert_exists(path):
+    """Assert supplied ``path`` exists.
+
+    Produces an informative AssertionError if it is does not.
+    """
     dir_path = os.path.dirname(path)
     msg = None
     if not exists(dir_path):

tests/test_utils.py

@@ -123,6 +123,13 @@ def tearDown(self):  # noqa
         super(CliShedTestCase, self).tearDown()
         self.mock_shed.shutdown()
 
+    def _shed_create(self, recursive=False):
+        create_command = ["shed_create"]
+        if recursive:
+            create_command.append("-r")
+        create_command.extend(self._shed_args())
+        self._check_exit_code(create_command)
+
     def _shed_args(self, read_only=False):
         args = [
             "--shed_target", self.mock_shed.url,