From 02578fddc7d2290c2b960c2841e37366bc552c9a Mon Sep 17 00:00:00 2001 From: Angelos Evripiotis Date: Wed, 20 Mar 2019 10:18:55 +0000 Subject: buildstream/_yaml: Provenance, fix type in doc str --- buildstream/_yaml.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/buildstream/_yaml.py b/buildstream/_yaml.py index 73818c209..26da5e3be 100644 --- a/buildstream/_yaml.py +++ b/buildstream/_yaml.py @@ -54,7 +54,7 @@ class ProvenanceFile(): # # Args: # node (dict, list, value): A binding to the originally parsed value -# filename (string): The filename the node was loaded from +# filename (str): The filename the node was loaded from # toplevel (dict): The toplevel of the loaded file, suitable for later dumps # line (int): The line number where node was parsed # col (int): The column number where node was parsed -- cgit v1.2.1 From 3bb273564e0f0b3666217ba087f319ad5c8d27d7 Mon Sep 17 00:00:00 2001 From: Angelos Evripiotis Date: Wed, 20 Mar 2019 14:03:22 +0000 Subject: test: Add yaml_file_get_provenance Add a new helper - testutils.yaml_file_get_provenance, this will let us make less fragile assertions about the presence of provenance in BuildStream output. --- tests/testutils/__init__.py | 1 + tests/testutils/yaml.py | 28 ++++++++++++++++++++++++++++ 2 files changed, 29 insertions(+) create mode 100644 tests/testutils/yaml.py diff --git a/tests/testutils/__init__.py b/tests/testutils/__init__.py index 173cc7c3e..67378c959 100644 --- a/tests/testutils/__init__.py +++ b/tests/testutils/__init__.py @@ -29,3 +29,4 @@ from .element_generators import create_element_size, update_element_size from .junction import generate_junction from .runner_integration import wait_for_cache_granularity from .python_repo import setup_pypi_repo +from .yaml import yaml_file_get_provenance diff --git a/tests/testutils/yaml.py b/tests/testutils/yaml.py new file mode 100644 index 000000000..4706a27bb --- /dev/null +++ b/tests/testutils/yaml.py @@ -0,0 +1,28 @@ +from buildstream import _yaml + + +# yaml_file_get_provenance() +# +# Load a yaml file and return it's _yaml.Provenance object. +# +# This is useful for checking the provenance in BuildStream output is as +# expected. +# +# Args: +# path (str): The path to the file to be loaded +# shortname (str): How the path should appear in the error +# key (str): Optional key to look up in the loaded file +# indices (list of indexes): Optional index path, in the case of list values +# +# Returns: +# The Provenance of the dict, member or list element +# +def yaml_file_get_provenance(path, shortname, key=None, indices=None): + with open(path) as data: + element_yaml = _yaml.load_data( + data, + _yaml.ProvenanceFile(path, shortname, project=None), + ) + provenance = _yaml.node_get_provenance(element_yaml, key, indices) + assert provenance is not None + return provenance -- cgit v1.2.1 From 85d13fc458097e7db05a7c4efdb0cd9c75126727 Mon Sep 17 00:00:00 2001 From: Angelos Evripiotis Date: Tue, 19 Mar 2019 10:58:46 +0000 Subject: loader: provenance in LoadErrors from _get_loader Give better error messages to the user by including provenance. Pass 'provenance' through to places where we raise exceptions in _get_loader. For example, here you can see the provenance information now included (the bit before the ':'): callHello.bst [line 8 column 4]: Could not find the project.conf file in the project referred to by junction element 'hello-junction.bst'. --- buildstream/_loader/loader.py | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/buildstream/_loader/loader.py b/buildstream/_loader/loader.py index 804dc5f7e..2a67a6e2a 100644 --- a/buildstream/_loader/loader.py +++ b/buildstream/_loader/loader.py @@ -266,7 +266,7 @@ class Loader(): if dep.junction: self._load_file(dep.junction, rewritable, ticker, fetch_subprojects, yaml_cache, dep.provenance) loader = self._get_loader(dep.junction, rewritable=rewritable, ticker=ticker, - fetch_subprojects=fetch_subprojects) + fetch_subprojects=fetch_subprojects, provenance=dep.provenance) else: loader = self @@ -474,7 +474,13 @@ class Loader(): # Raises: LoadError # # Returns: A Loader or None if specified junction does not exist - def _get_loader(self, filename, *, rewritable=False, ticker=None, level=0, fetch_subprojects=False): + def _get_loader(self, filename, *, rewritable=False, ticker=None, level=0, + fetch_subprojects=False, provenance=None): + + provenance_str = "" + if provenance is not None: + provenance_str = "{}: ".format(provenance) + # return previously determined result if filename in self._loaders: loader = self._loaders[filename] @@ -483,8 +489,8 @@ class Loader(): # do not allow junctions with the same name in different # subprojects raise LoadError(LoadErrorReason.CONFLICTING_JUNCTION, - "Conflicting junction {} in subprojects, define junction in {}" - .format(filename, self.project.name)) + "{}Conflicting junction {} in subprojects, define junction in {}" + .format(provenance_str, filename, self.project.name)) return loader @@ -492,7 +498,8 @@ class Loader(): # junctions in the parent take precedence over junctions defined # in subprojects loader = self._parent._get_loader(filename, rewritable=rewritable, ticker=ticker, - level=level + 1, fetch_subprojects=fetch_subprojects) + level=level + 1, fetch_subprojects=fetch_subprojects, + provenance=provenance) if loader: self._loaders[filename] = loader return loader @@ -517,7 +524,8 @@ class Loader(): meta_element = self._collect_element(self._elements[filename]) if meta_element.kind != 'junction': raise LoadError(LoadErrorReason.INVALID_DATA, - "{}: Expected junction but element kind is {}".format(filename, meta_element.kind)) + "{}{}: Expected junction but element kind is {}".format( + provenance_str, filename, meta_element.kind)) element = Element._new_from_meta(meta_element) element._preflight() @@ -535,7 +543,7 @@ class Loader(): else: detail = "Try fetching the project with `bst source fetch {}`".format(filename) raise LoadError(LoadErrorReason.SUBPROJECT_FETCH_NEEDED, - "Subproject fetch needed for junction: {}".format(filename), + "{}Subproject fetch needed for junction: {}".format(provenance_str, filename), detail=detail) # Handle the case where a subproject has no ref @@ -543,7 +551,7 @@ class Loader(): elif source.get_consistency() == Consistency.INCONSISTENT: detail = "Try tracking the junction element with `bst source track {}`".format(filename) raise LoadError(LoadErrorReason.SUBPROJECT_INCONSISTENT, - "Subproject has no ref for junction: {}".format(filename), + "{}Subproject has no ref for junction: {}".format(provenance_str, filename), detail=detail) workspace = element._get_workspace() @@ -571,7 +579,7 @@ class Loader(): except LoadError as e: if e.reason == LoadErrorReason.MISSING_PROJECT_CONF: message = ( - "Could not find the project.conf file in the project " + provenance_str + "Could not find the project.conf file in the project " "referred to by junction element '{}'.".format(element.name) ) if element.path: -- cgit v1.2.1 From 7a928a14d3357543789dc22b2f4c2a04f7a89594 Mon Sep 17 00:00:00 2001 From: Angelos Evripiotis Date: Wed, 20 Mar 2019 10:15:35 +0000 Subject: tests: ensure provenance for _get_loader errors Make the tests that currently cover _get_loader ensure that we are getting the expected provenance. Note that for some tests, we must use yaml_file_get_provenance, as the generated yaml is not stable across versions of ruamel. In later work we may replace all instances of provenance string tests with yaml_file_get_provenance, as it will be more robust to future changes. --- tests/format/junctions.py | 14 ++++++++++++++ tests/frontend/buildcheckout.py | 7 ++++++- tests/frontend/fetch.py | 7 ++++++- tests/frontend/show.py | 21 +++++++++++++++------ tests/frontend/track.py | 12 +++++++++++- 5 files changed, 52 insertions(+), 9 deletions(-) diff --git a/tests/format/junctions.py b/tests/format/junctions.py index 210455cb4..fc2dbc155 100644 --- a/tests/format/junctions.py +++ b/tests/format/junctions.py @@ -81,6 +81,9 @@ def test_junction_missing_project_conf(cli, datafiles): result = cli.run(project=project, args=['build', 'app.bst']) result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.INVALID_JUNCTION) + # Assert that we have the expected provenance encoded into the error + assert "app.bst [line 6 column 2]" in result.stderr + @pytest.mark.datafiles(DATA_DIR) def test_workspaced_junction_missing_project_conf(cli, datafiles): @@ -102,6 +105,9 @@ def test_workspaced_junction_missing_project_conf(cli, datafiles): result = cli.run(project=project, args=['build', 'app.bst']) result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.INVALID_JUNCTION) + # Assert that we have the expected provenance encoded into the error + assert "app.bst [line 6 column 2]" in result.stderr + @pytest.mark.datafiles(DATA_DIR) def test_build_of_same_junction_used_twice(cli, datafiles): @@ -203,6 +209,8 @@ def test_nested_conflict(cli, datafiles): result = cli.run(project=project, args=['build', 'target.bst']) result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.CONFLICTING_JUNCTION) + assert "bar.bst:target.bst [line 3 column 2]" in result.stderr + # Test that we error correctly when the junction element itself is missing @pytest.mark.datafiles(DATA_DIR) @@ -316,6 +324,9 @@ def test_git_show(cli, tmpdir, datafiles): result = cli.run(project=project, args=['show', 'target.bst']) result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.SUBPROJECT_FETCH_NEEDED) + # Assert that we have the expected provenance encoded into the error + assert "target.bst [line 3 column 2]" in result.stderr + # Explicitly fetch subproject result = cli.run(project=project, args=['source', 'fetch', 'base.bst']) result.assert_success() @@ -380,6 +391,9 @@ def test_git_missing_project_conf(cli, tmpdir, datafiles): result = cli.run(project=project, args=['build', 'app.bst']) result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.INVALID_JUNCTION) + # Assert that we have the expected provenance encoded into the error + assert "app.bst [line 6 column 2]" in result.stderr + @pytest.mark.datafiles(DATA_DIR) def test_cross_junction_names(cli, datafiles): diff --git a/tests/frontend/buildcheckout.py b/tests/frontend/buildcheckout.py index dc3c5e4d4..5145b1222 100644 --- a/tests/frontend/buildcheckout.py +++ b/tests/frontend/buildcheckout.py @@ -9,7 +9,7 @@ import subprocess import pytest from tests.testutils.site import IS_WINDOWS -from tests.testutils import generate_junction +from tests.testutils import generate_junction, yaml_file_get_provenance from buildstream.plugintestutils import cli # pylint: disable=unused-import from buildstream import _yaml @@ -457,6 +457,11 @@ def test_inconsistent_junction(cli, tmpdir, datafiles, ref_storage): result = cli.run(project=project, args=['build', 'junction-dep.bst']) result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.SUBPROJECT_INCONSISTENT) + # Assert that we have the expected provenance encoded into the error + provenance = yaml_file_get_provenance( + element_path, 'junction-dep.bst', key='depends', indices=[0]) + assert str(provenance) in result.stderr + @pytest.mark.datafiles(DATA_DIR) @pytest.mark.parametrize("ref_storage", [('inline'), ('project.refs')]) diff --git a/tests/frontend/fetch.py b/tests/frontend/fetch.py index 1a2c40602..8282e2131 100644 --- a/tests/frontend/fetch.py +++ b/tests/frontend/fetch.py @@ -4,7 +4,7 @@ import os import pytest -from tests.testutils import create_repo, generate_junction +from tests.testutils import create_repo, generate_junction, yaml_file_get_provenance from buildstream.plugintestutils import cli # pylint: disable=unused-import from buildstream import _yaml @@ -160,3 +160,8 @@ def test_inconsistent_junction(cli, tmpdir, datafiles, ref_storage): # informing the user to track the junction first result = cli.run(project=project, args=['source', 'fetch', 'junction-dep.bst']) result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.SUBPROJECT_INCONSISTENT) + + # Assert that we have the expected provenance encoded into the error + provenance = yaml_file_get_provenance( + element_path, 'junction-dep.bst', key='depends', indices=[0]) + assert str(provenance) in result.stderr diff --git a/tests/frontend/show.py b/tests/frontend/show.py index bb2fc2ee2..7e5ebfb77 100644 --- a/tests/frontend/show.py +++ b/tests/frontend/show.py @@ -6,7 +6,7 @@ import sys import shutil import itertools import pytest -from tests.testutils import generate_junction +from tests.testutils import generate_junction, yaml_file_get_provenance from buildstream.plugintestutils import cli # pylint: disable=unused-import from buildstream import _yaml from buildstream._exceptions import ErrorDomain, LoadErrorReason @@ -277,8 +277,7 @@ def test_unfetched_junction(cli, tmpdir, datafiles, ref_storage, element_name): @pytest.mark.datafiles(os.path.join(DATA_DIR, 'project')) @pytest.mark.parametrize("ref_storage", [('inline'), ('project.refs')]) -@pytest.mark.parametrize("element_name", ['junction-dep.bst', 'junction.bst:import-etc.bst']) -def test_inconsistent_junction(cli, tmpdir, datafiles, ref_storage, element_name): +def test_inconsistent_junction(cli, tmpdir, datafiles, ref_storage): project = str(datafiles) subproject_path = os.path.join(project, 'files', 'sub-project') junction_path = os.path.join(project, 'elements', 'junction.bst') @@ -305,10 +304,20 @@ def test_inconsistent_junction(cli, tmpdir, datafiles, ref_storage, element_name _yaml.dump(element, element_path) # Assert the correct error when trying to show the pipeline - result = cli.run(project=project, silent=True, args=[ - 'show', element_name]) + dep_result = cli.run(project=project, silent=True, args=[ + 'show', 'junction-dep.bst']) + + # Assert the correct error when trying to show the pipeline + etc_result = cli.run(project=project, silent=True, args=[ + 'show', 'junction.bst:import-etc.bst']) + + # Assert that we have the expected provenance encoded into the error + provenance = yaml_file_get_provenance( + element_path, 'junction-dep.bst', key='depends', indices=[0]) + assert str(provenance) in dep_result.stderr - result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.SUBPROJECT_INCONSISTENT) + dep_result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.SUBPROJECT_INCONSISTENT) + etc_result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.SUBPROJECT_INCONSISTENT) @pytest.mark.datafiles(os.path.join(DATA_DIR, 'project')) diff --git a/tests/frontend/track.py b/tests/frontend/track.py index 5a6460d3d..13d1d4646 100644 --- a/tests/frontend/track.py +++ b/tests/frontend/track.py @@ -4,7 +4,7 @@ import stat import os import pytest -from tests.testutils import create_repo, generate_junction +from tests.testutils import create_repo, generate_junction, yaml_file_get_provenance from buildstream.plugintestutils import cli # pylint: disable=unused-import from buildstream._exceptions import ErrorDomain, LoadErrorReason @@ -274,6 +274,11 @@ def test_inconsistent_junction(cli, tmpdir, datafiles, ref_storage): result = cli.run(project=project, args=['source', 'track', 'junction-dep.bst']) result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.SUBPROJECT_INCONSISTENT) + # Assert that we have the expected provenance encoded into the error + provenance = yaml_file_get_provenance( + element_path, 'junction-dep.bst', key='depends', indices=[0]) + assert str(provenance) in result.stderr + @pytest.mark.datafiles(DATA_DIR) @pytest.mark.parametrize("ref_storage", [('inline'), ('project.refs')]) @@ -307,6 +312,11 @@ def test_junction_element(cli, tmpdir, datafiles, ref_storage): result = cli.run(project=project, args=['show', 'junction-dep.bst']) result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.SUBPROJECT_INCONSISTENT) + # Assert that we have the expected provenance encoded into the error + provenance = yaml_file_get_provenance( + element_path, 'junction-dep.bst', key='depends', indices=[0]) + assert str(provenance) in result.stderr + # Now track the junction itself result = cli.run(project=project, args=['source', 'track', 'junction.bst']) result.assert_success() -- cgit v1.2.1 From 000f7eb3959f5a07a94af776b6cee96caa6df6b5 Mon Sep 17 00:00:00 2001 From: Angelos Evripiotis Date: Wed, 20 Mar 2019 10:49:58 +0000 Subject: tests: Add test_invalid_junctiondep_not_a_junction Test that we error correctly when we junction-depend on a non-junction. --- tests/format/junctions.py | 13 +++++++++++++ .../format/junctions/invalid/junctiondep-not-a-junction.bst | 4 ++++ 2 files changed, 17 insertions(+) create mode 100644 tests/format/junctions/invalid/junctiondep-not-a-junction.bst diff --git a/tests/format/junctions.py b/tests/format/junctions.py index fc2dbc155..3a215761a 100644 --- a/tests/format/junctions.py +++ b/tests/format/junctions.py @@ -251,6 +251,19 @@ def test_invalid_junction_dep(cli, datafiles): result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.INVALID_DATA) +# Test that we error correctly when we junction-depend on a non-junction +@pytest.mark.datafiles(DATA_DIR) +def test_invalid_junctiondep_not_a_junction(cli, datafiles): + project = os.path.join(str(datafiles), 'invalid') + copy_subprojects(project, datafiles, ['base']) + + result = cli.run(project=project, args=['build', 'junctiondep-not-a-junction.bst']) + result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.INVALID_DATA) + + # Assert that we have the expected provenance encoded into the error + assert "junctiondep-not-a-junction.bst [line 3 column 2]" in result.stderr + + @pytest.mark.datafiles(DATA_DIR) def test_options_default(cli, tmpdir, datafiles): project = os.path.join(str(datafiles), 'options-default') diff --git a/tests/format/junctions/invalid/junctiondep-not-a-junction.bst b/tests/format/junctions/invalid/junctiondep-not-a-junction.bst new file mode 100644 index 000000000..c59f1be25 --- /dev/null +++ b/tests/format/junctions/invalid/junctiondep-not-a-junction.bst @@ -0,0 +1,4 @@ +kind: stack +depends: +- junction: app.bst + filename: target.bst -- cgit v1.2.1