Refactoring Redis

[kwitsch]

Changes:

• config.RedisConfig renamed to config.Redis
• config.Redis got its own file
• config.Redis implements config.Configurable
• config.Redis got unittests
• redis.Client doesn't store it's own context
• methodes of redis.Client expect a context where it's needed
• redis.Client closes its channels when the context is done

Part of #1264

[codecov]

Codecov Report

Attention: 20 lines in your changes are missing coverage. Please review.

Comparison is base (15bd383) 93.87% compared to head (76664ee) 93.81%.

Files	Patch %	Lines
redis/redis.go	66.66%	15 Missing and 5 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1271      +/-   ##
==========================================
- Coverage   93.87%   93.81%   -0.06%     
==========================================
  Files          75       77       +2     
  Lines        6023     6080      +57     
==========================================
+ Hits         5654     5704      +50     
- Misses        284      289       +5     
- Partials       85       87       +2     

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

[ThinkChaos]

Have very minor suggestions, looks good and splitting config.go more is always great!

[ThinkChaos]

The approach you did looks fine to me. I had gone with the context per describe technique to not have to add initialization to each test and limit the changes I had to do, but I think what you did is what ginkgo actually expects.
Up to you with how you prefer it, I think as long as the tests work, we're good :)

[kwitsch]

I try to stay as close to the ginkgo principal/best practice since they seem to get flaky otherwise when running in parallel.

I'll fix the tests tomorrow when I'm back home. 👍

[kwitsch]

Currently uncovered are ctx.Done() returns and the sentinel client since I don't know how to test them properly. 🫣

[ThinkChaos]

If you really want to get coverage for the paths where the context is done, you can call the functions with a already cancelled context, that works in most cases.
And maybe make the channel send/ctx.Done() receive pattern a function and test it directly with a cancelled context.

But yeah it's can be annoying to test and worse case it doesn't have too much impact if broken so I'd say it's not mandatory to get 100%, testing the logic is more important :)

[kwitsch]

If you really want to get coverage for the paths where the context is done, you can call the functions with a already cancelled context, that works in most cases.

Thought about that but got stuck at GetRedisCache since there are surrounding function calls that are context aware it has to be closed at the right time and I couldn't think of a way to get the timing right. 🫣

And maybe make the channel send/ctx.Done() receive pattern a function and test it directly with a cancelled context.

I've already started working on something like that but as part of utility functions and determined it would be out of scope for this PR. So maybe add it later on? 🤔

But yeah it's can be annoying to test and worse case it doesn't have too much impact if broken so I'd say it's not mandatory to get 100%, testing the logic is more important :)

I like numbers and seeing em drop is discouraging... 😅

[ThinkChaos]

I've already started working on something like that but as part of utility functions and determined it would be out of scope for this PR. So maybe add it later on? 🤔

Yeah for sure can be done later just trying to give ideas for the tests.
IMO last thing I'd like to see done is just not have any context.TODO() in the tests, but if you want to do that later it's fine too.

[kwitsch]

Yeah for sure can be done later just trying to give ideas for the tests.

Was meant like: I was thinking in the same direction 😉

[kwitsch]

I added util.CtxSend which provides a context aware way to send something to a channel(should be usefull outside of redis).

Im currently satisfied with the result even if the coverage dropped since there are now some error checks which are troublesome to test but should work(if err != nil {log & return}) in theory. 😅