GNOME.org
 

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

Hi Sander,

On Wed, 2018-10-31 at 10:45 +0000, Sander Striker wrote:

Hi,


On Tue, Oct 30, 2018 at 1:33 PM Tristan Van Berkom via BuildStream-list <buildstream-list gnome org> 
wrote:
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.


While it is orthogonal, we are not talking about this in the
abstract.  On the contrary, proposed solutions imply separate
elements for testing.  I am also not sure this applies the same to
packaging.  I can see packaging be more limited to leaf nodes, for
example final applications, whereas testing would apply to every
element, including for example every library dependency.
In short, let's treat this as orthogonal but let's also not forget
the importance of this problem.


Indeed it is an important problem.


From my perspective; it has occurred several times now that proposals

involve adding complexity to the base Element class, which has a high
cost and also tends to impede on the flexibility of what one can
express in pipelines of many elements.

I see these proposals rooted in an aversion to having to maintain many
.bst files in a given project - which is of course very rational.

Without getting into specifics of a separate proposal, clearly we need
a more convenient way to express elements in YAML, such that more
complex pipeline/project configurations can be expressed more easily
and conveniently.




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.


That implies that app-pkg.bst will not actually proceed with building
until all tests of the dependent elements complete?


Yes it does.

Note that if parallelism and optimization of build planning is an issue
here, there is nothing stopping one from declaring an
`app-pkg-test.bst` stack element which has this requirement instead.

However, setting things up like this implies that you dont get the
desired behavior that:

  "If the dependencies dont all pass their tests, this package will
   never exist."


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.


This could get rather complicated.  


I wanted to add this to the specification here in order to avoid
complications, actually.

  * As soon as an element can impose that it wants conditions satisfied
    by dependencies, this can happen at any level of the pipeline, not
    only in leaf nodes.

  * By explicitly limiting these conditions to the regular dependency
    chain, we avoid blowing this up and ending up with surprising
    conclusions, or possibly unexpected circular dependency chains.


  * 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).


Thanks for resurfacing that part of the artifact command subgroup
thread in the context of this conversation.


I think it's important to keep this in mind, in order to avoid
optimizing the same problem away with multiple solutions :)


I do think there is another consideration missing.  Instead of
declaring in app-pkg.bst that I want all of my (transient)
dependencies to satisfy the 'tested' condition I want to be able to
run "bst build app.bst" declaring that I want all elements that are
built during my run to have the tested condition met at the end of my
run.  And otherwise consider my build a failure.  I don't think it's
satisfactory If the only way for me to do this is to "bst build app-
pkg.bst".


This is an interesting insight which we did not discuss at the
gathering.

I think that this could safely be implemented as an extension to this
plan (possibly as added user configuration and command line option, or
even the project.conf could recommend a preference here).


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.


While I don't see why it couldn't work, I do think there is a balance
we need to strike between keeping things generic (and simpler from an
implementation/maintainablity PoV in BuildStream), and making things
more specialized and convenient for users.

If we have project maintainers provide standardized build elements
(kinds), can those also take care of testing in one go, or do I now
need to create a -test.bst element that has a related kind?


You absolutely must create a separate element, however this separate
element should not have to be defined in a separate file, and it should
ultimately be convenient to express this other element (this ties back
into what Chandan is referring to as an orthogonal problem).

There are several aspects to this question I think:

  * Does a specific "kind" (plugin) exist for the purpose of running
    tests ?

    This is not covered in the proposal, I personally think just a
    "manual" element would be fine for this purpose.

    Previous iterations of the proposal discussed a possibility
    of having a specific test element which reads command lists
    off of the public data of it's dependencies and runs them.

    While that could be implemented orthogonally to this proposal,
    I think that if we allow multiple elements to be declared in the
    same `.bst` file more conveniently, we obviate the need for this.

  * Again in the context of discussing a separate proposal to allow
    pipelines to be expressed in YAML more conveniently, Jürg has been
    suggesting a new type of plugin which could be responsible for
    defining how multiple elements defined in the same file relate
    to eachother.

    From this angle, a plugin could be created which says that a
    .bst file containing multiple element declarations have an implicit
    relationship for the "tested" condition.

    Here is a vague idea of what this might look like in YAML for an
    element like lib.bst:

      =====================================
      kind: tested-condition
      --
      kind: autotools
      depends:
      - foo.bst
      - bar.bst
      sources:
      - kind: git
        url: https://foo.bar/baz.git
      ...
      --
      kind: manual
      build-commands:
      - make check
      =====================================

    In the above, we would have a .bst file which defines a "stream" of
    objects (YAML supports this with the `--` delimiter as illustrated
    above)

    Then the "tested-condition" plugin, would be responsible for
    defining the relationship of the objects declared in the given .bst
    file.

    In this example, the "tested-condition" plugin would simply say
    that second object in the stream depends on the first object in
    the stream, and that it satisfies the "tested" condition of the
    first object.

There could of course be other ways to make this convenient to
express... but I think we need to shift the attention towards making
project expression more convenient, and away from piling on more
capability into individual elements.

Cheers,
    -Tristan


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.


https://xkcd.com/910/
 

Thanks again Chandan for the thorough writeup !


+1!
 

Cheers,
     -Tristan


Cheers,

Sander
 

_______________________________________________
BuildStream-list mailing list
BuildStream-list gnome org
https://mail.gnome.org/mailman/listinfo/buildstream-list



-- 

Cheers,

Sander
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]