test: Allow soft-referencing of Cids in tests

[AndrewSisley]

Relevant issue(s)

Resolves #3172

Description

Allows soft-referencing of Cids in tests.

When encountering an unpleasant change-detector problem in #3173 I realised we had no need to reference Cids based on index, and we really only ever cared that they were valid, and either not the same as another cid, or the same.

This change allows us to soft-reference them without ever really caring about where they came from. This allows us to give them descriptive ids, and avoids having to have the test framework from having to fetch them independently from the DB - something that raises questions over whether we are testing the test framework or the production code, especially in the change detector.

The system can be extended to other types fairly easily.

How has this been tested?

I had a fairly long play around giving it bad ids in tests to make sure it would actually fail when appropriate.

[AndrewSisley]

thought: I did consider making this a map[any]any - allowing validators to independently manage/extend state the same was as we can with a context.Context, but decided it was overkill/too-early to do that.

It would mean that implementations of Validator would be entirely self contained - changing them or adding new ones would not require changes to any other types.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.39%. Comparing base (12669e4) to head (a8ae842).
Report is 1 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #3176      +/-   ##
===========================================
- Coverage    77.41%   77.39%   -0.02%     
===========================================
  Files          357      357              
  Lines        34809    34809              
===========================================
- Hits         26944    26937       -7     
- Misses        6249     6251       +2     
- Partials      1616     1621       +5     

Flag	Coverage Δ
all-tests	77.39% <ø> (-0.02%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 10 files with indirect coverage changes

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 12669e4...a8ae842. Read the comment docs.

[islamaliev]

Awesome proposal, but I requires some more elaboration as it has potentially large impact on how will use asserts in our tests in the future.

[islamaliev]

praise: that's cool stuff. I like this approach very much as it will allow us easily introduce individual matchers instead of checking exact equality.

I actually wanted myself to do something similar. I was even about to write a task a few weeks back, but something distracted me.

suggestion: when I was thinking about this the other day I had an ideal of using one of well established matching frameworks. My preference was Gomega. There are other ones as well like GoCrest.

For example Gomega matcher has the following interface:

type GomegaMatcher interface {
    Match(actual interface{}) (success bool, err error)
    FailureMessage(actual interface{}) (message string)
    NegatedFailureMessage(actual interface{}) (message string)
}

They have a bunch of very handy matchers for maps, slices, strings, async stuff and others.

Sadly testify asserts can't be used here, as they execute assertions right away and use no interfaces, hence can't be customized.

I do not propose to use them right now, because there a value in keeping things simple (as you did) for now. But it's something to consider (maybe even now).

But I believe replicating their matcher interface makes a lot of sense.
That means matchers don't do asserts themselves (e.g. no require.Equal...) they just match and return (bool, error). If it returns false we can ask it for the reason by calling FailureMessage. If we don't do this, we won't be able to combine our matchers (or validators).
For example, it works fine if you use a single matcher (like in your examples):

"cid": testUtils.NewUniqueCid("name"),

But what if you want to combine them like:

"cid": testUtils.AnyOf(
    testUtils.NewUniqueCid("name"),
    testUtils.NewUniqueCid("age"),
)

There is no way you can do it, because testUtils.NewUniqueCid("name") will panic right away if it doesn't match.

[AndrewSisley]

But what if you want to combine them

That is a really nice thought - thanks Islam! And I like the GomegaMatcher interface.

Like you seemed to hint, I also am a little torn as to whether to bring that complexity in now or not - I'll see what the others say but am leaning towards keeping it simple in the short term. Changing this later should not require changes to the tests, so it should be easy enough to postpone it.

If leaving as is, I'll create an link a new issue via a code-comment todo so that we don't lose this thought.

• Implement or create and link ticket

[shahzadlone]

+1 for gomega, the Ω can seem a bit trippy at first haha.

[islamaliev]

suggestion: I hate any. I believe this be the last resort in all cases as it kills strong typing.
In your examples you use string, so why not to make it string or int?

[AndrewSisley]

The thing is this doesn't benefit from being a strong type - and actually gains by not being so, as keys of different types are now possible (like in a context).

I think this one is best left up to the test author to pick whatever they feel like is most descriptive.

[islamaliev]

question: don't you think using int would be easier to read. At first I though these strings have some meaning (maybe you have predefined list of "composite", "update" ...) but reviewing further I learning it's just an arbitrary unique id. I think int is easier to use here and less prone to spelling errors.

[AndrewSisley]

I definitely understand what you mean, and it amused me a little to see this suggestion after your strongly typed id suggestion.

A string here is less information dense than an int, but typo's dont actually matter too much here - as if the same cid is referenced twice with different strings the test will fail (this does expose us to accidentally passing bad production code though if cids should be the same but differ).

Another option would be an enum, or variant of,that seems a bit overkill though:

func NewUniqueCid(keyVals ...any) *UniqueCid { ... }

testUtils.NewUniqueCid(testUtils.CreateCid, "age")

[fredcarle]

I'm not a fan of the enum suggestion. I don't mind the string. I do agree that int would be less error prone but strings are more descriptive (as descriptive as the test author wants).

[fredcarle]

At first I was like thinking — how the fuck is this better than the initial PR — but after letting it simmer a little I realized that we don't actually care what the CIDs are in most of those tests. To the point that when they changed we just copied what was returned and replaced. As long as we don't lose the ability to flag breaking changes, I'm quite happy with this change. I also really like the new Validator interface 👍.

[islamaliev]

It's a good first step. Good initiative, Andy.
Approving now, assuming #3189 won't take too long to land.
Maybe we should also rename Validator to Matcher.