Refactor tests and increase required go version to 1.18 #9
Conversation
* Split large tests into multiple sub-tests * Add tests for per-call allocator support * Always run `touch` command tests Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
There was a problem hiding this comment.
LGTM, definitely an improvement over the current tests.
This is a suggestion for a future PR and doesn't block merging this one: testWithClient is getting quite long, and I don't love the dependencies between some of the scenarios (eg. the set and get scenarios must be run in sequence to pass) - perhaps testWithClient could be broken up into a number of self-contained test methods?
| t.Run("set", func(t *testing.T) { | ||
| foo := &Item{Key: "foo", Value: []byte("fooval"), Flags: 123} | ||
| err := c.Set(foo) | ||
| checkErr(err, "first set(foo): %v", err) |
There was a problem hiding this comment.
Probably something for a subsequent PR: it might be nice to use github.com/stretchr/testify to make the assertions clearer and remove some of the duplication
There was a problem hiding this comment.
Great idea! I'll take care of it in a follow up PR.
I didn't even realize they depended on each other like that. I'll fix before merging this PR |
|
This seems to be going a long way away from the initial intention to host a fork of changes we expected to be upstreamed some day. |
Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>
I wasn't aware we were still intending to upstream these changes. |
|
All of the initial changes were already PRs upstream. |
|
OK, I can update the README to indicate that. We've already taken on the burden of maintaining forked Thanos caching code in Mimir and now dskit and to me this client is even simpler than that code (and seems "done" for the most part) . |
touchcommand testsSigned-off-by: Nick Pillitteri nick.pillitteri@grafana.com