Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

test(store/snapshot): fix flaky test #20078

Merged
merged 9 commits into from
Apr 26, 2024
Merged

test(store/snapshot): fix flaky test #20078

Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
9f985c0
test(store/snapshot): fix flaky test
EmilGeorgiev Apr 17, 2024
0aebb60
add comments
EmilGeorgiev Apr 17, 2024
358b7a5
fix comments
EmilGeorgiev Apr 18, 2024
db7700d
add missing test case
EmilGeorgiev Apr 20, 2024
6f4637d
add missing test case
EmilGeorgiev Apr 23, 2024
047dea8
Merge branch 'main' into fix-test-flaky-19834
cool-develope Apr 23, 2024
5044de3
assert for heights in the test
EmilGeorgiev Apr 26, 2024
27e71ab
Merge branch 'fix-test-flaky-19834' of github.com:EmilGeorgiev/cosmos…
EmilGeorgiev Apr 26, 2024
46dabc2
Merge branch 'main' into fix-test-flaky-19834
EmilGeorgiev Apr 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 41 additions & 15 deletions store/snapshots/store_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"errors"
"io"
"sync"
"sync/atomic"
"testing"
"time"

Expand Down Expand Up @@ -262,6 +263,7 @@ func TestStore_Prune(t *testing.T) {
}

func TestStore_Save(t *testing.T) {
t.Parallel()
store := setupStore(t)
// Saving a snapshot should work
snapshot, err := store.Save(4, 1, makeChunks([][]byte{{1}, {2}}))
Expand Down Expand Up @@ -318,19 +320,43 @@ func TestStore_Save(t *testing.T) {

// Saving a snapshot should error if a snapshot is already in progress for the same height,
// regardless of format. However, a different height should succeed.
ch = make(chan io.ReadCloser)
mtx := sync.Mutex{}
mtx.Lock()
go func() {
mtx.Unlock()
_, err := store.Save(7, 1, ch)
require.NoError(t, err)
}()
mtx.Lock()
defer mtx.Unlock()
_, err = store.Save(7, 2, makeChunks(nil))
require.Error(t, err)
_, err = store.Save(8, 1, makeChunks(nil))
require.NoError(t, err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the case of different height should succeed is missed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

close(ch)
var (
wgStart, wgDone sync.WaitGroup
errCount atomic.Uint32
)
const n = 3
wgStart.Add(n + 1)
wgDone.Add(n + 1)
for i := 0; i <= n; i++ {
ch = make(chan io.ReadCloser, 1)
ch <- &ReadCloserMock{} // does not block on a buffered channel
close(ch)
go func(i int) {
wgStart.Done()
wgStart.Wait() // wait for all routines started
var err error
if i < n {
_, err = store.Save(7, 1, ch)
} else {
_, err = store.Save(8, 1, makeChunks(nil))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good intend but this condition on n does not make it very readable, IMHO.
Either you can cover the second test case in separate code block or you refactor towards the height as argument. For example:

	var (
		wgStart, wgDone sync.WaitGroup
		mu              sync.Mutex
		gotErrHeights   []uint64
	)
	srcHeights := []uint64{7, 7, 7, 8, 9}
	wgStart.Add(len(srcHeights))
	wgDone.Add(len(srcHeights))
	for _, h := range srcHeights {
		ch = make(chan io.ReadCloser, 1)
		ch <- &ReadCloserMock{} // does not block on a buffered channel
		close(ch)
		go func(height uint64) {
			wgStart.Done()
			wgStart.Wait() // wait for all routines started
			if _, err = store.Save(height, 1, ch); err != nil {
				mu.Lock()
				gotErrHeights = append(gotErrHeights, height)
				mu.Unlock()
			}
			wgDone.Done()
		}(h)
	}
	wgDone.Wait() // wait for all routines completed
	assert.Equal(t, []uint64{7, 7}, gotErrHeights)

This approach has the benefit that you can identify the failing height rather than relying on the count. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great point

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

}
if err != nil {
errCount.Add(1)
}
wgDone.Done()
}(i)
}
wgDone.Wait() // wait for all routines completed
assert.Equal(t, uint32(n-1), errCount.Load())
}

type ReadCloserMock struct {
}

func (r ReadCloserMock) Read(p []byte) (n int, err error) {
return len(p), io.EOF
}

func (r ReadCloserMock) Close() error {
return nil
Comment on lines +349 to +357
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ReadCloserMock is used for testing but currently returns a fixed response. Consider enhancing this mock to return configurable data to increase the flexibility and realism of the tests.

- func (r ReadCloserMock) Read(p []byte) (n int, err error) {
-     return len(p), io.EOF
+ func (r *ReadCloserMock) Read(p []byte) (n int, err error) {
+     if len(r.data) > 0 {
+         n = copy(p, r.data)
+         r.data = r.data[n:]
+         return n, nil
+     }
+     return 0, io.EOF
+ }
+
+ func NewReadCloserMock(data []byte) *ReadCloserMock {
+     return &ReadCloserMock{data: data}
+ }
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
type ReadCloserMock struct {
}
func (r ReadCloserMock) Read(p []byte) (n int, err error) {
return len(p), io.EOF
}
func (r ReadCloserMock) Close() error {
return nil
type ReadCloserMock struct {
data []byte
}
func (r *ReadCloserMock) Read(p []byte) (n int, err error) {
if len(r.data) > 0 {
n = copy(p, r.data)
r.data = r.data[n:]
return n, nil
}
return 0, io.EOF
}
func (r ReadCloserMock) Close() error {
return nil
}
func NewReadCloserMock(data []byte) *ReadCloserMock {
return &ReadCloserMock{data: data}
}

}
Loading