maint(testutils/golden): Add GoldenTracker to track the used golden files and use it

[3v1n0]

If a test using a golden file gets renamed or removed we don't have any
strategy to check if the related golden file(s) got updated as well.

To make this possible, add a new struct that allows tests to track their
test cases and check if the related golden files are being used, or fail
otherwise

/cc @adombeck as per comments in #598 (comment)

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.88%. Comparing base (ffae569) to head (5689351).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #601      +/-   ##
==========================================
+ Coverage   82.84%   82.88%   +0.04%     
==========================================
  Files          80       80              
  Lines        8514     8514              
  Branches       75       75              
==========================================
+ Hits         7053     7057       +4     
+ Misses       1130     1127       -3     
+ Partials      331      330       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[denisonbarbosa]

Ok, I have some reservations regarding these changes:

1. If the golden tracker is being used in every test that relies on golden files, then it should become a required argument of the LoadWithUpdateGolden functions instead of an optional one.

2. If we are using it on every single test instance, maybe we can figure out a way of not having to "manually" instantiate the tracker and hook it to the test? I didn't investigate more into this yet, so I'm not sure if it's possible, but maybe it's worth a look?

Also, some food for thoughts: Is this even necessary?
I know it could be useful not having to worry about cleaning golden files, but, IMHO, remembering to delete/rename the files should be akin to renaming a variable/function in the code, i.e. when we rename/remove a test, we must remember to look at what this renaming/removing will trigger and adjust it, no? Manually dealing with golden files can be a pain, but it also helps us figure out what went wrong or what behavior is not covered/is not required anymore. Particularly, when I need to update a test or a test case, I remove every single golden file related to that test and then run the update command, then I look at the git diff to see if that's what I expected to happen (if the changes make sense, what file is missing, what file is new and so on).

[3v1n0]

1. it's fine, I made it not mandatory because it was easier to enable it only in specific cases, but maybe it can be enforced.
2. Eh, I'm not sure if this can be done, another option is to do it at global TestMain level, but still it requires some setting up...

Also, some food for thoughts: Is this even necessary?

I wouldn't define it necessary, but it's an useful helper to avoid leaving around leftovers... We normally remove them, but as proved in various previous PRs we're not great at that since it happened already various times of having leftovers...

[adombeck]

I'm very much in favor of automating the cleanup of obsolete golden files. I've seen multiple cases already where we forgot to remove a golden file and didn't notice it before merging.

Manually dealing with golden files can be a pain, but it also helps us figure out what went wrong or what behavior is not covered/is not required anymore. Particularly, when I need to update a test or a test case, I remove every single golden file related to that test and then run the update command, then I look at the git diff to see if that's what I expected to happen

The git diff will still tell us what's happening, and if it's ensured that obsolete golden files are removed, the diff better reflects the impact of the change (it will show you what is not being tested anymore).

[didrocks]

I'm very much in favor of automating the cleanup of obsolete golden files. I've seen multiple cases already where we forgot to remove a golden file and didn't notice it before merging.

+1. However, keep in mind the cases where we only have some subtests running (with a -run parameters). Maybe it could be a test injected when we know we run all the tests for a package and fails if some files were not used?

In adsys, we didn’t really get the case due to file tree projectionning, but here, the approach is slightly different.

[3v1n0]

+1. However, keep in mind the cases where we only have some subtests running (with a -run parameters)

Yah, this is already considered and handled:

https://github.com/ubuntu/authd/pull/601/files#diff-c30f5aac548e7e06ca03b621357484e38819feced6cfbd41572ce90d7e7e32e1R171-R177

So, for the same reason we may do this as a form of a test, but running as last isn't possible.