From 062e1ab7f2bd894de0922b74f75ed94580e5aa72 Mon Sep 17 00:00:00 2001 From: David Murray Date: Wed, 11 May 2016 15:43:18 +0100 Subject: [PATCH 1/3] Added a unit test for expand filespecs. --- lib/iris/io/__init__.py | 13 ++-- .../tests/unit/io/test_expand_filespecs.py | 75 +++++++++++++++++++ 2 files changed, 83 insertions(+), 5 deletions(-) create mode 100644 lib/iris/tests/unit/io/test_expand_filespecs.py diff --git a/lib/iris/io/__init__.py b/lib/iris/io/__init__.py index b431e814c1..d73b819af8 100644 --- a/lib/iris/io/__init__.py +++ b/lib/iris/io/__init__.py @@ -147,23 +147,26 @@ def expand_filespecs(file_specs): File paths which may contain '~' elements or wildcards. Returns: - A list of matching file paths. If any of the file-specs matches no + A list of matching absolute file paths. If any of the file-specs matches no existing files, an exception is raised. """ - # Remove any hostname component - currently unused - filenames = [os.path.expanduser(fn[2:] if fn.startswith('//') else fn) + # Remove any hostname component - currently unused - expand paths as absolutes + filenames = [os.path.abspath(os.path.expanduser(fn[2:] if fn.startswith('//') else fn)) for fn in file_specs] # Try to expand all filenames as globs glob_expanded = {fn : sorted(glob.glob(fn)) for fn in filenames} # If any of the specs expanded to an empty list then raise an error - value_lists = glob_expanded.values() + value_list_raw = glob_expanded.values() + + # Here is a term to sort value_lists alphabetically, for consistency + value_lists = sorted(value_list_raw) if not all(value_lists): raise IOError("One or more of the files specified did not exist %s." % ["%s expanded to %s" % (pattern, expanded if expanded else "empty") - for pattern, expanded in six.iteritems(glob_expanded)]) + for pattern, expanded in sorted(six.iteritems(glob_expanded))]) return sum(value_lists, []) diff --git a/lib/iris/tests/unit/io/test_expand_filespecs.py b/lib/iris/tests/unit/io/test_expand_filespecs.py new file mode 100644 index 0000000000..79457d9c09 --- /dev/null +++ b/lib/iris/tests/unit/io/test_expand_filespecs.py @@ -0,0 +1,75 @@ +# (C) British Crown Copyright 2016, Met Office +# +# This file is part of Iris. +# +# Iris is free software: you can redistribute it and/or modify it under +# the terms of the GNU Lesser General Public License as published by the +# Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. +# +# Iris is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU Lesser General Public License for more details. +# +# You should have received a copy of the GNU Lesser General Public License +# along with Iris. If not, see . +"""Unit tests for the `iris.io.expand_filespecs` function.""" + +from __future__ import (absolute_import, division, print_function) +from six.moves import (filter, input, map, range, zip) # noqa + +# Import iris.tests first so that some things can be initialised before +# importing anything else. +import iris.tests as tests + +import os +import tempfile +import shutil + +import iris.io as iio + + +class TestExpandFilespecs(tests.IrisTest): + + def setUp(self): + tests.IrisTest.setUp(self) + self.tmpdir = tempfile.mkdtemp() + self.fnames = ['a.foo', 'b.txt'] + for fname in self.fnames: + with open(os.path.join(self.tmpdir, fname), 'w') as fh: + fh.write('anything') + + def tearDown(self): + shutil.rmtree(self.tmpdir) + + def test_absolute_path(self): + result = iio.expand_filespecs([os.path.join(self.tmpdir, '*')]) + expected = [os.path.join(self.tmpdir, fname) for fname in self.fnames] + self.assertEqual(result, expected) + + def test_double_slash(self): + product = iio.expand_filespecs(['//' + os.path.join(self.tmpdir, '*')]) + predicted = [os.path.join(self.tmpdir, fname) for fname in self.fnames] + self.assertEqual(product, predicted) + + def test_relative_path(self): + os.chdir(self.tmpdir) + item_out = iio.expand_filespecs(['*']) + item_in = [os.path.join(self.tmpdir, fname) for fname in self.fnames] + self.assertEqual(item_out, item_in) + + def test_no_files_found(self): + msg = 'b expanded to empty' + with self.assertRaisesRegexp(IOError, msg): + iio.expand_filespecs([self.tmpdir + '_b']) + + def test_files_and_none(self): + msg = 'b expanded to empty.*expanded to .*b.txt' + with self.assertRaisesRegexp(IOError, msg): + iio.expand_filespecs([self.tmpdir + '_b', + os.path.join(self.tmpdir, '*')]) + + +if __name__ == "__main__": + tests.main() From d963263a23d14fab71cb597b8f3e9ddff81ac6c5 Mon Sep 17 00:00:00 2001 From: Phil Elson Date: Tue, 24 Jan 2017 07:12:51 +0000 Subject: [PATCH 2/3] Ordered the inputs so that filenames are loaded in the given order. --- lib/iris/io/__init__.py | 37 +++++++++++-------- lib/iris/tests/__init__.py | 10 +++++ .../tests/unit/io/test_expand_filespecs.py | 34 ++++++++++++++--- 3 files changed, 60 insertions(+), 21 deletions(-) diff --git a/lib/iris/io/__init__.py b/lib/iris/io/__init__.py index d73b819af8..66330ba4ba 100644 --- a/lib/iris/io/__init__.py +++ b/lib/iris/io/__init__.py @@ -23,6 +23,7 @@ from six.moves import (filter, input, map, range, zip) # noqa import six +from collections import OrderedDict import glob import os.path import re @@ -147,28 +148,34 @@ def expand_filespecs(file_specs): File paths which may contain '~' elements or wildcards. Returns: - A list of matching absolute file paths. If any of the file-specs matches no - existing files, an exception is raised. + A well-ordered list of matching absolute file paths. + If any of the file-specs match no existing files, an + exception is raised. """ - # Remove any hostname component - currently unused - expand paths as absolutes - filenames = [os.path.abspath(os.path.expanduser(fn[2:] if fn.startswith('//') else fn)) + # Remove any hostname component - currently unused + filenames = [os.path.abspath(os.path.expanduser( + fn[2:] if fn.startswith('//') else fn)) for fn in file_specs] # Try to expand all filenames as globs - glob_expanded = {fn : sorted(glob.glob(fn)) for fn in filenames} + glob_expanded = OrderedDict([[fn, sorted(glob.glob(fn))] + for fn in filenames]) # If any of the specs expanded to an empty list then raise an error - value_list_raw = glob_expanded.values() - - # Here is a term to sort value_lists alphabetically, for consistency - value_lists = sorted(value_list_raw) - if not all(value_lists): - raise IOError("One or more of the files specified did not exist %s." % - ["%s expanded to %s" % (pattern, expanded if expanded else "empty") - for pattern, expanded in sorted(six.iteritems(glob_expanded))]) - - return sum(value_lists, []) + all_expanded = glob_expanded.values() + + if not all(all_expanded): + msg = "One or more of the files specified did not exist:" + for pattern, expanded in six.iteritems(glob_expanded): + if expanded: + file_list = '\n - {}'.format(', '.join(expanded)) + else: + file_list = '' + msg += '\n - "{}" matched {} file(s){}'.format(pattern, len(expanded), file_list) + raise IOError(msg) + + return [fname for fnames in all_expanded for fname in fnames] def load_files(filenames, callback, constraints=None): diff --git a/lib/iris/tests/__init__.py b/lib/iris/tests/__init__.py index 480239c295..a7890832e6 100644 --- a/lib/iris/tests/__init__.py +++ b/lib/iris/tests/__init__.py @@ -249,6 +249,16 @@ def get_result_path(relative_path): relative_path = os.path.join(*relative_path) return os.path.abspath(os.path.join(_RESULT_PATH, relative_path)) + def assertStringEqual(self, reference_str, test_str, + type_comparison_name='strings'): + if reference_str != test_str: + diff = '\n'.join(difflib.unified_diff(reference_str.splitlines(), + test_str.splitlines(), + 'Reference', 'Test result', + '', '', 0)) + self.fail("{} do not match:\n{}".format(type_comparison_name, + diff)) + def result_path(self, basename=None, ext=''): """ Return the full path to a test result, generated from the \ diff --git a/lib/iris/tests/unit/io/test_expand_filespecs.py b/lib/iris/tests/unit/io/test_expand_filespecs.py index 79457d9c09..5339638ef8 100644 --- a/lib/iris/tests/unit/io/test_expand_filespecs.py +++ b/lib/iris/tests/unit/io/test_expand_filespecs.py @@ -1,4 +1,4 @@ -# (C) British Crown Copyright 2016, Met Office +# (C) British Crown Copyright 2017, Met Office # # This file is part of Iris. # @@ -26,12 +26,12 @@ import os import tempfile import shutil +import textwrap import iris.io as iio class TestExpandFilespecs(tests.IrisTest): - def setUp(self): tests.IrisTest.setUp(self) self.tmpdir = tempfile.mkdtemp() @@ -59,16 +59,38 @@ def test_relative_path(self): item_in = [os.path.join(self.tmpdir, fname) for fname in self.fnames] self.assertEqual(item_out, item_in) + def test_return_order(self): + # It is really quite important what order we return the + # files. They should be in the order that was provided, + # so that we can control the order of load (for instance, + # this can be used with PP files to ensure that there is + # a surface reference). + patterns = [os.path.join(self.tmpdir, 'a.*'), + os.path.join(self.tmpdir, 'b.*')] + expected = [os.path.join(self.tmpdir, fname) + for fname in ['a.foo', 'b.txt']] + result = iio.expand_filespecs(patterns) + self.assertEqual(result, expected) + result = iio.expand_filespecs(patterns[::-1]) + self.assertEqual(result, expected[::-1]) + def test_no_files_found(self): - msg = 'b expanded to empty' + msg = r'_b\" matched 0 file\(s\)' with self.assertRaisesRegexp(IOError, msg): iio.expand_filespecs([self.tmpdir + '_b']) def test_files_and_none(self): - msg = 'b expanded to empty.*expanded to .*b.txt' - with self.assertRaisesRegexp(IOError, msg): + with self.assertRaises(IOError) as err: iio.expand_filespecs([self.tmpdir + '_b', - os.path.join(self.tmpdir, '*')]) + os.path.join(self.tmpdir, '*')]) + expected = textwrap.dedent(""" + One or more of the files specified did not exist: + - "{0}_b" matched 0 file(s) + - "{0}/*" matched 2 file(s) + - {0}/a.foo, {0}/b.txt + """).strip().format(self.tmpdir) + + self.assertStringEqual(str(err.exception), expected) if __name__ == "__main__": From 2a765b576d7e1772142673c74f0e438e001109bc Mon Sep 17 00:00:00 2001 From: Phil Elson Date: Mon, 23 Oct 2017 15:58:04 +0100 Subject: [PATCH 3/3] Addressed test concern about shared temporary directory. --- lib/iris/io/__init__.py | 3 ++- .../tests/unit/io/test_expand_filespecs.py | 26 ++++++++++++------- 2 files changed, 18 insertions(+), 11 deletions(-) diff --git a/lib/iris/io/__init__.py b/lib/iris/io/__init__.py index 66330ba4ba..6eeaf8060f 100644 --- a/lib/iris/io/__init__.py +++ b/lib/iris/io/__init__.py @@ -172,7 +172,8 @@ def expand_filespecs(file_specs): file_list = '\n - {}'.format(', '.join(expanded)) else: file_list = '' - msg += '\n - "{}" matched {} file(s){}'.format(pattern, len(expanded), file_list) + msg += '\n - "{}" matched {} file(s){}'.format( + pattern, len(expanded), file_list) raise IOError(msg) return [fname for fnames in all_expanded for fname in fnames] diff --git a/lib/iris/tests/unit/io/test_expand_filespecs.py b/lib/iris/tests/unit/io/test_expand_filespecs.py index 5339638ef8..387291241d 100644 --- a/lib/iris/tests/unit/io/test_expand_filespecs.py +++ b/lib/iris/tests/unit/io/test_expand_filespecs.py @@ -34,7 +34,7 @@ class TestExpandFilespecs(tests.IrisTest): def setUp(self): tests.IrisTest.setUp(self) - self.tmpdir = tempfile.mkdtemp() + self.tmpdir = os.path.realpath(tempfile.mkdtemp()) self.fnames = ['a.foo', 'b.txt'] for fname in self.fnames: with open(os.path.join(self.tmpdir, fname), 'w') as fh: @@ -54,10 +54,15 @@ def test_double_slash(self): self.assertEqual(product, predicted) def test_relative_path(self): - os.chdir(self.tmpdir) - item_out = iio.expand_filespecs(['*']) - item_in = [os.path.join(self.tmpdir, fname) for fname in self.fnames] - self.assertEqual(item_out, item_in) + cwd = os.getcwd() + try: + os.chdir(self.tmpdir) + item_out = iio.expand_filespecs(['*']) + item_in = [os.path.join(self.tmpdir, fname) + for fname in self.fnames] + self.assertEqual(item_out, item_in) + finally: + os.chdir(cwd) def test_return_order(self): # It is really quite important what order we return the @@ -75,17 +80,18 @@ def test_return_order(self): self.assertEqual(result, expected[::-1]) def test_no_files_found(self): - msg = r'_b\" matched 0 file\(s\)' + msg = r'\/no_exist.txt\" matched 0 file\(s\)' with self.assertRaisesRegexp(IOError, msg): - iio.expand_filespecs([self.tmpdir + '_b']) + iio.expand_filespecs([os.path.join(self.tmpdir, 'no_exist.txt')]) def test_files_and_none(self): with self.assertRaises(IOError) as err: - iio.expand_filespecs([self.tmpdir + '_b', - os.path.join(self.tmpdir, '*')]) + iio.expand_filespecs( + [os.path.join(self.tmpdir, 'does_not_exist.txt'), + os.path.join(self.tmpdir, '*')]) expected = textwrap.dedent(""" One or more of the files specified did not exist: - - "{0}_b" matched 0 file(s) + - "{0}/does_not_exist.txt" matched 0 file(s) - "{0}/*" matched 2 file(s) - {0}/a.foo, {0}/b.txt """).strip().format(self.tmpdir)