Code review comment for ~cjwatson/launchpad:ci-build-multi-arch-ui

Revision history for this message
Jürgen Gmach (jugmac00) wrote :

Thanks for keeping this discussion going. Having tests "trivially correct upon inspection" is one of several building blocks which I would love to see for Launchpad to be incorporated, to make maintaining such a huge project, which was written and is and will be maintained by many generations of developers over decades more sustainable.

I see your point. I have been bitten by hardcoded test values myself. On the other hand, I have seen tests where both the input and the output were generated, and although both were wrong, the tests passed. You'd need to step through the tests in order to prove they do what they claim.

The issue with a hardcoded value can be mitigated by testing several values. In this special case we could use a couple of series and architectures. This is trivial with pytest's parametrization feature, but should also be also possible with our setup, either with subtests https://blog.ganssle.io/articles/2020/04/subtests-in-python.html or with asserting in a loop.

In case of "using several input values to mitigate the fact of one hardcoded value could mislead us" is not enough of a compromise, I also thought about adding comments.

e.g.
```
text=re.compile(r".*/.*"),
```
turns into
```
text=re.compile(r".*/.*"), # e.g. "focal/arm64"
```

This helps with understanding the test, but unfortunately does not help with "trivially correct upon inspection". And it brings all the problems along of diverging code and comments.

And as you asked me about where exactly to improve expressiveness - right after the "text=..." - there should be a string, e.g. `text=focal/arm64`, just as above in your commit message.

I have incorporated this kind of writing tests many years ago, and I strongly believe this is the way to go. As you mentioned, we had the discussion before and we agree to disagree. I respect that, and that is why I have approved the MP.

We should also keep two things in mind. Whenever possible we should write code in a way that it can be easily understood by all team members, not just by oneself. And as senior engineers, it is not just about a personal preference, every decision has a bigger impact.

By the way, the quote from above, i.e "trivially correct upon inspection", is from a book I just recently started reading: "Software Engineering at Google" - in which a couple of google engineers discuss what they have learned from maintaining massive code bases for decades. It is not like we need take everything written as gospel truth, but I have to admit it is kinda nice to provide a link to a highly regarded book to support one's standpoint:
https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests

« Back to merge proposal