Re: [BuildStream] Guidance on unit tests

[Angelos Evripiotis <angelos evripiotis gmail com>]

On Fri, Nov 16, 2018 at 10:48 AM Tristan Van Berkom
<tristan vanberkom codethink co uk> wrote:
> On Thu, 2018-11-15 at 18:28 +0000, Angelos Evripiotis via BuildStream-list wrote:
> > Hello list,
> >
> > I have opened an MR to add guidance on unit tests to the contributing guide:
> >
> > https://gitlab.com/BuildStream/buildstream/merge_requests/943
> > I’m posting it here too so that it gets a decent airing, it would be significant to all contributors.
> >
> > One aim of mine here is to decrease uncertainty around whether unit tests are welcome in the project.
> >
> > Another is to establish some common ground, before later proposing more pro-unit-test changes.
> >
> > Changes proposed in this merge request:
> >
> > o State that exercising things via the cli is to be preferred. Perhaps it should also say the public API.
> > o State that unit tests are good where they are robust to unrelated changes made to internals.
>
> I'd like to ring in here and state that I personally view unit testing
> as a last resort, or useful for low level internal API surfaces (like
> the YAML layer for which we have these) which we want to test
> independently.

I think we have common ground on the low level internal API surfaces bit.

In later work I'd like to provide evidence of the value expanded
unit-tests can bring to BuildStream, if you're open to changing your
mind on them being the last resort. I'm expecting to be able to find
and fix some juicy bugs :)

> There are a lot of problems surrounding unit testing of individual
> functions, the tests have a tendency of testing that the function does
> what it was written to do; which is subtly different from testing what
> the function *should* be doing, which is often times something that we
> will discover later on, once we realize that it was written to do
> something it shouldn't be doing... Having a test that passes is often
> an obstacle to the critical thinking that is needed when necessarily
> reconsidering what a function should be doing.

I have shared this pain too, sometimes folks write tests that just
ensure the function does what the function does. If someone else
realises the behaviour is wrong and fixes it, the test 'fails',
uselessly. This slows development down, as diligent folks take the
time to figure out why the test was asserting that way in the first
place.

I'd like to add something more substantial about what good looks like
with unit-testing, I'm thinking it's beyond the scope of this current
discussion though.

> I find that unit testing in the sense of writing tests before
> implementing functions was a great way for starting projects, e.g. when
> the whole end-to-end process didn't yet exist it was important; it was
> then also important to throw away all of that cruft and convert these
> to end to end tests.
>
> 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.

There's this bit in the text, which I think you agree with:

---
You should first aim to write tests that exercise your changes from the cli.
This is so that the testing is end-to-end, and the changes are guaranteed to
work for the end-user.
---

Perhaps we could add something to the effect of:
"This also gives us more confidence it's useful for the user, which is
the main aim of the project after all."

Does that seem satisfactory?

> 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 thoroughly agree with erasing dead code. In general I take the
mindset that code is a liability, rather than an asset.

I'm also prejudiced against optional complexity, even if it is in use.

The relevant bit of the text for this concern seems to be:
---
It may be impractical to sufficiently examine some changes this way
...
It may also be difficult to contrive circumstances to cover every line
of the change.
---

Maybe we could add something to the effect of:
"If it's actually impossible to cover every line of the change from
the cli, consider whether it's actually dead code, and should be
removed instead of tested."

Does that seem good?

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?

> Cheers,
>     -Tristan

Cheers!
Angelos