From 8fea7e90484917aa06e9be510a9590dfbfc1ae2d Mon Sep 17 00:00:00 2001
From: mvdbeek <m.vandenbeek@gmail.com>
Date: Tue, 21 May 2019 11:04:58 +0200
Subject: [PATCH 01/12] Use byte comparison for contains

I don't think there's any drawback open this
in byte mode.

Fixes https://github.com/galaxyproject/galaxy/issues/7957#issuecomment-493954901
---
 lib/galaxy/tools/verify/__init__.py | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/galaxy/tools/verify/__init__.py b/lib/galaxy/tools/verify/__init__.py
index f78d43b7f4f..2ed6e54fdb0 100644
--- a/lib/galaxy/tools/verify/__init__.py
+++ b/lib/galaxy/tools/verify/__init__.py
@@ -303,13 +303,13 @@ def files_re_match_multiline(file1, file2, attributes=None):
 
 def files_contains(file1, file2, attributes=None):
     """Check the contents of file2 for substrings found in file1, on a per-line basis."""
-    local_file = io.open(file1, encoding='utf-8').readlines()  # regex file
+    local_file = open(file1, 'rb').readlines()  # regex file
     # TODO: allow forcing ordering of contains
-    history_data = io.open(file2, encoding='utf-8').read()
+    history_data = open(file2, 'rb').read()
     lines_diff = int(attributes.get('lines_diff', 0))
     line_diff_count = 0
     while local_file:
-        contains = local_file.pop(0).rstrip('\n\r')
+        contains = local_file.pop(0).rstrip(b'\n\r')
         if contains not in history_data:
             line_diff_count += 1
         if line_diff_count > lines_diff:

From ab7a38631e15c0c326b210a63926c9642f3f6159 Mon Sep 17 00:00:00 2001
From: mvdbeek <m.vandenbeek@gmail.com>
Date: Wed, 22 May 2019 14:34:28 +0200
Subject: [PATCH 02/12] Unit test verify module

---
 test/unit/test_verify.py | 74 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 74 insertions(+)
 create mode 100644 test/unit/test_verify.py

diff --git a/test/unit/test_verify.py b/test/unit/test_verify.py
new file mode 100644
index 00000000000..e325471647b
--- /dev/null
+++ b/test/unit/test_verify.py
@@ -0,0 +1,74 @@
+import collections
+import os
+import tempfile
+
+import pytest
+
+from galaxy.tools.verify import (
+    files_contains,
+    files_diff,
+    files_re_match,
+    files_re_match_multiline,
+)
+
+
+F1 = b"A\nB\nC"
+F2 = b"A\nB\nD\nE"
+F3 = b"A\nB\nD\n\xfc"
+MULTINE_MATCH = b".*"
+TestFile = collections.namedtuple('TestFile', 'value path')
+
+
+def generate_tests(multiline=False):
+    files = []
+    for b in [F1, F2, F3, MULTINE_MATCH]:
+        fd, path = tempfile.mkstemp()
+        with os.fdopen(fd, 'wb') as out:
+            out.write(b)
+        files.append(TestFile(b, path))
+    f1, f2, f3, multiline_match = files
+    if multiline:
+        tests = [(multiline_match, f1, {'lines_diff': 0, 'sort': True}, None)]
+    else:
+        tests = [(f1, f1, {'lines_diff': 0}, None)]
+    tests.extend([
+        (f1, f2, {'lines_diff': 0}, AssertionError),
+        (f1, f3, {'lines_diff': 0}, AssertionError),
+    ])
+    return tests
+
+
+@pytest.mark.parametrize('file1,file2,attributes,expect', generate_tests())
+def test_files_contains(file1, file2, attributes, expect):
+    if expect is not None:
+        with pytest.raises(expect):
+            files_contains(file1.path, file2.path, attributes)
+    else:
+        files_contains(file2.path, file2.path, attributes)
+
+
+@pytest.mark.parametrize('file1,file2,attributes,expect', generate_tests())
+def test_files_diff(file1, file2, attributes, expect):
+    if expect is not None:
+        with pytest.raises(expect):
+            files_diff(file1.path, file2.path, attributes)
+    else:
+        files_diff(file1.path, file2.path, attributes)
+
+
+@pytest.mark.parametrize('file1,file2,attributes,expect', generate_tests())
+def test_files_re_match(file1, file2, attributes, expect):
+    if expect is not None:
+        with pytest.raises(expect):
+            files_re_match(file1.path, file2.path, attributes)
+    else:
+        files_re_match(file1.path, file2.path, attributes)
+
+
+@pytest.mark.parametrize('file1,file2,attributes,expect', generate_tests(multiline=True))
+def test_files_re_match_multiline(file1, file2, attributes, expect):
+    if expect is not None:
+        with pytest.raises(expect):
+            files_re_match_multiline(file1.path, file2.path, attributes)
+    else:
+        files_re_match_multiline(file1.path, file2.path, attributes)

From 30bec0028dc79e5e2e9d3a117f2908c47506be5a Mon Sep 17 00:00:00 2001
From: mvdbeek <m.vandenbeek@gmail.com>
Date: Wed, 22 May 2019 14:35:01 +0200
Subject: [PATCH 03/12] Use bytestring comparisons in verify module

---
 lib/galaxy/tools/verify/__init__.py | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/lib/galaxy/tools/verify/__init__.py b/lib/galaxy/tools/verify/__init__.py
index 2ed6e54fdb0..9f8a55fa4df 100644
--- a/lib/galaxy/tools/verify/__init__.py
+++ b/lib/galaxy/tools/verify/__init__.py
@@ -3,7 +3,6 @@
 import difflib
 import filecmp
 import hashlib
-import io
 import logging
 import os
 import os.path
@@ -268,9 +267,9 @@ def files_diff(file1, file2, attributes=None):
 
 def files_re_match(file1, file2, attributes=None):
     """Check the contents of 2 files for differences using re.match."""
-    local_file = io.open(file1, encoding='utf-8').readlines()  # regex file
-    history_data = io.open(file2, encoding='utf-8').readlines()
-    assert len(local_file) == len(history_data), 'Data File and Regular Expression File contain a different number of lines (%d != %d)\nHistory Data (first 40 lines):\n%s' % (len(local_file), len(history_data), ''.join(history_data[:40]))
+    local_file = open(file1, 'rb').readlines()  # regex file
+    history_data = open(file2, 'rb').readlines()
+    assert len(local_file) == len(history_data), b'Data File and Regular Expression File contain a different number of lines (%d != %d)\nHistory Data (first 40 lines):\n%s' % (len(local_file), len(history_data), b''.join(history_data[:40]))
     if attributes is None:
         attributes = {}
     if attributes.get('sort', False):
@@ -279,24 +278,24 @@ def files_re_match(file1, file2, attributes=None):
     line_diff_count = 0
     diffs = []
     for i in range(len(history_data)):
-        if not re.match(local_file[i].rstrip('\r\n'), history_data[i].rstrip('\r\n')):
+        if not re.match(local_file[i].rstrip(b'\r\n'), history_data[i].rstrip(b'\r\n')):
             line_diff_count += 1
-            diffs.append('Regular Expression: %s\nData file         : %s' % (local_file[i].rstrip('\r\n'), history_data[i].rstrip('\r\n')))
+            diffs.append(b'Regular Expression: %s\nData file         : %s' % (local_file[i].rstrip(b'\r\n'), history_data[i].rstrip(b'\r\n')))
         if line_diff_count > lines_diff:
-            raise AssertionError("Regular expression did not match data file (allowed variants=%i):\n%s" % (lines_diff, "".join(diffs)))
+            raise AssertionError(b"Regular expression did not match data file (allowed variants=%i):\n%s" % (lines_diff, b"".join(diffs)))
 
 
 def files_re_match_multiline(file1, file2, attributes=None):
     """Check the contents of 2 files for differences using re.match in multiline mode."""
-    local_file = io.open(file1, encoding='utf-8').read()  # regex file
+    local_file = open(file1, 'rb').read()  # regex file
     if attributes is None:
         attributes = {}
     if attributes.get('sort', False):
-        history_data = io.open(file2, encoding='utf-8').readlines()
+        history_data = open(file2, 'rb').readlines()
         history_data.sort()
-        history_data = ''.join(history_data)
+        history_data = b''.join(history_data)
     else:
-        history_data = io.open(file2, encoding='utf-8').read()
+        history_data = open(file2, 'rb').read()
     # lines_diff not applicable to multiline matching
     assert re.match(local_file, history_data, re.MULTILINE), "Multiline Regular expression did not match data file"
 

From 9f6bc89d69e0b4fe0d7fe86cffecad1c77ab736b Mon Sep 17 00:00:00 2001
From: mvdbeek <m.vandenbeek@gmail.com>
Date: Wed, 22 May 2019 17:59:49 +0200
Subject: [PATCH 04/12] Do non-shallow file comparison

---
 lib/galaxy/tools/verify/__init__.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/galaxy/tools/verify/__init__.py b/lib/galaxy/tools/verify/__init__.py
index 9f8a55fa4df..5cc4830866e 100644
--- a/lib/galaxy/tools/verify/__init__.py
+++ b/lib/galaxy/tools/verify/__init__.py
@@ -195,7 +195,7 @@ def files_diff(file1, file2, attributes=None):
                 count += 1
         return count
 
-    if not filecmp.cmp(file1, file2):
+    if not filecmp.cmp(file1, file2, shallow=False):
         if attributes is None:
             attributes = {}
         decompress = attributes.get("decompress", None)

From 2d71b0bdfe8a234d28ceb90dcd85ad41384ecdfe Mon Sep 17 00:00:00 2001
From: mvdbeek <m.vandenbeek@gmail.com>
Date: Thu, 23 May 2019 11:07:06 +0200
Subject: [PATCH 05/12] Use io.open with errors="repalce" instead of comparing
 byte strings

---
 lib/galaxy/tools/verify/__init__.py | 54 +++++++++++++----------------
 test/unit/test_verify.py            | 14 ++++----
 2 files changed, 32 insertions(+), 36 deletions(-)

diff --git a/lib/galaxy/tools/verify/__init__.py b/lib/galaxy/tools/verify/__init__.py
index 5cc4830866e..6e873818b28 100644
--- a/lib/galaxy/tools/verify/__init__.py
+++ b/lib/galaxy/tools/verify/__init__.py
@@ -3,6 +3,7 @@
 import difflib
 import filecmp
 import hashlib
+import io
 import logging
 import os
 import os.path
@@ -15,6 +16,7 @@ try:
 except ImportError:
     pysam = None
 
+from galaxy.util import unicodify
 from galaxy.util.compression_utils import get_fileobj
 from .asserts import verify_assertions
 from .test_data import TestDataResolver
@@ -109,9 +111,9 @@ def verify(
                 log.error(error_log_msg, exc_info=True)
             else:
                 log.debug('## GALAXY_TEST_SAVE=%s. saved %s' % (keep_outputs_dir, ofn))
+        compare = attributes.get('compare', 'diff')
         try:
-            compare = attributes.get('compare', 'diff')
-            if attributes.get('ftype', None) in ['bam', 'qname_sorted.bam', 'qname_input_sorted.bam', 'unsorted.bam']:
+            if attributes.get('ftype', None) in ['bam', 'qname_sorted.bam', 'qname_input_sorted.bam', 'unsorted.bam', 'cram']:
                 try:
                     local_fh, temp_name = _bam_to_sam(local_name, temp_name)
                     local_name = local_fh.name
@@ -204,17 +206,9 @@ def files_diff(file1, file2, attributes=None):
             compressed_formats = None
         else:
             compressed_formats = []
-        is_pdf = False
-        try:
-            local_file = get_fileobj(file1, compressed_formats=compressed_formats).readlines()
-            history_data = get_fileobj(file2, compressed_formats=compressed_formats).readlines()
-        except UnicodeDecodeError:
-            if file1.endswith('.pdf') or file2.endswith('.pdf'):
-                is_pdf = True
-                local_file = open(file1, 'rb').readlines()
-                history_data = open(file2, 'rb').readlines()
-            else:
-                raise AssertionError("Binary data detected, not displaying diff")
+        local_file = [unicodify(l) for l in get_fileobj(file1, mode='rb', compressed_formats=compressed_formats)]
+        history_data = [unicodify(l) for l in get_fileobj(file2, mode='rb', compressed_formats=compressed_formats)]
+        is_pdf = file1.endswith('.pdf') or file2.endswith('.pdf')
         if attributes.get('sort', False):
             local_file.sort()
             history_data.sort()
@@ -267,9 +261,9 @@ def files_diff(file1, file2, attributes=None):
 
 def files_re_match(file1, file2, attributes=None):
     """Check the contents of 2 files for differences using re.match."""
-    local_file = open(file1, 'rb').readlines()  # regex file
-    history_data = open(file2, 'rb').readlines()
-    assert len(local_file) == len(history_data), b'Data File and Regular Expression File contain a different number of lines (%d != %d)\nHistory Data (first 40 lines):\n%s' % (len(local_file), len(history_data), b''.join(history_data[:40]))
+    local_file = io.open(file1, 'r', encoding='utf-8', errors='replace').readlines()  # regex file
+    history_data = io.open(file2, 'r', encoding='utf-8', errors='replace').readlines()
+    assert len(local_file) == len(history_data), 'Data File and Regular Expression File contain a different number of lines (%d != %d)\nHistory Data (first 40 lines):\n%s' % (len(local_file), len(history_data), ''.join(history_data[:40]))
     if attributes is None:
         attributes = {}
     if attributes.get('sort', False):
@@ -277,38 +271,40 @@ def files_re_match(file1, file2, attributes=None):
     lines_diff = int(attributes.get('lines_diff', 0))
     line_diff_count = 0
     diffs = []
-    for i in range(len(history_data)):
-        if not re.match(local_file[i].rstrip(b'\r\n'), history_data[i].rstrip(b'\r\n')):
+    for regex_line, data_line in zip(local_file, history_data):
+        if not re.match(regex_line.rstrip('\r\n'), data_line.rstrip('\r\n')):
             line_diff_count += 1
-            diffs.append(b'Regular Expression: %s\nData file         : %s' % (local_file[i].rstrip(b'\r\n'), history_data[i].rstrip(b'\r\n')))
-        if line_diff_count > lines_diff:
-            raise AssertionError(b"Regular expression did not match data file (allowed variants=%i):\n%s" % (lines_diff, b"".join(diffs)))
+            diffs.append('Regular Expression: %s, Data file: %s\n' % (regex_line.rstrip('\r\n'), data_line.rstrip('\r\n')))
+    if line_diff_count > lines_diff:
+        raise AssertionError("Regular expression did not match data file (allowed variants=%i):\n%s" % (lines_diff, "".join(diffs)))
 
 
 def files_re_match_multiline(file1, file2, attributes=None):
     """Check the contents of 2 files for differences using re.match in multiline mode."""
-    local_file = open(file1, 'rb').read()  # regex file
+    local_file = io.open(file1, 'r', encoding='utf-8', errors='replace').read()  # regex file
     if attributes is None:
         attributes = {}
     if attributes.get('sort', False):
-        history_data = open(file2, 'rb').readlines()
+        history_data = io.open(file2, 'r', encoding='utf-8', errors='replace').readlines()
         history_data.sort()
-        history_data = b''.join(history_data)
+        history_data = ''.join(history_data)
     else:
-        history_data = open(file2, 'rb').read()
+        history_data = io.open(file2, 'r', encoding='utf-8', errors='replace').read()
     # lines_diff not applicable to multiline matching
     assert re.match(local_file, history_data, re.MULTILINE), "Multiline Regular expression did not match data file"
 
 
 def files_contains(file1, file2, attributes=None):
     """Check the contents of file2 for substrings found in file1, on a per-line basis."""
-    local_file = open(file1, 'rb').readlines()  # regex file
+    if attributes is None:
+        attributes = {}
+    local_file = io.open(file1, 'r', encoding='utf-8', errors='replace').readlines()  # regex file
     # TODO: allow forcing ordering of contains
-    history_data = open(file2, 'rb').read()
+    history_data = io.open(file2, 'r', encoding='utf-8', errors='replace').read()
     lines_diff = int(attributes.get('lines_diff', 0))
     line_diff_count = 0
-    while local_file:
-        contains = local_file.pop(0).rstrip(b'\n\r')
+    for contains in local_file:
+        contains = contains.rstrip('\r\n')
         if contains not in history_data:
             line_diff_count += 1
         if line_diff_count > lines_diff:
diff --git a/test/unit/test_verify.py b/test/unit/test_verify.py
index e325471647b..2dddb543135 100644
--- a/test/unit/test_verify.py
+++ b/test/unit/test_verify.py
@@ -13,16 +13,16 @@ from galaxy.tools.verify import (
 
 
 F1 = b"A\nB\nC"
-F2 = b"A\nB\nD\nE"
-F3 = b"A\nB\nD\n\xfc"
+F2 = b"A\nB\nD\nE" * 61
+F3 = b"A\nB\n\xfc"
 MULTINE_MATCH = b".*"
 TestFile = collections.namedtuple('TestFile', 'value path')
 
 
 def generate_tests(multiline=False):
     files = []
-    for b in [F1, F2, F3, MULTINE_MATCH]:
-        fd, path = tempfile.mkstemp()
+    for b, ext in [(F1, '.txt'), (F2, '.txt'), (F3, '.pdf'), (MULTINE_MATCH, '.txt')]:
+        fd, path = tempfile.mkstemp(suffix=ext)
         with os.fdopen(fd, 'wb') as out:
             out.write(b)
         files.append(TestFile(b, path))
@@ -30,10 +30,10 @@ def generate_tests(multiline=False):
     if multiline:
         tests = [(multiline_match, f1, {'lines_diff': 0, 'sort': True}, None)]
     else:
-        tests = [(f1, f1, {'lines_diff': 0}, None)]
+        tests = [(f1, f1, {'lines_diff': 0, 'sort': True}, None)]
     tests.extend([
-        (f1, f2, {'lines_diff': 0}, AssertionError),
-        (f1, f3, {'lines_diff': 0}, AssertionError),
+        (f1, f2, {'lines_diff': 0, 'sort': True}, AssertionError),
+        (f1, f3, None, AssertionError),
     ])
     return tests
 

From a398127ede922d504ec7f1c39e66df22a9f0a16d Mon Sep 17 00:00:00 2001
From: mvdbeek <m.vandenbeek@gmail.com>
Date: Thu, 23 May 2019 13:43:57 +0200
Subject: [PATCH 06/12] Work with bytestrings in `files_contains`,
 `files_re_match_*`

But read utf-8 unicode in `lines_diff`. This will replace non-utf8
characters and seems like a reasonable tradeoff for text files
mixed with byte contents, like pdfs.
---
 lib/galaxy/tools/verify/__init__.py | 31 ++++++++++++++++-------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/lib/galaxy/tools/verify/__init__.py b/lib/galaxy/tools/verify/__init__.py
index 6e873818b28..a58cb39bcf2 100644
--- a/lib/galaxy/tools/verify/__init__.py
+++ b/lib/galaxy/tools/verify/__init__.py
@@ -3,7 +3,6 @@
 import difflib
 import filecmp
 import hashlib
-import io
 import logging
 import os
 import os.path
@@ -206,6 +205,9 @@ def files_diff(file1, file2, attributes=None):
             compressed_formats = None
         else:
             compressed_formats = []
+
+        # Open expected contents and history data in binary mode and run lines through unicodify,
+        # which will replace non utf-8 characters.
         local_file = [unicodify(l) for l in get_fileobj(file1, mode='rb', compressed_formats=compressed_formats)]
         history_data = [unicodify(l) for l in get_fileobj(file2, mode='rb', compressed_formats=compressed_formats)]
         is_pdf = file1.endswith('.pdf') or file2.endswith('.pdf')
@@ -261,9 +263,9 @@ def files_diff(file1, file2, attributes=None):
 
 def files_re_match(file1, file2, attributes=None):
     """Check the contents of 2 files for differences using re.match."""
-    local_file = io.open(file1, 'r', encoding='utf-8', errors='replace').readlines()  # regex file
-    history_data = io.open(file2, 'r', encoding='utf-8', errors='replace').readlines()
-    assert len(local_file) == len(history_data), 'Data File and Regular Expression File contain a different number of lines (%d != %d)\nHistory Data (first 40 lines):\n%s' % (len(local_file), len(history_data), ''.join(history_data[:40]))
+    local_file = open(file1, 'rb').readlines()  # regex file
+    history_data = open(file2, 'rb').readlines()
+    assert len(local_file) == len(history_data), 'Data File and Regular Expression File contain a different number of lines (%d != %d)\nHistory Data (first 40 lines):\n%s' % (len(local_file), len(history_data), ''.join(unicodify(history_data[:40])))
     if attributes is None:
         attributes = {}
     if attributes.get('sort', False):
@@ -272,24 +274,25 @@ def files_re_match(file1, file2, attributes=None):
     line_diff_count = 0
     diffs = []
     for regex_line, data_line in zip(local_file, history_data):
-        if not re.match(regex_line.rstrip('\r\n'), data_line.rstrip('\r\n')):
+        if not re.match(regex_line.rstrip(b'\r\n'), data_line.rstrip(b'\r\n')):
             line_diff_count += 1
-            diffs.append('Regular Expression: %s, Data file: %s\n' % (regex_line.rstrip('\r\n'), data_line.rstrip('\r\n')))
+            diffs.append('Regular Expression: %s, Data file: %s\n' % (unicodify(regex_line).rstrip('\r\n'),
+                                                                      unicodify(data_line).rstrip('\r\n')))
     if line_diff_count > lines_diff:
         raise AssertionError("Regular expression did not match data file (allowed variants=%i):\n%s" % (lines_diff, "".join(diffs)))
 
 
 def files_re_match_multiline(file1, file2, attributes=None):
     """Check the contents of 2 files for differences using re.match in multiline mode."""
-    local_file = io.open(file1, 'r', encoding='utf-8', errors='replace').read()  # regex file
+    local_file = open(file1, 'rb').read()  # regex file
     if attributes is None:
         attributes = {}
     if attributes.get('sort', False):
-        history_data = io.open(file2, 'r', encoding='utf-8', errors='replace').readlines()
+        history_data = open(file2, 'rb').readlines()
         history_data.sort()
-        history_data = ''.join(history_data)
+        history_data = b''.join(history_data)
     else:
-        history_data = io.open(file2, 'r', encoding='utf-8', errors='replace').read()
+        history_data = open(file2, 'rb').read()
     # lines_diff not applicable to multiline matching
     assert re.match(local_file, history_data, re.MULTILINE), "Multiline Regular expression did not match data file"
 
@@ -298,14 +301,14 @@ def files_contains(file1, file2, attributes=None):
     """Check the contents of file2 for substrings found in file1, on a per-line basis."""
     if attributes is None:
         attributes = {}
-    local_file = io.open(file1, 'r', encoding='utf-8', errors='replace').readlines()  # regex file
+    local_file = open(file1, 'rb').readlines()  # regex file
     # TODO: allow forcing ordering of contains
-    history_data = io.open(file2, 'r', encoding='utf-8', errors='replace').read()
+    history_data = open(file2, 'rb').read()
     lines_diff = int(attributes.get('lines_diff', 0))
     line_diff_count = 0
     for contains in local_file:
-        contains = contains.rstrip('\r\n')
+        contains = contains.rstrip(b'\r\n')
         if contains not in history_data:
             line_diff_count += 1
         if line_diff_count > lines_diff:
-            raise AssertionError("Failed to find '%s' in history data. (lines_diff=%i):\n" % (contains, lines_diff))
+            raise AssertionError("Failed to find '%s' in history data. (lines_diff=%i):\n" % (unicodify(contains), lines_diff))

From d9b0f5490668bec16fa7b3f1e6f875fa2f09430b Mon Sep 17 00:00:00 2001
From: Nicola Soranzo <nicola.soranzo@earlham.ac.uk>
Date: Sun, 26 May 2019 14:55:14 +0100
Subject: [PATCH 07/12] Try to verify tool test output first as Unicode

---
 lib/galaxy/tools/verify/__init__.py | 85 +++++++++++++++++++++--------
 test/unit/test_verify.py            |  4 +-
 2 files changed, 65 insertions(+), 24 deletions(-)

diff --git a/lib/galaxy/tools/verify/__init__.py b/lib/galaxy/tools/verify/__init__.py
index a58cb39bcf2..b95fed8c46b 100644
--- a/lib/galaxy/tools/verify/__init__.py
+++ b/lib/galaxy/tools/verify/__init__.py
@@ -3,6 +3,7 @@
 import difflib
 import filecmp
 import hashlib
+import io
 import logging
 import os
 import os.path
@@ -205,12 +206,21 @@ def files_diff(file1, file2, attributes=None):
             compressed_formats = None
         else:
             compressed_formats = []
-
-        # Open expected contents and history data in binary mode and run lines through unicodify,
-        # which will replace non utf-8 characters.
-        local_file = [unicodify(l) for l in get_fileobj(file1, mode='rb', compressed_formats=compressed_formats)]
-        history_data = [unicodify(l) for l in get_fileobj(file2, mode='rb', compressed_formats=compressed_formats)]
-        is_pdf = file1.endswith('.pdf') or file2.endswith('.pdf')
+        is_pdf = False
+        try:
+            with get_fileobj(file2, compressed_formats=compressed_formats) as fh:
+                history_data = fh.readlines()
+            with get_fileobj(file1, compressed_formats=compressed_formats) as fh:
+                local_file = fh.readlines()
+        except UnicodeDecodeError:
+            if file1.endswith('.pdf') or file2.endswith('.pdf'):
+                is_pdf = True
+                # Replace non-Unicode characters using unicodify(),
+                # difflib.unified_diff doesn't work on list of bytes
+                history_data = [unicodify(l) for l in get_fileobj(file2, mode='rb', compressed_formats=compressed_formats)]
+                local_file = [unicodify(l) for l in get_fileobj(file1, mode='rb', compressed_formats=compressed_formats)]
+            else:
+                raise AssertionError("Binary data detected, not displaying diff")
         if attributes.get('sort', False):
             local_file.sort()
             history_data.sort()
@@ -263,9 +273,21 @@ def files_diff(file1, file2, attributes=None):
 
 def files_re_match(file1, file2, attributes=None):
     """Check the contents of 2 files for differences using re.match."""
-    local_file = open(file1, 'rb').readlines()  # regex file
-    history_data = open(file2, 'rb').readlines()
-    assert len(local_file) == len(history_data), 'Data File and Regular Expression File contain a different number of lines (%d != %d)\nHistory Data (first 40 lines):\n%s' % (len(local_file), len(history_data), ''.join(unicodify(history_data[:40])))
+    join_char = ''
+    to_strip = os.linesep
+    try:
+        with io.open(file2, encoding='utf-8') as fh:
+            history_data = fh.readlines()
+        with io.open(file1, encoding='utf-8') as fh:
+            local_file = fh.readlines()
+    except UnicodeDecodeError:
+        join_char = b''
+        to_strip = os.linesep.encode('utf-8')
+        with open(file2, 'rb') as fh:
+            history_data = fh.readlines()
+        with open(file1, 'rb') as fh:
+            local_file = fh.readlines()
+    assert len(local_file) == len(history_data), 'Data File and Regular Expression File contain a different number of lines (%d != %d)\nHistory Data (first 40 lines):\n%s' % (len(local_file), len(history_data), join_char.join(history_data[:40]))
     if attributes is None:
         attributes = {}
     if attributes.get('sort', False):
@@ -274,41 +296,60 @@ def files_re_match(file1, file2, attributes=None):
     line_diff_count = 0
     diffs = []
     for regex_line, data_line in zip(local_file, history_data):
-        if not re.match(regex_line.rstrip(b'\r\n'), data_line.rstrip(b'\r\n')):
+        regex_line = regex_line.rstrip(to_strip)
+        data_line = data_line.rstrip(to_strip)
+        if not re.match(regex_line, data_line):
             line_diff_count += 1
-            diffs.append('Regular Expression: %s, Data file: %s\n' % (unicodify(regex_line).rstrip('\r\n'),
-                                                                      unicodify(data_line).rstrip('\r\n')))
+            diffs.append('Regular Expression: %s, Data file: %s\n' % (regex_line, data_line))
     if line_diff_count > lines_diff:
         raise AssertionError("Regular expression did not match data file (allowed variants=%i):\n%s" % (lines_diff, "".join(diffs)))
 
 
 def files_re_match_multiline(file1, file2, attributes=None):
     """Check the contents of 2 files for differences using re.match in multiline mode."""
-    local_file = open(file1, 'rb').read()  # regex file
+    join_char = ''
+    try:
+        with io.open(file2, encoding='utf-8') as fh:
+            history_data = fh.readlines()
+        with io.open(file1, encoding='utf-8') as fh:
+            local_file = fh.read()
+    except UnicodeDecodeError:
+        join_char = b''
+        with open(file2, 'rb') as fh:
+            history_data = fh.readlines()
+        with open(file1, 'rb') as fh:
+            local_file = fh.read()
     if attributes is None:
         attributes = {}
     if attributes.get('sort', False):
-        history_data = open(file2, 'rb').readlines()
         history_data.sort()
-        history_data = b''.join(history_data)
-    else:
-        history_data = open(file2, 'rb').read()
+    history_data = join_char.join(history_data)
     # lines_diff not applicable to multiline matching
     assert re.match(local_file, history_data, re.MULTILINE), "Multiline Regular expression did not match data file"
 
 
 def files_contains(file1, file2, attributes=None):
     """Check the contents of file2 for substrings found in file1, on a per-line basis."""
+    # TODO: allow forcing ordering of contains
+    to_strip = os.linesep
+    try:
+        with io.open(file2, encoding='utf-8') as fh:
+            history_data = fh.read()
+        with io.open(file1, encoding='utf-8') as fh:
+            local_file = fh.readlines()
+    except UnicodeDecodeError:
+        to_strip = os.linesep.encode('utf-8')
+        with open(file2, 'rb') as fh:
+            history_data = fh.read()
+        with open(file1, 'rb') as fh:
+            local_file = fh.readlines()
     if attributes is None:
         attributes = {}
-    local_file = open(file1, 'rb').readlines()  # regex file
-    # TODO: allow forcing ordering of contains
-    history_data = open(file2, 'rb').read()
     lines_diff = int(attributes.get('lines_diff', 0))
     line_diff_count = 0
     for contains in local_file:
-        contains = contains.rstrip(b'\r\n')
+        contains = contains.rstrip(to_strip)
         if contains not in history_data:
             line_diff_count += 1
         if line_diff_count > lines_diff:
-            raise AssertionError("Failed to find '%s' in history data. (lines_diff=%i):\n" % (unicodify(contains), lines_diff))
+            raise AssertionError("Failed to find '%s' in history data. (lines_diff=%i):\n" % (contains, lines_diff))
diff --git a/test/unit/test_verify.py b/test/unit/test_verify.py
index 2dddb543135..ac78103aa73 100644
--- a/test/unit/test_verify.py
+++ b/test/unit/test_verify.py
@@ -15,13 +15,13 @@ from galaxy.tools.verify import (
 F1 = b"A\nB\nC"
 F2 = b"A\nB\nD\nE" * 61
 F3 = b"A\nB\n\xfc"
-MULTINE_MATCH = b".*"
+MULTILINE_MATCH = b".*"
 TestFile = collections.namedtuple('TestFile', 'value path')
 
 
 def generate_tests(multiline=False):
     files = []
-    for b, ext in [(F1, '.txt'), (F2, '.txt'), (F3, '.pdf'), (MULTINE_MATCH, '.txt')]:
+    for b, ext in [(F1, '.txt'), (F2, '.txt'), (F3, '.pdf'), (MULTILINE_MATCH, '.txt')]:
         fd, path = tempfile.mkstemp(suffix=ext)
         with os.fdopen(fd, 'wb') as out:
             out.write(b)

From 62f5572a430df6457038ae2b50e2e0278d055262 Mon Sep 17 00:00:00 2001
From: mvdbeek <m.vandenbeek@gmail.com>
Date: Mon, 27 May 2019 11:35:20 +0200
Subject: [PATCH 08/12] Add carriage return unit test

---
 test/unit/test_verify.py | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/test/unit/test_verify.py b/test/unit/test_verify.py
index ac78103aa73..8de580cec5b 100644
--- a/test/unit/test_verify.py
+++ b/test/unit/test_verify.py
@@ -15,18 +15,19 @@ from galaxy.tools.verify import (
 F1 = b"A\nB\nC"
 F2 = b"A\nB\nD\nE" * 61
 F3 = b"A\nB\n\xfc"
+F4 = b"A\r\nB\nC"
 MULTILINE_MATCH = b".*"
 TestFile = collections.namedtuple('TestFile', 'value path')
 
 
 def generate_tests(multiline=False):
     files = []
-    for b, ext in [(F1, '.txt'), (F2, '.txt'), (F3, '.pdf'), (MULTILINE_MATCH, '.txt')]:
+    for b, ext in [(F1, '.txt'), (F2, '.txt'), (F3, '.pdf'), (F4, '.txt'), (MULTILINE_MATCH, '.txt')]:
         fd, path = tempfile.mkstemp(suffix=ext)
         with os.fdopen(fd, 'wb') as out:
             out.write(b)
         files.append(TestFile(b, path))
-    f1, f2, f3, multiline_match = files
+    f1, f2, f3, f4, multiline_match = files
     if multiline:
         tests = [(multiline_match, f1, {'lines_diff': 0, 'sort': True}, None)]
     else:
@@ -34,6 +35,7 @@ def generate_tests(multiline=False):
     tests.extend([
         (f1, f2, {'lines_diff': 0, 'sort': True}, AssertionError),
         (f1, f3, None, AssertionError),
+        (f1, f4, None, None),
     ])
     return tests
 

From 7ea502be77c56b77b8e03b9d78f4d0836b3b93f9 Mon Sep 17 00:00:00 2001
From: John Chilton <jmchilton@gmail.com>
Date: Thu, 18 Apr 2019 13:24:18 -0400
Subject: [PATCH 09/12] Run API tests with a nested object store.

---
 run_tests.sh             |  2 ++
 test/base/driver_util.py | 30 ++++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/run_tests.sh b/run_tests.sh
index 5634c1d7793..0efac9e7111 100755
--- a/run_tests.sh
+++ b/run_tests.sh
@@ -362,6 +362,8 @@ do
           fi
           ;;
       -a|-api|--api)
+          GALAXY_TEST_USE_HIERARCHICAL_OBJECT_STORE="True"  # Run these tests with a non-trivial object store.
+          export GALAXY_TEST_USE_HIERARCHICAL_OBJECT_STORE
           GALAXY_TEST_TOOL_CONF="config/tool_conf.xml.sample,test/functional/tools/samples_tool_conf.xml"
           test_script="pytest"
           report_file="./run_api_tests.html"
diff --git a/test/base/driver_util.py b/test/base/driver_util.py
index 7b09c9b91eb..c828c848389 100644
--- a/test/base/driver_util.py
+++ b/test/base/driver_util.py
@@ -240,6 +240,36 @@ def setup_galaxy_config(
     )
     config.update(database_conf(tmpdir, prefer_template_database=prefer_template_database))
     config.update(install_database_conf(tmpdir, default_merged=default_install_db_merged))
+    if asbool(os.environ.get("GALAXY_TEST_USE_HIERARCHICAL_OBJECT_STORE")):
+        object_store_config = os.path.join(tmpdir, "object_store_conf.yml")
+        with open(object_store_config, "w") as f:
+            contents = """
+type: hierarchical
+backends:
+   - id: files1
+     type: disk
+     weight: 1
+     files_dir: "${temp_directory}/files1"
+     extra_dirs:
+     - type: temp
+       path: "${temp_directory}/tmp1"
+     - type: job_work
+       path: "${temp_directory}/job_working_directory1"
+   - id: files2
+     type: disk
+     weight: 1
+     files_dir: "${temp_directory}/files2"
+     extra_dirs:
+     - type: temp
+       path: "${temp_directory}/tmp2"
+     - type: job_work
+       path: "${temp_directory}/job_working_directory2"
+"""
+            contents_template = string.Template(contents)
+            expanded_contents = contents_template.safe_substitute(temp_directory=tmpdir)
+            f.write(expanded_contents)
+        config["object_store_config_file"] = object_store_config
+
     if datatypes_conf is not None:
         config['datatypes_config_file'] = datatypes_conf
     if enable_tool_shed_check:

From 419bae1f0917875705634d5eaf89c0e0aa31ccbd Mon Sep 17 00:00:00 2001
From: John Chilton <jmchilton@gmail.com>
Date: Thu, 18 Apr 2019 14:34:25 -0400
Subject: [PATCH 10/12] Don't calculate real path in tool action code if
 unused.

---
 lib/galaxy/tools/wrappers.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/galaxy/tools/wrappers.py b/lib/galaxy/tools/wrappers.py
index 474a54b60f5..8e23cb72585 100644
--- a/lib/galaxy/tools/wrappers.py
+++ b/lib/galaxy/tools/wrappers.py
@@ -322,7 +322,7 @@ class HasDatasets(object):
 
     def _dataset_wrapper(self, dataset, dataset_paths, **kwargs):
         wrapper_kwds = kwargs.copy()
-        if dataset:
+        if dataset and dataset_paths:
             real_path = dataset.file_name
             if real_path in dataset_paths:
                 wrapper_kwds["dataset_path"] = dataset_paths[real_path]

From 563ae4d7d3ffd24c811046421000645a7b011f52 Mon Sep 17 00:00:00 2001
From: mvdbeek <m.vandenbeek@gmail.com>
Date: Sun, 12 May 2019 20:13:57 +0200
Subject: [PATCH 11/12] Raise ObjectNotFound if file does not exist in object
 store

And return an empty string if a path does not exist when calling
`get_filename` or` get_extra_files_path` on a Dataset.
---
 lib/galaxy/model/__init__.py                   | 18 ++++++++++++------
 lib/galaxy/objectstore/__init__.py             |  5 ++++-
 test/unit/jobs/test_job_wrapper.py             |  3 +++
 test/unit/test_model_store.py                  |  1 +
 test/unit/tools/test_actions.py                |  3 +++
 .../tools/test_collect_primary_datasets.py     |  3 +++
 6 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/lib/galaxy/model/__init__.py b/lib/galaxy/model/__init__.py
index faebe5498d2..f0a3b589ee3 100644
--- a/lib/galaxy/model/__init__.py
+++ b/lib/galaxy/model/__init__.py
@@ -42,6 +42,7 @@ from sqlalchemy.orm import (
 )
 from sqlalchemy.schema import UniqueConstraint
 
+import galaxy.exceptions
 import galaxy.model.metadata
 import galaxy.model.orm.now
 import galaxy.model.tags
@@ -2156,8 +2157,10 @@ class Dataset(StorableObject, RepresentById):
     def get_file_name(self):
         if not self.external_filename:
             assert self.object_store is not None, "Object Store has not been initialized for dataset %s" % self.id
-            filename = self.object_store.get_filename(self)
-            return filename
+            if self.object_store.exists(self):
+                return self.object_store.get_filename(self)
+            else:
+                return ''
         else:
             filename = self.external_filename
         # Make filename absolute
@@ -2175,7 +2178,9 @@ class Dataset(StorableObject, RepresentById):
         # actual database column so if SA instantiates this object - the
         # attribute won't exist yet.
         if not getattr(self, "external_extra_files_path", None):
-            return self.object_store.get_filename(self, dir_only=True, extra_dir=self._extra_files_rel_path)
+            if self.object_store.exists(self, dir_only=True, extra_dir=self._extra_files_rel_path):
+                return self.object_store.get_filename(self, dir_only=True, extra_dir=self._extra_files_rel_path)
+            return ''
         else:
             return os.path.abspath(self.external_extra_files_path)
 
@@ -2266,11 +2271,12 @@ class Dataset(StorableObject, RepresentById):
     def full_delete(self):
         """Remove the file and extra files, marks deleted and purged"""
         # os.unlink( self.file_name )
-        self.object_store.delete(self)
+        try:
+            self.object_store.delete(self)
+        except galaxy.exceptions.ObjectNotFound:
+            pass
         if self.object_store.exists(self, extra_dir=self._extra_files_rel_path, dir_only=True):
             self.object_store.delete(self, entire_dir=True, extra_dir=self._extra_files_rel_path, dir_only=True)
-        # if os.path.exists( self.extra_files_path ):
-        #     shutil.rmtree( self.extra_files_path )
         # TODO: purge metadata files
         self.deleted = True
         self.purged = True
diff --git a/lib/galaxy/objectstore/__init__.py b/lib/galaxy/objectstore/__init__.py
index b73a4a8ad56..6294ff77832 100644
--- a/lib/galaxy/objectstore/__init__.py
+++ b/lib/galaxy/objectstore/__init__.py
@@ -467,7 +467,10 @@ class DiskObjectStore(ObjectStore):
             # construct and return hashed path
             if os.path.exists(path):
                 return path
-        return self._construct_path(obj, **kwargs)
+        path = self._construct_path(obj, **kwargs)
+        if not os.path.exists(path):
+            raise ObjectNotFound
+        return path
 
     def update_from_file(self, obj, file_name=None, create=False, **kwargs):
         """`create` parameter is not used in this implementation."""
diff --git a/test/unit/jobs/test_job_wrapper.py b/test/unit/jobs/test_job_wrapper.py
index 5e370587116..50389f204ef 100644
--- a/test/unit/jobs/test_job_wrapper.py
+++ b/test/unit/jobs/test_job_wrapper.py
@@ -191,6 +191,9 @@ class MockObjectStore(object):
     def create(self, *args, **kwds):
         pass
 
+    def exists(self, *args, **kwargs):
+        return True
+
     def get_filename(self, *args, **kwds):
         if kwds.get("base_dir", "") == "job_work":
             return self.working_directory
diff --git a/test/unit/test_model_store.py b/test/unit/test_model_store.py
index 8614658bfe7..ec2cb414c99 100644
--- a/test/unit/test_model_store.py
+++ b/test/unit/test_model_store.py
@@ -283,6 +283,7 @@ def test_import_export_composite_datasets():
     h = model.History(name="Test History", user=u)
 
     d1 = _create_datasets(sa_session, h, 1, extension="html")[0]
+    app.object_store.create(d1.dataset, dir_only=True, extra_dir=d1.dataset._extra_files_rel_path)
     sa_session.add_all((h, d1))
     sa_session.flush()
 
diff --git a/test/unit/tools/test_actions.py b/test/unit/tools/test_actions.py
index 3c23507ab6b..8628cd21205 100644
--- a/test/unit/tools/test_actions.py
+++ b/test/unit/tools/test_actions.py
@@ -274,6 +274,9 @@ class MockObjectStore(object):
         self.first_create = True
         self.object_store_id = "mycoolid"
 
+    def exists(self, *args, **kwargs):
+        return True
+
     def create(self, dataset):
         self.created_datasets.append(dataset)
         if self.first_create:
diff --git a/test/unit/tools/test_collect_primary_datasets.py b/test/unit/tools/test_collect_primary_datasets.py
index 0f1814680b3..b926502e03f 100644
--- a/test/unit/tools/test_collect_primary_datasets.py
+++ b/test/unit/tools/test_collect_primary_datasets.py
@@ -408,6 +408,9 @@ class MockObjectStore(object):
         path = self.created_datasets[dataset]
         return os.stat(path).st_size
 
+    def exists(self, *args, **kwargs):
+        return True
+
     def get_filename(self, dataset):
         return self.created_datasets[dataset]
 

From 5b05866545c2edfe05eaca400b0a661a9a81eeda Mon Sep 17 00:00:00 2001
From: mvdbeek <m.vandenbeek@gmail.com>
Date: Fri, 17 May 2019 14:04:23 +0200
Subject: [PATCH 12/12] Explicitly create extra_files_path when discovering
 outputs with extra files

---
 lib/galaxy/model/__init__.py                  | 4 ++++
 lib/galaxy/tools/parameters/output_collect.py | 1 +
 test/unit/test_model_store.py                 | 2 +-
 3 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/lib/galaxy/model/__init__.py b/lib/galaxy/model/__init__.py
index f0a3b589ee3..1448ce6f75e 100644
--- a/lib/galaxy/model/__init__.py
+++ b/lib/galaxy/model/__init__.py
@@ -2184,6 +2184,10 @@ class Dataset(StorableObject, RepresentById):
         else:
             return os.path.abspath(self.external_extra_files_path)
 
+    def create_extra_files_path(self):
+        if not self.extra_files_path_exists():
+            self.object_store.create(self, dir_only=True, extra_dir=self._extra_files_rel_path)
+
     def set_extra_files_path(self, extra_files_path):
         if not extra_files_path:
             self.external_extra_files_path = None
diff --git a/lib/galaxy/tools/parameters/output_collect.py b/lib/galaxy/tools/parameters/output_collect.py
index caca15031cd..81c10a8d22d 100644
--- a/lib/galaxy/tools/parameters/output_collect.py
+++ b/lib/galaxy/tools/parameters/output_collect.py
@@ -316,6 +316,7 @@ def collect_primary_datasets(job_context, output, input_ext):
                 extra_files_path = new_primary_datasets_attributes.get('extra_files', None)
                 if extra_files_path:
                     extra_files_path_joined = os.path.join(job_working_directory, extra_files_path)
+                    primary_data.dataset.create_extra_files_path()
                     for root, dirs, files in os.walk(extra_files_path_joined):
                         extra_dir = os.path.join(primary_data.extra_files_path, root.replace(extra_files_path_joined, '', 1).lstrip(os.path.sep))
                         extra_dir = os.path.normpath(extra_dir)
diff --git a/test/unit/test_model_store.py b/test/unit/test_model_store.py
index ec2cb414c99..4f26f5b2574 100644
--- a/test/unit/test_model_store.py
+++ b/test/unit/test_model_store.py
@@ -283,7 +283,7 @@ def test_import_export_composite_datasets():
     h = model.History(name="Test History", user=u)
 
     d1 = _create_datasets(sa_session, h, 1, extension="html")[0]
-    app.object_store.create(d1.dataset, dir_only=True, extra_dir=d1.dataset._extra_files_rel_path)
+    d1.dataset.create_extra_files_path()
     sa_session.add_all((h, d1))
     sa_session.flush()