tests: test file/line attribution in config errors

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

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')

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.