Re: [BuildStream] Guidance on unit tests

[Angelos Evripiotis <angelos evripiotis gmail com>]

Hiya Tristan, Jim,

My aim for this thread and the associated MR is still to clarify the
current situation for contributors like me, rather than propose
changes.

My idea is to be able to commit to workable common ground quickly, and
then continue from there.

For that reason I’m trying to keep my opinions out of it. That being
said, I won’t put forward something I can’t sign up to, so some may
still leak in :o)

If you disagree with what’s there, then it’s hopefully because I
haven’t understood the status quo.

On Tue, 20 Nov 2018 at 10:33, Tristan Van Berkom via BuildStream-list
<buildstream-list gnome org> wrote:
> Hi Jim, Angelos,
>
> On Mon, 2018-11-19 at 11:13 +0000, Jim MacArthur via BuildStream-list wrote:
> > On 16/11/2018 10:48, Tristan Van Berkom via BuildStream-list wrote:
> [...]
> > > In summary, I feel that saying that end-to-end testing is "preferred"
> > > is not strongly worded enough, I would rather the statement discourage
> > > unit testing more strongly, and make it explicit that unit testing is
> > > to be avoided as much as at all possible - and that we should strive to
> > > convert any existing unit tests to end to end tests at all times.
> > >
> > > Further than this, one should ask themselves the question: If it is not
> > > possible to exercise this code path or branch statement by running
> > > BuildStream in any way; then what is the justification for this line of
> > > code to exist ?
> > >
> > > If a line of code can be proven to not be testable in an end to end
> > > test, it does not really mean that it should be unit tested, it rather
> > > means that the line of code has been proven to be dead code and code
> > > should rather be restructured to exclude the line.
> >
> > I agree with all the above, particularly testing at the function level
> > which I think is too low-level. However, I think it misses one value of
> > unit tests, that they provide a direct indication of what has broken.
> > You should be able to exercise all code paths via the user interface,
> > just as one can test the strength of rivets by sailing dozens of ocean
> > liners, but providing coverage isn't all of testing. Unit testing done
> > right (it isn't always) will tell you more precisely what is broken than
> > an end-to-end test.
> >
> > Some of this could be improved by adding more assertions and proving
> > more detailed exceptions in the code instead, but these can't capture
> > the necessary context for a test in all cases.
>
> You make a very good analogy and I agree with your approach.
>
> I think that we could do well to have tests for internal surfaces which
> make sense to test and we want to have a wall of defense for, which is
> why I like having the YAML level tests in place.
>
> I think another good example of an internal API surface worthy of it's
> own test battery is the CasCache, I would be very supportive of
> additional tests which test internal API surfaces, as it helps to keep
> the architecture sane to have well defined behavioral expectations of
> internal components like this (it could turn out that your ocean liners
> are sailing properly *because* of a malfunctioning rivet, this is also
> a very undesirable situation).

This all gives me some good material for my next attempt, thanks!

> That said, I don't think that the changes made to the contributing
> guide reflect this approach and strategy at all; we should not be less
> strict about the end to end testing due to the existence of unit
> testing of internal API surfaces, and we should not really shy away
> from writing tests which are backed by internal APIs, we should rather
> strive for testing of internal API surfaces which need to have well
> defined API contracts for the higher level codebase to rely on, without
> omitting tests which observe how these behaviors impact and define the
> outward user experience.

This sounds good. I’m not sure, but I think you’re saying this is
missing from the current text, rather than it contradicts it.

> On Fri, 2018-11-16 at 17:39 +0000, Angelos Evripiotis wrote:
> [...]
> > I fear the change may have landed prematurely then. Are you good with
> > it as-is, on the understanding that these enhancements will also make
> > their way in separately?
>
> Don't get me wrong, I am not frustrated to the point of reverting.
>
> I do fear we are sending the wrong message with this and hope to reach
> consensus and rectify that as soon as possible.

Oh dear! Frustration doesn’t sound like I’m making friends and earning
trust with it. I’d prefer it were reverted if it’s going to annoy you
until it's done.

> Do you agree with Jim and myself on the above conversation ?

I happily agree with the pro-testing-in-general bits, I'm glad to work
on such a project that is clearly shaped from experience.

Not the anti-function-testing bit, I can't propose something that
rules them out. I'll avoid that for now and see if we can still move
forward.

> As you wrote this last patch to the guide, I do hope that you will
> offer up an alternative.

Of course! I think there's enough said for me to start another MR,
I'll post a link back here when it's ready.

> Cheers,
>     -Tristan

Cheers,
Angelos