Re: [BuildStream] Proposal: Add support for running tests in BuildStream

[Tristan Van Berkom <tristan vanberkom codethink co uk>]

Hi Chandan,

This is a beautiful writeup and I feel it accurately describes what we
discussed in the gathering.

That said I am a bit biased by having been present for the discussion;
it is quite fairly possible that someone who was not present at the
discussion would have a hard time to understand this writeup :)

I will add some things to this inline to your email...

On Thu, 2018-10-25 at 14:56 +0100, Chandan Singh wrote:
> Hi all,
>
> After several long discussions in this thread, another thread [0], and in
> person at the recent BuildStream Gathering [1], I think we have finally reached
> some consensus on how testing should work in BuildStream. In this message, I
> aim to summarize what we discussed in person. Unless anyone has any objections
> with the high-level plan, I will summarize the implementation plan in a
> following message.
>
> Problem
> =======
>
> Let's start by doing a recap of the problem we are trying to solve.
>
> As software integrators, we would like to run tests as part of BuildStream
> builds and be able to validate (at a later stage in build) that the tests for a
> given element have passed. Ideally, such tests should have the following
> properties:
>
>   1.) Tests should not block reverse dependency builds.
>   2.) Strong association between an element and its tests.
>   3.) Additional dependencies for testing an element should not be required
>       while building it.
>   4.) Elements should be able to assert that the tests for their dependencies
>       have passed.
>   5.) Test elements are not necessarily pass-through elements - they can have
>       meaningful output such as coverage reports.

  6.) Tests are allowed to carry additional dependencies which are not
      necessarily needed at build or run time.

> Note that having tests as separate elements can result in essentially doubling
> the number of elements in a given project - this problem is orthogonal to
> testing and also applies to things like packaging. So, let us acknowledge that
> is important and deal with it separately.
>
> Proposed solution
> =================
>
> To ensure that all of us are talking about the same thing, let us consider the
> following dependency graph as an example.
>
>       [base.bst]
>           |
>           |
>           v
>       [lib.bst] --> [lib-test.bst]
>           |
>           |
>           v
>       [app.bst] --> [app-test.bst]
>           |
>           |
>           v
>     [app-pkg.bst]
>
> This is a simple project with a single app and a single library, where the
> final output is a package of the app. Other than the standard elements for the
> base, lib and app; we also have a few additional elements:
>
>   a.) lib-test.bst - a standard BuildStream element that will run tests for
>       lib.bst as part of its build.
>   b.) app-test.bst - same as lib-test.bst but will run tests for app.bst.
>   c.) app-pkg.bst - a standard BuildStream element that will produce a package
>       for the app, depends only on app.bst.
>
> If this project is used as-is, there will be no way for app-pkg.bst to verify
> that the tests have actually passed. Moreover, since lib-test.bst is not listed
> as a dependency for anything in the plan for app-pkg.bst, it will not even get
> built. To remedy that, we need to introduce the notion of "conditions".
>
> XXX: Maybe we need a better word for "conditions" - suggestions welcome :)
>
> Conditions
> ----------
>
> Conditions will essentially be a mapping from a keyword to a set of elements.
> The keyword can be anything as far as BuildStream is concerned but the projects
> are free to enforce their own guidelines. The set of elements will usually be a
> subset of the reverse dependencies of the given element.
>
> Conditions can be things like: "tested", "packaged", "license verified" etc.
>
> When listing dependencies, an element can require certain conditions to be
> true for its dependencies. Doing so will implicitly add a dependency on the
> corresponding set of elements.
>
> Coming back to our example, lib.bst can declare its "tested" condition to be
> the set of elements: [lib-test.bst]. Then, app-pkg.bst can require the "tested"
> condition to be met for its dependencies. So, although it only explicitly
> depends on app.bst, BuildStream would automatically add a dependency on
> lib-test.bst and app-test.bst when building it.
>
> It is also worth noting that since base.bst does not have a corresponding test
> element and does not declare a "tested" condition, this condition does not
> apply to it.
>
> While this example only mentions the "tested" condition, it should be easy to
> see how this might work with other conditions as well.

I'd like to illustrate a couple of interesting properties here,
starting with your graph above.

Consider the build graph below as Chandan described it, and consider
that lib-test.bst satisfies the "tested" condition of lib.bst.

Similarly app-test.bst satisfies the "tested" condition of app.bst.

                     [base.bst]
                         |
                         |
                         v
  [lib-test.bst] <-- [lib.bst]
                         |
                         |
                         v
                     [app.bst] --> [app-test.bst]
                         |
                         |
                         v
                   [app-pkg.bst]


Add to this, that app-pkg.bst makes the following statement:

  I require that all of my dependencies have the "tested" condition
  satisfied

Then the above dependency graph is transformed in the following way:

                     [base.bst]
                         |
        
                |
                         v
  [lib-test.bst] <--
[lib.bst]
         |               |
         |               |

        |               v
         |           [app.bst] --> [app-
test.bst]
         |               |               |
         |          
    |               |
         |               v               |
        
+--------> [app-pkg.bst] <------+


Some things to note about this automatic transformation:

  * The two newly grown dependency relationships are not regular
    build or runtime dependencies, consequently their outputs are not
    staged during the app-pkg.bst build.

  * The app-pkg.bst cache key *does* however take the cache keys
    of the new "condition" dependencies into account.

    This has the interesting property of blocking the creation of the
    resulting artifact until the tests have successfully run.

  * While the requirement of the "tested" condition imposed by
    app-pkg.bst is recursive in nature, inasmuch as it requires
    the "tested" condition recursively across it's explicitly
    declared dependencies, it is not recursive across implicit
    condition based boundaries.

    This is to say that:
    - app-pkg.bst requires the tested condition of app.bst AND lib.bst
    - app-pkg.bst does NOT require the tested condition of lib-test.bst

    This is because lib-test.bst was implicitly pulled into the graph
    due to the condition requirement.

    However, if lib-test.bst imposes other requirements on it's own
    dependencies, those will be treated in the same way as app-pkg.bst.

  * If an artifact for app-pkg.bst exists but the tests declared
    for lib-test.bst are changed, this will cause app-pkg.bst to have
    a new cache key, requiring the tests to be re-run.

    HOWEVER, following some plans which were discussed in artifact
    subcommand group proposal, around here (see earlier and later
    messages for better context):

      https://mail.gnome.org/archives/buildstream-list/2018-October/msg00006.html

    With "artifact aliasing" (also needs a better name), it will be
    possible to avoid the actual processing in rebuilding app-pkg.bst,
    since the actual staged inputs and build commands will remain
    unchanged.

    Ideally after implementing this orthogonal feature; the effect of
    re-building app-pkg.bst after having only changed a test, is that:

      - A new cache key is derived for app-pkg.bst
      - This new cache key requires the tests to pass for all deps,
        causing app-test.bst to be rebuilt
      - The actual package payload is not re-run, only a new artifact
        is created with the same files content as a previously existing
        artifact.

    Until this measure of build avoidance is implemented in a generic
    way, app-pkg.bst will be rebuilt every time the tests of it's
    dependencies are changed and require rebuilds (retests).

> Considerations
> ==============
>
> Using this approach, we can satisfy the criteria listed above. Additionally, it
> also provides the following advantages:
>
>   - BuildStream does not require any implicit knowledge about testing and
>     treats tests as regular elements.
>   - Since the idea of conditions is generic, it can also be used for other
>     purposes like "packaged", "license verified" etc.
>   - It is easily enforceable by maintainers of a BuildStream project, either
>     manually or automatically.
>
> At various points in the past, we have discussed other approaches. Here is a
> short (and incomplete) list of approaches that we considered but realized were
> not fit for purpose for some reason. I am listing them here in case anyone
> thinks that we missed some obvious choices:
>
>    - Running tests as part of the build - basically works but we lose the
>      potential parallelism and block the reverse dependency builds.
>    - Retroactively marking build as failed if tests fail: altering the artifact
>      after it has been published seems wrong, and failing a test should not
>      prevent the existence of the artifact for a successful build.
>    - Having leaf elements explicitly depend on the test elements - basically
>      works but not easy to enforce standards this way and it is easy to miss
>      tests for certain dependencies.
>    - Have additional cache keys for testing - requires BuildStream to be aware
>      that tests are special and also suffers from the same problem as having an
>      explicit dependency on test elements.
>
>
> I have purposefully tried to keep the implementation details out of this
> message, so that we can agree on the high-level plan first, and decide on the
> details later. I will be very curious to hear what others think about
> "conditions" :)

I'm also wary of the name here, let's consider what the YAML will look
like and what contexts in which the vocabulary will be used, using
"conditions", I think it will look like this:

======= lib.bst =======

# Specify that the lib-test.bst element provides its "tested" condition
condition-providers:
  tested:
  - lib-test.bst


======= app-pkg.bst =======

# Specify that this element requires the "tested" condition of its
# dependencies
condition-requirements:
- tested


I'm not sure I love "condition" as a word for this, it does make some
kind of sense when thinking about state chart terminology (e.g., the
built app-pkg.bst state cannot be reached until the conditions are all
satisfied), but clashes a bit with what we call "conditional
statements" in our YAML layer.

The word "state" is not much better.

Another approach would be to talk in terms of "extensions", as in the
lib.bst declares lib-test.bst as one of it's "tested extensions".

I'm not sure this is better either, and would also be interested to
hear other suggestions on the terminology to use.

Thanks again Chandan for the thorough writeup !

Cheers,
     -Tristan