changes suggested by lint check

[ranakan19]

Separate PR with linter changes for easier review, otherwise #1057 would have been even bigger to review.

pre-req - #1073 to be merged

[alexeykazakov]

Looks good!

[alexeykazakov]

I'm curios why linter doesn't like Equal() here... :)

[MatousJobanek]

Could you please revert the InDelta changes back to Equal?

[MatousJobanek]

This doesn't test the same - the value should be 2, not something between 0 and 2.
So I have the same question as Alexey - why is the Equal so problematic? We use it all over the place. And why the Delta makes is better? I see it rather as a regression.

[xcoulon]

the testifylint linter complains about the usage of Equal for float numbers and suggests we use InDelta or InEpsilon instead: https://github.com/Antonboom/testifylint?tab=readme-ov-file#float-compare

The delta arg is the "margin of error" of the value to check, not the lower-bound of a range (eg: [0,2])

InDelta asserts that the two numerals are within delta of each other.

We could also use this wrapper func, instead

[alexeykazakov]

If I'm reading it right then from the technical perspective those two assertion are 100% identical:

• assert.Equal(t, float64(1), promtestutil.ToFloat64(m.WithLabelValues("member-1")))
• assert.InDelta(t, float64(1), promtestutil.ToFloat64(m.WithLabelValues("member-1")), 0)

First checks if two float64 numbers are equal. The second check if two float64 numbers are within a delta 0 which means exactly equal. So it's just a trick to make the linter happy.
If we wanted to avoid the potential floating point rounding issues we would need to use something like
assert.InDelta(t, float64(1), promtestutil.ToFloat64(m.WithLabelValues("member-1")), 0.0001)

I didn't look very closely but It looks like we shouldn't really have a rounding problem in this case. We use floats (because of how Prometheus metrics work I guess?) but they really always round to integers. So I would rather use Equal() and suppress the linter so the code is easy to understand.
If for whatever reason we can expect the the actual metric can be slightly off (1.0001 instead of 1.0) for example then I would use InDelta() with some small delta. But I doubt we need it.

[ranakan19]

So I would rather use Equal() and suppress the linter so the code is easy to understand.

I'd say using the wrapper function xavier suggested would keep the code easy to understand without suppressing the linter.

[ranakan19]

@alexeykazakov I was thinking something similar when i wrote the delta to be 0 expecting that the two numbers would be exactly equal because of the rounding off.
On reading further I understand that the concern here is the way a float variable is represented in memory i.e even though we expect the value to be 2 - it could be stored in memory as 1.999999999 as the closest float to 2. ref- https://medium.com/pragmatic-programmers/testing-floating-point-numbers-in-go-9872fe6de17f
This explains:

1. Why the linter expects to use a InDelta or Inepsilon for comparing floats.
2. In the function InDelta/InEpsilon - the value of delta/epsilon should not be zero (it goes against the whole concept of using these separate functions for comparing float variable)

So looks like wrapper function would indeed be the best solution here?

[ranakan19]

oh well, looks like using the wrapper won't be straightforward - because of this there is import cycling dependency - so will have to rearrange a few things.

[alexeykazakov]

Let's maybe move the wrappers to common?

[ranakan19]

yeah sure, do you want me to that in this same PR or maybe a separate one would be better because the wrapper is used in a lot of places.

[alexeykazakov]

A separate PR would be easy to handle I think.

[ranakan19]

here is the PR - #1073
once those are merged I will update the current PR as well accordingly.

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexeykazakov, MatousJobanek, ranakan19, xcoulon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [MatousJobanek,alexeykazakov,ranakan19,xcoulon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[sonarcloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.08%. Comparing base (57e09f4) to head (4575f79).
Report is 2 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1072   +/-   ##
=======================================
  Coverage   85.08%   85.08%           
=======================================
  Files          55       55           
  Lines        5034     5034           
=======================================
  Hits         4283     4283           
  Misses        580      580           
  Partials      171      171