Write FIFO cache unit tests #1201
Conversation
cache/fifo_test.go
Outdated
| limit: 10, | ||
| maxVal: 10, | ||
| check: func(t *testing.T, cache *FIFO[int, int]) { | ||
| require := require.New(t) | ||
| _, ok := cache.Get(0) | ||
| require.False(ok) | ||
| v, ok := cache.Get(1) | ||
| require.True(ok) | ||
| require.Equal(1, v) | ||
| }, |
There was a problem hiding this comment.
I think these tests are on the right track but still find that these are a little hard to reason about (particularly the limit/maxval) parameter are not obvious on how they work until you read the test body.
I think one way we could make this cleaner (and remove the check field) would be to make a struct like so:
func TestFIFOCache(t *testing.T) {
...
type put struct {
i int
ok bool
}
type get struct {
i int
ok bool
}
and if we did this I think we could write tests as:
{
puts:[]put{
{i: 1, ok: true}
{i: 1, ok: false}
}
gets:[]get{
{i: 1, ok: true}
}
}
There was a problem hiding this comment.
This structure looks like a nice improvement. This should make it a bit more readable and we can just use a size like 2, so it's not excessive.
I think we should test the following cases here:
- no elements removed when the cache is less than full
- no elements removed when cache is exactly full
- elements removed in FIFO order when cache overfills
- elements removed in FIFO order after additional get/put ops for element to be removed FIFO
I think we're currently missing the last case. There's a unit test to make sure if we insert the first element again, then it stays in the cache, but we should additionally test that this does not change its position in the queue for removal to differentiate FIFO from LRU.
There was a problem hiding this comment.
Thanks both !
I have changed the test suite to only have a name, osp pairs and it's way more compact/readable.
Also added tests cases to cover @aaronbuchwald ones, haven't implemented the last one for now (TODO!)
There was a problem hiding this comment.
I'm not sure if there is a real difference between the two last test cases, the case "elements removed in FIFO order when cache overfills" seems to fulfil both, please tell me if that's wrong.
There was a problem hiding this comment.
Actually they are different, see the test case "elements removed in FIFO order and not LRU".
cache/fifo_test.go
Outdated
| maxVal: 8, | ||
| check: func(t *testing.T, cache *FIFO[int, int]) { | ||
| require := require.New(t) | ||
| exists := cache.Put(9, 9) |
There was a problem hiding this comment.
Usually it's idiomatic to use ok in Go instead of exists
There was a problem hiding this comment.
I think that we should only use ok instead of exists for Get because ok seems to imply that something can go wrong while it just means that the value is already in the cache so no need to insert it again. It seems like ok could be used if the cache does not support overwriting value for instance.
cache/fifo_test.go
Outdated
| require := require.New(t) | ||
|
|
||
| cache, err := NewFIFO[int, int](tt.limit) | ||
| require.Equal(tt.expected, err) |
There was a problem hiding this comment.
nit: use ErrorIs. This does a match to check if the error was wrapped instead of just the text which is more durable (ref)
There was a problem hiding this comment.
I can't use ErrorIs, the error that says "maxSize must be greater than 0" is private and cannot be imported. It's defined in https://github.com/ava-labs/avalanchego/blob/321e727328a86bcfdb8f5bef164e7d50e698408b/utils/buffer/bounded_nonblocking_queue.go#L11
This error check has been moved to a unit test TestEmptyCacheFails that checks that this error occurs when the cache is initialized with a length of 0, so it's better contained.
No description provided.