[Refactor] Slight changes to engine and executors tests to improve readability

[oleiade]

Hi folks,

As I was looking into #1986, I made some slight changes to some test files, mostly in lib/executor and core/engine_test.go. Those are really tiny improvements that I did to help me improve my understanding of the test code. As they might be helpful to more than just me, I thought I'd submit a PR for them.

Let me know if you find them useful 🙏🏻 If so, I'll probably extend it to the rest of the lib/executor package.

[codebien]

I'm against having the code with a lot of these comments, if it isn't clear where the arrange-act-assert state operations start and end then it probably means that the test needs to be refactored again.

[na--]

Me too, these comments don't really add anything besides noise for me 😕

[oleiade]

I wholeheartedly disagree with you folks, but I don't have a strong enough opinion about it to argue 😄

To expand on my view on this: tests are also documentation. They need to be easy to read/parse, structured, and make their intent and the behavior they outline explicit. I personally had a hard time reading through them and understanding them; as it required me quite a bit of overhead. One of the main reasons, I found, was that it was actually not clear to me what is set up code (arrange), what is actually being tested (act), and what is the expected output state (assert).
AAA testing is a technique I've been introduced to a while ago and that I've followed ever since, and in general goes farther than just having comments to outline the different sections of the tests. Another best practice on that front is to have want prefixed local variables to outline what the outcome state is expected to be, and got prefix variables to compare them to, outlining what output we actually got.

[na--]

Googling "AAA testing" leads me to

Abdominal aortic aneurysm (AAA) screening is a way of checking if there's a bulge or swelling in the aorta

😅 The second result is https://medium.com/@pjbgf/title-testing-code-ocd-and-the-aaa-pattern-df453975ab80 though - do you have a better introduction to the concept? Because I think that's written by someone who likes the concept, like you, but also has the same opinion as @codebien and me about the explicit comments:

Test code, in the same way as production code, is supposed to be clean and self-explanatory. Preempting what you are going to do just adds unnecessary clutter. You can still have a clear distinction of the three steps by using spaces and indentation. Which are ultimately the key features the compiler provides you to structure your code.
It does look silly having a comment preceding each paragraph in this text, it tires the reader, as it has to constantly go through the obvious (or skip it). So why would you do that with your code?

Couldn't have written it better myself 😅 👍 for refactoring tests to be cleaner and easier to read, ideally self-explanatory. The same as any other piece of code. 👎 for comments that should be obvious from the context.

[oleiade]

Sorry I used an acronym, I'm generally the one chasing people telling them not to do it, and here I go losing at my own game 🟥 ...

AAA stands for Arrange, Act, Assert. It's a very common pattern in software testing, specifically because it helps outline and make the natural flow of unit tests explicit: arrange some input, act upon them, assert the produced output.

I don't mean to play the "someone is wrong on the internet" card, but I found the article you point at quite judgmental, and left me disagreeing rather than convinced. Here a few counter examples I would like to point you too, from Microsoft, a Manning book about Unit testing, or a blog post I often end up stumbling upon when I need a refresher.

Here, I end up arguing about it, although I wanted to actually avoid that. All I can say is that I've been introduced to this technique years ago by a colleague. I was really skeptical at first, but I never went back once I started. I spend 90% reading code rather than writing it, and anything that makes my life "easier" (I acknowledge this means different things to different people 🙏🏻) as a reader is a gain from my perspective. It took me a lot of effort to make sense out of these tests, I found them (too) long, complicated, and it was unclear to me at first glance what was actually setting up code, actually "act" code, and assertion code. You might have a different experience, but that was mine 😇

Now I acknowledge everyone has a different experience, a different brain, and that's exactly why I'm not especially trying to convince you folks. What becomes apparent, though, is that I would like to have some document somewhere where we gather, discuss, and decide on best practices we follow on this repository to be able to quickly sort out these disagreements in the future 😄

[na--]

It took me a lot of effort to make sense out of these tests, I found them (too) long, complicated, and it was unclear to me at first glance what was actually setting up code, actually "act" code, and assertion code. You might have a different experience, but that was mine innocent

I don't disagree with this part, a lot of the tests can definitely be improved, substantially 😅 I don't even disagree that tests should be split into these 3 sections whenever possible, that's the natural flow for most unit tests. However, I disagree that we should add these // Arrange, // Act, // Assert comments in the tests. Instead, I think the tests should be refactored to be so clean and easy to read that we don't need such comments, i.e. in such a way that the sections are obvious without them 🙂

[codebien]

I think we could decrease this timeout, probably for getting a feedback faster in case we would hit the case

[na--]

why? the timeout is here just in case, so we don't hit the global 120s one 🤷‍♂️

[codebien]

assert.NoError(t, run())

we often use this form in the codebase, you think that is it not readable?

[na--]

For me, the single line variant is much more readable... It's the test equivalent of if err := something; err != nil {}. Why would we have err in a global variable if we're not going to use it down the line?

[oleiade]

As in my previous comment: I wholeheartedly disagree with you folks, but I don't have a strong enough opinion about it to argue 😄

This change was inline with AAA testing, and I should probably have renamed err to gotErr to be more explicit. Personally I find it a better practice, because it's more explicit, to be as explicit and stateful as possible in the context of a test.

Regarding the if err := something; err != nil {} format, I think it's a matter of personal preference, and I don't care enough to argue about it, but personally I find this syntax not so readable and always avoid it in my code. Once again it's personal and likely linked to our own experiences, but I do find:

err := something()
if err != nil {
  log.Fatal(err)
}

Because it creates a sequence of visual blocks that are easier for me to parse visually. Each line does only one thing. I personally tend to prefer explicit to shorter.

[na--]

Explicit > shorter is fine, but I personally prefer to have each variable live in the smallest possible scope, whenever possible, to avoid "pollution". It is also easier to reason about, at least for me, because with require.NoError(t, someFunc()) and if err := something; err != nil {} I don't need to wonder if err will be used below.

[oleiade]

Closing this PR as the changes they bring raise some disagreements that are rather about coding practices than logic changes (of which there were actually none). Thanks for your input, folks 👍🏻

[na--]

@oleiade, some of the changes here (e.g. removal of t.Run() sub-tests, reordering of some test statements) looked fine, do you want to make a separate PR (or reopen this one) with just them?