diff options
author | bst-marge-bot <marge-bot@buildstream.build> | 2019-10-10 12:45:39 +0000 |
---|---|---|
committer | bst-marge-bot <marge-bot@buildstream.build> | 2019-10-10 12:45:39 +0000 |
commit | 6c6c581162ed7d87bad680330610e29d828d0f25 (patch) | |
tree | dc44ea6bba209746b1fdbaa92a4ee8714a3eb2d9 | |
parent | 28803a91bb0525a197de004dce5fbe750a00fbde (diff) | |
parent | bcbd1803151e24f411a11a788441c2da2686bf7a (diff) | |
download | buildstream-6c6c581162ed7d87bad680330610e29d828d0f25.tar.gz |
Merge branch 'tlater/progress-tests' into 'master'
Improve assertions around element loading progress reporting
Closes #1094
See merge request BuildStream/buildstream!1608
-rw-r--r-- | src/buildstream/_loader/loader.py | 42 | ||||
-rw-r--r-- | src/buildstream/_project.py | 2 | ||||
-rw-r--r-- | tests/format/junctions.py | 2 | ||||
-rw-r--r-- | tests/frontend/progress.py | 139 | ||||
-rw-r--r-- | tests/frontend/project/files/sub-project/files/deps.bst | 1 | ||||
-rw-r--r-- | tests/frontend/project/files/sub2-project/elements/import-sub.bst | 4 | ||||
-rw-r--r-- | tests/frontend/project/files/sub2-project/files/etc-files/etc/animal.conf | 1 | ||||
-rw-r--r-- | tests/frontend/project/files/sub2-project/project.conf | 4 | ||||
-rw-r--r-- | tests/frontend/show.py | 44 | ||||
-rw-r--r-- | tests/internals/loader.py | 15 | ||||
-rw-r--r-- | tests/testutils/context.py | 9 |
11 files changed, 204 insertions, 59 deletions
diff --git a/src/buildstream/_loader/loader.py b/src/buildstream/_loader/loader.py index d578787a3..d6f8ca861 100644 --- a/src/buildstream/_loader/loader.py +++ b/src/buildstream/_loader/loader.py @@ -37,6 +37,11 @@ from ..types import CoreWarnings from .._message import Message, MessageType +# This should be used to deliberately disable progress reporting when +# collecting an element +_NO_PROGRESS = object() + + # Loader(): # # The Loader class does the heavy lifting of parsing target @@ -94,7 +99,7 @@ class Loader(): # Raises: LoadError # # Returns: The toplevel LoadElement - def load(self, targets, rewritable=False, ticker=None, task=None): + def load(self, targets, task, rewritable=False, ticker=None): for filename in targets: if os.path.isabs(filename): @@ -148,7 +153,7 @@ class Loader(): self._clean_caches() # Cache how many Elements have just been loaded - if task: + if task is not _NO_PROGRESS: # Workaround for task potentially being None (because no State object) self.loaded = task.current_progress @@ -483,7 +488,7 @@ class Loader(): # Cache it now, make sure it's already there before recursing self._meta_elements[element.name] = meta_element - if task: + if task is not _NO_PROGRESS: task.add_current_progress() return meta_element @@ -591,14 +596,39 @@ class Loader(): return None # meta junction element - # XXX: This is a likely point for progress reporting to end up - # missing some elements, but it currently doesn't appear to be the case. - meta_element = self._collect_element(self._elements[filename], None) + # + # Note that junction elements are not allowed to have + # dependencies, so disabling progress reporting here should + # have no adverse effects - the junction element itself cannot + # be depended on, so it would be confusing for its load to + # show up in logs. + # + # Any task counting *inside* the junction will be handled by + # its loader. + meta_element = self._collect_element_no_deps(self._elements[filename], _NO_PROGRESS) if meta_element.kind != 'junction': raise LoadError("{}{}: Expected junction but element kind is {}" .format(provenance_str, filename, meta_element.kind), LoadErrorReason.INVALID_DATA) + # We check that junctions have no dependencies a little + # early. This is cheating, since we don't technically know + # that junctions aren't allowed to have dependencies. + # + # However, this makes progress reporting more intuitive + # because we don't need to load dependencies of an element + # that shouldn't have any, and therefore don't need to + # duplicate the load count for elements that shouldn't be. + # + # We also fail slightly earlier (since we don't need to go + # through the entire loading process), which is nice UX. It + # would be nice if this could be done for *all* element types, + # but since we haven't loaded those yet that's impossible. + if self._elements[filename].dependencies: + raise LoadError( + "Dependencies are forbidden for 'junction' elements", + LoadErrorReason.INVALID_JUNCTION) + element = Element._new_from_meta(meta_element) element._update_state() diff --git a/src/buildstream/_project.py b/src/buildstream/_project.py index 6ae8aa9db..7ba93bba4 100644 --- a/src/buildstream/_project.py +++ b/src/buildstream/_project.py @@ -433,7 +433,7 @@ class Project(): # def load_elements(self, targets, *, rewritable=False): with self._context.messenger.simple_task("Loading elements", silent_nested=True) as task: - meta_elements = self.loader.load(targets, rewritable=rewritable, ticker=None, task=task) + meta_elements = self.loader.load(targets, task, rewritable=rewritable, ticker=None) with self._context.messenger.simple_task("Resolving elements") as task: if task: diff --git a/tests/format/junctions.py b/tests/format/junctions.py index b810c55a3..a0af521a2 100644 --- a/tests/format/junctions.py +++ b/tests/format/junctions.py @@ -238,7 +238,7 @@ def test_invalid_with_deps(cli, datafiles): copy_subprojects(project, datafiles, ['base']) result = cli.run(project=project, args=['build', 'junction-with-deps.bst']) - result.assert_main_error(ErrorDomain.ELEMENT, 'element-forbidden-depends') + result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.INVALID_JUNCTION) # Test that we error correctly when a junction is directly depended on diff --git a/tests/frontend/progress.py b/tests/frontend/progress.py new file mode 100644 index 000000000..e3b127f3b --- /dev/null +++ b/tests/frontend/progress.py @@ -0,0 +1,139 @@ +# Pylint doesn't play well with fixtures and dependency injection from pytest +# pylint: disable=redefined-outer-name + +import os +import pytest + +from buildstream.testing import cli # pylint: disable=unused-import +from buildstream import _yaml +from buildstream._exceptions import ErrorDomain, LoadErrorReason + +from tests.testutils import generate_junction + +# Project directory +DATA_DIR = os.path.join(os.path.dirname(os.path.realpath(__file__)), ) + + +@pytest.mark.datafiles(os.path.join(DATA_DIR, 'project')) +def test_show_progress_tally(cli, datafiles): + # Check that the progress reporting messages give correct tallies + project = str(datafiles) + result = cli.run(project=project, args=['show', 'compose-all.bst']) + result.assert_success() + assert " 3 subtasks processed" in result.stderr + assert "3 of 3 subtasks processed" in result.stderr + + +@pytest.mark.datafiles(os.path.join(DATA_DIR, 'project')) +def test_junction_tally(cli, tmpdir, datafiles): + # Check that the progress reporting messages count elements in junctions + project = str(datafiles) + subproject_path = os.path.join(project, 'files', 'sub-project') + junction_path = os.path.join(project, 'elements', 'junction.bst') + element_path = os.path.join(project, 'elements', 'junction-dep.bst') + + # Create a repo to hold the subproject and generate a junction element for it + generate_junction(tmpdir, subproject_path, junction_path, store_ref=True) + + # Create a stack element to depend on a cross junction element + # + element = { + 'kind': 'stack', + 'depends': [{ + 'junction': 'junction.bst', + 'filename': 'import-etc.bst' + }] + } + _yaml.roundtrip_dump(element, element_path) + + result = cli.run(project=project, + silent=True, + args=['source', 'fetch', 'junction.bst']) + result.assert_success() + + # Assert the correct progress tallies are in the logging + result = cli.run(project=project, args=['show', 'junction-dep.bst']) + assert " 2 subtasks processed" in result.stderr + assert "2 of 2 subtasks processed" in result.stderr + + +@pytest.mark.datafiles(os.path.join(DATA_DIR, 'project')) +def test_nested_junction_tally(cli, tmpdir, datafiles): + # Check that the progress reporting messages count elements in + # junctions of junctions + project = str(datafiles) + sub1_path = os.path.join(project, 'files', 'sub-project') + sub2_path = os.path.join(project, 'files', 'sub2-project') + # A junction element which pulls sub1 into sub2 + sub1_element = os.path.join(project, 'files', 'sub2-project', 'elements', 'sub-junction.bst') + # A junction element which pulls sub2 into the main project + sub2_element = os.path.join(project, 'elements', 'junction.bst') + element_path = os.path.join(project, 'elements', 'junction-dep.bst') + + generate_junction(tmpdir / "sub-project", sub1_path, sub1_element, store_ref=True) + generate_junction(tmpdir / "sub2-project", sub2_path, sub2_element, store_ref=True) + + # Create a stack element to depend on a cross junction element + # + element = { + 'kind': 'stack', + 'depends': [{ + 'junction': 'junction.bst', + 'filename': 'import-sub.bst' + }] + } + _yaml.roundtrip_dump(element, element_path) + + result = cli.run(project=project, + silent=True, + args=['source', 'fetch', 'junction.bst']) + result.assert_success() + + # Assert the correct progress tallies are in the logging + result = cli.run(project=project, args=['show', 'junction-dep.bst']) + assert " 3 subtasks processed" in result.stderr + assert "3 of 3 subtasks processed" in result.stderr + + +@pytest.mark.datafiles(os.path.join(DATA_DIR, 'project')) +def test_junction_dep_tally(cli, tmpdir, datafiles): + # Check that the progress reporting messages count elements in junctions + project = str(datafiles) + subproject_path = os.path.join(project, 'files', 'sub-project') + junction_path = os.path.join(project, 'elements', 'junction.bst') + element_path = os.path.join(project, 'elements', 'junction-dep.bst') + + # Create a repo to hold the subproject and generate a junction element for it + generate_junction(tmpdir, subproject_path, junction_path, store_ref=True) + + # Add dependencies to the junction (not allowed, but let's do it + # anyway) + with open(junction_path, 'a') as f: + deps = { + 'depends': [ + 'manual.bst' + ] + } + _yaml.roundtrip_dump(deps, f) + + # Create a stack element to depend on a cross junction element + # + element = { + 'kind': 'stack', + 'depends': [{ + 'junction': 'junction.bst', + 'filename': 'import-etc.bst' + }] + } + _yaml.roundtrip_dump(element, element_path) + + result = cli.run(project=project, + silent=True, + args=['source', 'fetch', 'junction-dep.bst']) + + # Since we aren't allowed to specify any dependencies on a + # junction, we should fail + result.assert_main_error(ErrorDomain.LOAD, LoadErrorReason.INVALID_JUNCTION) + + # We don't get a final tally in this case + assert "subtasks processed" not in result.stderr diff --git a/tests/frontend/project/files/sub-project/files/deps.bst b/tests/frontend/project/files/sub-project/files/deps.bst new file mode 100644 index 000000000..abd99305d --- /dev/null +++ b/tests/frontend/project/files/sub-project/files/deps.bst @@ -0,0 +1 @@ +- import-etc.bst diff --git a/tests/frontend/project/files/sub2-project/elements/import-sub.bst b/tests/frontend/project/files/sub2-project/elements/import-sub.bst new file mode 100644 index 000000000..ded8fcb23 --- /dev/null +++ b/tests/frontend/project/files/sub2-project/elements/import-sub.bst @@ -0,0 +1,4 @@ +kind: stack +depends: + - junction: 'sub-junction.bst' + filename: 'import-etc.bst' diff --git a/tests/frontend/project/files/sub2-project/files/etc-files/etc/animal.conf b/tests/frontend/project/files/sub2-project/files/etc-files/etc/animal.conf new file mode 100644 index 000000000..db8c36cba --- /dev/null +++ b/tests/frontend/project/files/sub2-project/files/etc-files/etc/animal.conf @@ -0,0 +1 @@ +animal=Pony diff --git a/tests/frontend/project/files/sub2-project/project.conf b/tests/frontend/project/files/sub2-project/project.conf new file mode 100644 index 000000000..bbb8414a3 --- /dev/null +++ b/tests/frontend/project/files/sub2-project/project.conf @@ -0,0 +1,4 @@ +# Project config for frontend build test +name: subtest + +element-path: elements diff --git a/tests/frontend/show.py b/tests/frontend/show.py index 0d444a925..bc51d2967 100644 --- a/tests/frontend/show.py +++ b/tests/frontend/show.py @@ -40,16 +40,6 @@ def test_show(cli, datafiles, target, fmt, expected): .format(expected, result.output)) -@pytest.mark.datafiles(os.path.join(DATA_DIR, 'project')) -def test_show_progress_tally(cli, datafiles): - # Check that the progress reporting messages give correct tallies - project = str(datafiles) - result = cli.run(project=project, args=['show', 'compose-all.bst']) - result.assert_success() - assert " 3 subtasks processed" in result.stderr - assert "3 of 3 subtasks processed" in result.stderr - - @pytest.mark.datafiles(os.path.join( os.path.dirname(os.path.realpath(__file__)), "invalid_element_path", @@ -397,40 +387,6 @@ def test_fetched_junction(cli, tmpdir, datafiles, element_name, workspaced): assert 'junction.bst:import-etc.bst-buildable' in results -@pytest.mark.datafiles(os.path.join(DATA_DIR, 'project')) -def test_junction_tally(cli, tmpdir, datafiles): - # Check that the progress reporting messages count elements in junctions - project = str(datafiles) - subproject_path = os.path.join(project, 'files', 'sub-project') - junction_path = os.path.join(project, 'elements', 'junction.bst') - element_path = os.path.join(project, 'elements', 'junction-dep.bst') - - # Create a repo to hold the subproject and generate a junction element for it - generate_junction(tmpdir, subproject_path, junction_path, store_ref=True) - - # Create a stack element to depend on a cross junction element - # - element = { - 'kind': 'stack', - 'depends': [ - { - 'junction': 'junction.bst', - 'filename': 'import-etc.bst' - } - ] - } - _yaml.roundtrip_dump(element, element_path) - - result = cli.run(project=project, silent=True, args=[ - 'source', 'fetch', 'junction.bst']) - result.assert_success() - - # Assert the correct progress tallies are in the logging - result = cli.run(project=project, args=['show', 'junction-dep.bst']) - assert " 2 subtasks processed" in result.stderr - assert "2 of 2 subtasks processed" in result.stderr - - ############################################################### # Testing recursion depth # ############################################################### diff --git a/tests/internals/loader.py b/tests/internals/loader.py index 9af2bf161..39ef8ac99 100644 --- a/tests/internals/loader.py +++ b/tests/internals/loader.py @@ -5,6 +5,7 @@ import pytest from buildstream._exceptions import LoadError, LoadErrorReason from buildstream._project import Project from buildstream._loader import MetaElement +from buildstream._loader.loader import _NO_PROGRESS from tests.testutils import dummy_context @@ -30,7 +31,7 @@ def test_one_file(datafiles): basedir = str(datafiles) with make_loader(basedir) as loader: - element = loader.load(['elements/onefile.bst'])[0] + element = loader.load(['elements/onefile.bst'], _NO_PROGRESS)[0] assert isinstance(element, MetaElement) assert element.kind == 'pony' @@ -41,7 +42,7 @@ def test_missing_file(datafiles): basedir = str(datafiles) with make_loader(basedir) as loader, pytest.raises(LoadError) as exc: - loader.load(['elements/missing.bst']) + loader.load(['elements/missing.bst'], _NO_PROGRESS) assert exc.value.reason == LoadErrorReason.MISSING_FILE @@ -51,7 +52,7 @@ def test_invalid_reference(datafiles): basedir = str(datafiles) with make_loader(basedir) as loader, pytest.raises(LoadError) as exc: - loader.load(['elements/badreference.bst']) + loader.load(['elements/badreference.bst'], _NO_PROGRESS) assert exc.value.reason == LoadErrorReason.INVALID_YAML @@ -61,7 +62,7 @@ def test_invalid_yaml(datafiles): basedir = str(datafiles) with make_loader(basedir) as loader, pytest.raises(LoadError) as exc: - loader.load(['elements/badfile.bst']) + loader.load(['elements/badfile.bst'], _NO_PROGRESS) assert exc.value.reason == LoadErrorReason.INVALID_YAML @@ -73,7 +74,7 @@ def test_fail_fullpath_target(datafiles): fullpath = os.path.join(basedir, 'elements', 'onefile.bst') with make_loader(basedir) as loader, pytest.raises(LoadError) as exc: - loader.load([fullpath]) + loader.load([fullpath], _NO_PROGRESS) assert exc.value.reason == LoadErrorReason.INVALID_DATA @@ -83,7 +84,7 @@ def test_invalid_key(datafiles): basedir = str(datafiles) with make_loader(basedir) as loader, pytest.raises(LoadError) as exc: - loader.load(['elements/invalidkey.bst']) + loader.load(['elements/invalidkey.bst'], _NO_PROGRESS) assert exc.value.reason == LoadErrorReason.INVALID_DATA @@ -93,6 +94,6 @@ def test_invalid_directory_load(datafiles): basedir = str(datafiles) with make_loader(basedir) as loader, pytest.raises(LoadError) as exc: - loader.load(['elements/']) + loader.load(['elements/'], _NO_PROGRESS) assert exc.value.reason == LoadErrorReason.LOADING_DIRECTORY diff --git a/tests/testutils/context.py b/tests/testutils/context.py index 899bad247..849895e92 100644 --- a/tests/testutils/context.py +++ b/tests/testutils/context.py @@ -15,10 +15,13 @@ # License along with this library. If not, see <http://www.gnu.org/licenses/>. import os +from unittest.mock import MagicMock +from types import MethodType from contextlib import contextmanager from buildstream._context import Context +from buildstream._state import _Task # Handle messages from the pipeline @@ -26,6 +29,11 @@ def _dummy_message_handler(message, is_silenced): pass +@contextmanager +def _get_dummy_task(self, activity_name, *, element_name=None, full_name=None, silent_nested=False): + yield MagicMock(spec=_Task("state", activity_name, full_name, 0)) + + # dummy_context() # # Context manager to create minimal context for tests. @@ -42,5 +50,6 @@ def dummy_context(*, config=None): context.load(config=config) context.messenger.set_message_handler(_dummy_message_handler) + context.messenger.simple_task = MethodType(_get_dummy_task, context.messenger) yield context |