tests(refactor): Improve consistency and documentation for test helpers

[georglauterbach]

Description

Refactors test helper. Naming was incoherent and documentation was missing.

Type of change

• Improvement (non-breaking change that does improve existing functionality)

Checklist:

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation (README.md or the documentation under docs/)
• If necessary I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes

[polarathene]

I spotted a few minor concerns and suggestions, and one bug (change to implicit container name in mail_changedetector.bats). Otherwise looking pretty good 👍

[georglauterbach]

I rewrote history so we can rebase-merge this. I incorporated the review feedback from @polarathene into the new commits. I think reviewing commit by commit is again a nice strategy, as this PR again is rather big.

---

What's new with the new commits? I added one nice feature: for some test helper functions, we can now omit the container name if CONTAINER_NAME is already set. I made sure this functionality is robust and added a function that handles the naming. I also made sure documentation for this is on-point. It looks like this in VS Code for me:

[polarathene]

I think reviewing commit by commit is again a nice strategy, as this PR again is rather big.

That's extremely tedious for us to review though with these kind of changes.

It takes a long time to go through each commit and compare line by line like I did last time to catch any errors. Each time you rewrite history like that it reduces the quality of the review as less effort will be put into repeating that process.

I rewrote history so we can rebase-merge this.

I don't see what warrants a merge other than squash in this PR?

I'd would have preferred to see new commits with requested changes so the diff of what actually changed was much smaller, then after approval by all means rewrite history then and I'll trust that's all it was.

---

What's new with the new commits?

That sounds great, and I love the VSCode screenshot showing off the doc tooltip! 😀

---

I'll start a review now, but like I said a thorough review is taxing, so I'm placing more trust in this time round that we don't have any accidents like in the initial review with testing the wrong container for a method 😖 (easy mistake to make, but hard to spot in large noisy/repetitive diffs)

EDIT: Praise Github, it actually offers a diff of before / after the history rewrite 🙏

[georglauterbach]

Yeah, again sorry for the force-push, but I think that squash-merging is not the way to go. If I am overruled, I will definitely squash-merge it though. Not a big deal :)

EDIT: Praise Github, it actually offers a diff of before / after the history rewrite pray

I definitely knew about that, thought you did too, haha :D That should do the trick and make the review easier, right?:)

[polarathene]

I also wanted to inquire about the changes that swap the load order to setup,common instead of common,setup? Was there a reason for that all? I had been using the latter as alphabetical sorting 😅

[polarathene]

I definitely knew about that, thought you did too, haha :D That should do the trick and make the review easier, right?:)

Yes, but it still took me about 2 hours to comb through.

I definitely would have missed a bunch if I had to wade through the full diff with old history interleaved/overwritten.

---

Yeah, again sorry for the force-push, but I think that squash-merging is not the way to go.

I'm not entirely opposed to it, but I'm not sure what the benefit is? The master branch would have a single logical commit tied to this PR which has full discussion of review and commit history.

The changes are all related / dependent, so makes more sense to me for them to be bundled into a squash merge.

If this is about the two commits that refactor mail_privacy.bats and adjusted Makefile, those could be extracted out to a separate PR? It's a few clicks for me to pull those commits out of the branch with cherry-pick into a stash that can be applied after this is merged, and push those up into a follow-up PR. Especially since they're cleanly isolated from the other changes in this PR.

[georglauterbach]

I also wanted to inquire about the changes that swap the load order to setup,common instead of common,setup? Was there a reason for that all? I had been using the latter as alphabetical sorting sweat_smile

No particular reason; I guess a matter of style again. I figured the setup functions are needed before the common functionality but it does not actually matter.

---

If this is about the two commits that refactor mail_privacy.bats and adjusted Makefile, those could be extracted out to a separate PR? It's a few clicks for me to pull those commits out of the branch with cherry-pick into a stash that can be applied after this is merged, and push those up into a follow-up PR. Especially since they're cleanly isolated from the other changes in this PR.

Yes, this may be a good idea to keep the diff small. I will update this PR and apply PF feedback; you can then go ahead and pick them out of this PR :) We can then squash this PR, that's fine with me.

[georglauterbach]

UPDATE: Applied PR feedback; you can go ahead and pick out the 2 two commits as mentioned earlier @polarathene :)

[polarathene]

LGTM 👍

I left your original two commits alone after extracting out the two files we discussed into their own PRs. Then merged the two review feedback commits into one.