From 15910debb24a359b297f0102a5135be7bdbceacf Mon Sep 17 00:00:00 2001
From: Todd Gamblin <tgamblin@llnl.gov>
Date: Sun, 3 Jun 2018 00:45:20 -0700
Subject: [PATCH] tests: test file/line attribution in config errors
---
lib/spack/spack/config.py | 77 ++++++++++++++++++++------------
lib/spack/spack/test/config.py | 73 +++++++++++++++++++++++++++++-
lib/spack/spack/test/conftest.py | 20 +++++++++
3 files changed, 140 insertions(+), 30 deletions(-)
diff --git a/lib/spack/spack/config.py b/lib/spack/spack/config.py
index d54dd74d5a..ec37c0a972 100644
--- a/lib/spack/spack/config.py
+++ b/lib/spack/spack/config.py
@@ -187,7 +187,6 @@ def get_section(self, section):
return self.sections[section]
def write_section(self, section):
- import jsonschema
filename = self.get_section_filename(section)
data = self.get_section(section)
try:
@@ -195,8 +194,6 @@ def write_section(self, section):
with open(filename, 'w') as f:
_validate_section(data, section_schemas[section])
syaml.dump(data, stream=f, default_flow_style=False)
- except jsonschema.ValidationError as e:
- raise ConfigSanityError(e, data)
except (yaml.YAMLError, IOError) as e:
raise ConfigFileError(
"Error writing to config file: '%s'" % str(e))
@@ -531,8 +528,9 @@ def scopes():
def _validate_section_name(section):
"""Exit if the section is not a valid section."""
if section not in section_schemas:
- tty.die("Invalid config section: '%s'. Options are: %s"
- % (section, " ".join(section_schemas.keys())))
+ raise ConfigSectionError(
+ "Invalid config section: '%s'. Options are: %s"
+ % (section, " ".join(section_schemas.keys())))
def _validate_section(data, schema):
@@ -697,6 +695,10 @@ class ConfigError(SpackError):
"""Superclass for all Spack config related errors."""
+class ConfigSectionError(ConfigError):
+ """Error for referring to a bad config section name in a configuration."""
+
+
class ConfigFileError(ConfigError):
"""Issue reading or accessing a configuration file."""
@@ -705,10 +707,38 @@ class ConfigFormatError(ConfigError):
"""Raised when a configuration format does not match its schema."""
def __init__(self, validation_error, data):
- # Try to get line number from erroneous instance and its parent
- instance_mark = getattr(validation_error.instance, '_start_mark', None)
- parent_mark = getattr(validation_error.parent, '_start_mark', None)
- path = [str(s) for s in getattr(validation_error, 'path', None)]
+ location = '<unknown file>'
+ mark = self._get_mark(validation_error, data)
+ if mark:
+ location = '%s' % mark.name
+ if mark.line is not None:
+ location += ':%d' % (mark.line + 1)
+
+ message = '%s: %s' % (location, validation_error.message)
+ super(ConfigError, self).__init__(message)
+
+ def _get_mark(self, validation_error, data):
+ """Get the file/line mark fo a validation error from a Spack YAML file.
+ """
+ def _get_mark_or_first_member_mark(obj):
+ # mark of object itelf
+ mark = getattr(obj, '_start_mark', None)
+ if mark:
+ return mark
+
+ # mark of first member if it is a container
+ if isinstance(obj, (list, dict)):
+ first_member = next(iter(obj), None)
+ if first_member:
+ mark = getattr(first_member, '_start_mark', None)
+ if mark:
+ return mark
+
+ # Try various places, starting with instance and parent
+ for obj in (validation_error.instance, validation_error.parent):
+ mark = _get_mark_or_first_member_mark(obj)
+ if mark:
+ return mark
def get_path(path, data):
if path:
@@ -719,29 +749,18 @@ def get_path(path, data):
# Try really hard to get the parent (which sometimes is not
# set) This digs it out of the validated structure if it's not
# on the validation_error.
- if path and not parent_mark:
- parent_path = list(path)[:-1]
- parent = get_path(parent_path, data)
+ path = validation_error.path
+ if path:
+ parent = get_path(list(path)[:-1], data)
if path[-1] in parent:
if isinstance(parent, dict):
- keylist = parent.keys()
+ keylist = list(parent.keys())
elif isinstance(parent, list):
keylist = parent
idx = keylist.index(path[-1])
- parent_mark = getattr(keylist[idx], '_start_mark', None)
-
- if instance_mark:
- location = '%s:%d' % (instance_mark.name, instance_mark.line + 1)
- elif parent_mark:
- location = '%s:%d' % (parent_mark.name, parent_mark.line + 1)
- elif path:
- location = 'At ' + ':'.join(path)
- else:
- location = '<unknown line>'
-
- message = '%s: %s' % (location, validation_error.message)
- super(ConfigError, self).__init__(message)
-
+ mark = getattr(keylist[idx], '_start_mark', None)
+ if mark:
+ return mark
-class ConfigSanityError(ConfigFormatError):
- """Same as ConfigFormatError, raised when config is written by Spack."""
+ # give up and return None if nothing worked
+ return None
diff --git a/lib/spack/spack/test/config.py b/lib/spack/spack/test/config.py
index 7b1e7d0ee1..95cd941547 100644
--- a/lib/spack/spack/test/config.py
+++ b/lib/spack/spack/test/config.py
@@ -518,7 +518,7 @@ def test_internal_config_from_data():
def test_keys_are_ordered():
-
+ """Test that keys in Spack YAML files retain their order from the file."""
expected_order = (
'bin',
'man',
@@ -543,3 +543,74 @@ def test_keys_are_ordered():
for actual, expected in zip(prefix_inspections, expected_order):
assert actual == expected
+
+
+def test_config_format_error(mutable_config):
+ """This is raised when we try to write a bad configuration."""
+ with pytest.raises(spack.config.ConfigFormatError):
+ spack.config.set('compilers', {'bad': 'data'}, scope='site')
+
+
+def get_config_error(filename, schema, yaml_string):
+ """Parse a YAML string and return the resulting ConfigFormatError.
+
+ Fail if there is no ConfigFormatError
+ """
+ with open(filename, 'w') as f:
+ f.write(yaml_string)
+
+ # parse and return error, or fail.
+ try:
+ spack.config._read_config_file(filename, schema)
+ except spack.config.ConfigFormatError as e:
+ return e
+ else:
+ pytest.fail('ConfigFormatError was not raised!')
+
+
+def test_config_parse_dict_in_list(tmpdir):
+ with tmpdir.as_cwd():
+ e = get_config_error(
+ 'repos.yaml', spack.schema.repos.schema, """\
+repos:
+- https://foobar.com/foo
+- https://foobar.com/bar
+- error:
+ - abcdef
+- https://foobar.com/baz
+""")
+ assert "repos.yaml:4" in str(e)
+
+
+def test_config_parse_str_not_bool(tmpdir):
+ with tmpdir.as_cwd():
+ e = get_config_error(
+ 'config.yaml', spack.schema.config.schema, """\
+config:
+ verify_ssl: False
+ checksum: foobar
+ dirty: True
+""")
+ assert "config.yaml:3" in str(e)
+
+
+def test_config_parse_list_in_dict(tmpdir):
+ with tmpdir.as_cwd():
+ e = get_config_error(
+ 'mirrors.yaml', spack.schema.mirrors.schema, """\
+mirrors:
+ foo: http://foobar.com/baz
+ bar: http://barbaz.com/foo
+ baz: http://bazfoo.com/bar
+ travis: [1, 2, 3]
+""")
+ assert "mirrors.yaml:5" in str(e)
+
+
+def test_bad_config_section(config):
+ """Test that getting or setting a bad section gives an error."""
+ with pytest.raises(spack.config.ConfigSectionError):
+ spack.config.set('foobar', 'foobar')
+
+ with pytest.raises(spack.config.ConfigSectionError):
+ spack.config.get('foobar')
diff --git a/lib/spack/spack/test/conftest.py b/lib/spack/spack/test/conftest.py
index d1f96451d4..71c4a95446 100644
--- a/lib/spack/spack/test/conftest.py
+++ b/lib/spack/spack/test/conftest.py
@@ -254,6 +254,26 @@ def config(configuration_dir):
spack.package_prefs.PackagePrefs.clear_caches()
+@pytest.fixture(scope='function')
+def mutable_config(tmpdir_factory, configuration_dir, config):
+ """Like config, but tests can modify the configuration."""
+ spack.package_prefs.PackagePrefs.clear_caches()
+
+ mutable_dir = tmpdir_factory.mktemp('mutable_config').join('tmp')
+ configuration_dir.copy(mutable_dir)
+
+ real_configuration = spack.config.config
+
+ spack.config.config = spack.config.Configuration(
+ *[spack.config.ConfigScope(name, str(mutable_dir))
+ for name in ['site', 'system', 'user']])
+
+ yield spack.config.config
+
+ spack.config.config = real_configuration
+ spack.package_prefs.PackagePrefs.clear_caches()
+
+
def _populate(mock_db):
"""Populate a mock database with packages.
--
GitLab