Merge branch 'tlater/progress-tests' into 'master'

Improve assertions around element loading progress reporting

Closes #1094

See merge request BuildStream/buildstream!1608

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