Re: [BuildStream] Guidance on unit tests

[Angelos Evripiotis <angelos evripiotis gmail com>]

Hi again!

Here's the MR thread copied in, with replies.

Taken from:
https://gitlab.com/BuildStream/buildstream/merge_requests/973#note_120329379

> tristanvb wrote:
>
> > If I understand correctly, I think you would say for these arbitrary examples, unit tests are forbidden 
> > and we must exercise via the command-line in an end-to-end fashion exclusively:
> >
> > - A function to parse a string provided by a user, e.g. a URL.
>
> This is probably way overkill for what we should allow in tests, yeah; we're also probably mostly 
> interested in this case how the `CLI` responds to the user when given invalid URLs.

I'm surprised, but I think I can agree in some circumstances.

If the function helps with only a particular cli endpoint, we need to
thoroughly test that endpoint anyway. There seems to be minor value in
duplicating the test at the function level as well.

I wonder if we could agree that this variation would be worthy of unit testing:

- A function as fundamental as upper() or lower(), used throughout the
code. Perhaps something like an internationalized sentence break
iterator.

> > - A function to determine something about a graph.
>
> If it exists in the codebase, it exists for a reason; there is not much I can see in terms of excuses for 
> being too lazy to write end to end tests for it, if the reason for choosing a unit test is a matter of 
> "ease", that is definitely a bad excuse.

Again, I think I can agree if it's specialized to some particular cli
endpoint - we need to test the endpoint anyway.

I wonder if we could agree that this variation would be worthy of unit testing:

- A DAG data structure and functions to operate on it.

> > - A context manager to manage a temporary git clone.
>
> Why not properly find ways to actually create test scenarios where things are likely to fail, and ensure it 
> behaves properly... I think the unit test approach to these things is going to be even less recommendable, 
> i.e. things like `unittest.mock()` should just be avoided, we can setup a scenario for things to actually 
> fail instead.

I don't understand what you mean here, perhaps the example was
overloaded. What about this one:

- A context manager to manage a temporary directory (in some useful
way tempfile doesn't already), something we are (hypothetically) using
in many places in the project.

> > - A binary tree implementation.
>
> In this case I think we are talking about a low level rivet again, which probably has a well defined API 
> surface and we probably _do_ want assurances around this.

Ace.

> > - A helper class on top of heapq.
>
> This looks like another well defined rivet I _think_, although it also makes me suspicious as to what would 
> justify a "helper wrapper class" of something, that is always an interesting decision to make (I've found 
> myself regretting wrapping up `libXML` in `C` code, which was a bad mistake because at first I felt it was 
> "too hard" to use directly, eventually I found out it was important to instead just bare the burden of 
> actually understanding the thing wherever it was used).

I think I take your point; here I'm thinking more about a convenience
wrapper to reduce ceremony than hiding complexity in a leaky way. Does
that sound good?

> > - A function to implement some special-case sort algorithm.
>
> Again, this is probably an `Element` function or such ? it plays a role in sorting the order of the build 
> queue ? Definitely use an end to end test and observe the output of `bst show`, I don't see why a unit test 
> is necessary here.

I could agree if we were talking about a one-off 'key' function that
is a parameter to the sort algorithm. There shouldn't be a large test
space for that, and again we want to make sure the cil endpoint works.

I wonder if we could agree that this variation would be worthy of unit testing:

- An implmentation of quicksort, which for some hypothetical reason we
need to use from YAML and CasCache.

> > My understanding is that you would not accept end-to-end tests that make sure the above things are hooked 
> > up correctly, in combination with unit tests to comprehensively ensure they behave as they say they do. 
> > Is that correct?
>
> Here is an example of a test I let slip in just because I didnt have patience to explain why I think the 
> burden of actually having the test in the codebase far outweighs the usefulness of the test: [the test for 
> our custom handling of choices when prompting the user what to do when a build 
> fails](https://gitlab.com/BuildStream/buildstream/blob/master/tests/frontend/main.py)
>
> This cannot be properly tested because we currently dont (yet) have support for testing the interactive 
> side of things... but I would _love_ to remove that test, and ensure we don't ever pile most stuff like 
> that into our test battery.

Interesting! I'll think about that example some more.

Thanks for your time on this, I think once we have more certainty in
contributing.rst it's going to be a great help to productivity.

Cheers,
Angelos