Fix of all known flaky tests

.gitlab-ci.yml

@@ -45,6 +45,8 @@ lint:
     - branches
   script:
     - make lint
+  tags:
+    - medium
 
 test:
   stage: test
@@ -60,6 +62,8 @@ test:
     - pass init `gpg --list-keys | grep "^   " | tail -1 | tr -d '[:space:]'`
     # Then finally run the tests
     - make test
+  tags:
+    - medium
 
 test-integration:
   stage: test

internal/imap/cache/cache_test.go

@@ -18,7 +18,6 @@
 package cache
 
 import (
-	"bytes"
 	"fmt"
 	"testing"
 	"time"
@@ -59,34 +58,34 @@ func TestClearOld(t *testing.T) {
 }
 
 func TestClearBig(t *testing.T) {
-	msg := []byte("Test message")
+	r := require.New(t)
+	wantMessage := []byte("Test message")
 
-	nSize := 3
+	wantCacheSize := 3
-	cacheSizeLimit = nSize*len(msg) + 1
+	nTestMessages := wantCacheSize * wantCacheSize
-	cacheTimeLimit = int64(nSize * nSize * 2) // be sure the message will survive
+	cacheSizeLimit = wantCacheSize*len(wantMessage) + 1
+	cacheTimeLimit = int64(1 << 20) // be sure the message will survive
 
-	// It should have more than nSize items.
+	// It should never have more than nSize items.
-	for i := 0; i < nSize*nSize; i++ {
+	for i := 0; i < nTestMessages; i++ {
 		time.Sleep(1 * time.Millisecond)
-		SaveMail(fmt.Sprintf("%s%d", testUID, i), msg, bs)
+		SaveMail(fmt.Sprintf("%s%d", testUID, i), wantMessage, bs)
-		if len(mailCache) > nSize {
+		r.LessOrEqual(len(mailCache), wantCacheSize, "cache too big when %d", i)
-			t.Error("Number of items in cache should not be more than", nSize)
-		}
 	}
 
 	// Check that the oldest are deleted first.
-	for i := 0; i < nSize*nSize; i++ {
+	for i := 0; i < nTestMessages; i++ {
 		iUID := fmt.Sprintf("%s%d", testUID, i)
 		reader, _ := LoadMail(iUID)
-		if i < nSize*(nSize-1) && reader.Len() != 0 {
+		mail := mailCache[iUID]
-			mail := mailCache[iUID]
-			t.Error("LoadMail should return empty but have:", mail.data, iUID, mail.key.Timestamp)
-		}
-		stored := make([]byte, len(msg))
-		_, _ = reader.Read(stored)
 
-		if i >= nSize*(nSize-1) && !bytes.Equal(stored, msg) {
+		if i < (nTestMessages - wantCacheSize) {
-			t.Error("LoadMail returned wrong message:", stored, iUID)
+			r.Zero(reader.Len(), "LoadMail should return empty, but have %s for %s time %d ", string(mail.data), iUID, mail.key.Timestamp)
+		} else {
+			stored := make([]byte, len(wantMessage))
+			_, err := reader.Read(stored)
+			r.NoError(err)
+			r.Equal(wantMessage, stored, "LoadMail returned wrong message: %s for %s time %d", stored, iUID, mail.key.Timestamp)
 		}
 	}
 }

internal/store/event_loop_test.go

@@ -24,7 +24,6 @@ import (
 
 	"github.com/ProtonMail/proton-bridge/pkg/pmapi"
 	"github.com/golang/mock/gomock"
-	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
@@ -54,10 +53,10 @@ func TestEventLoopProcessMoreEvents(t *testing.T) {
 		}, nil),
 	)
 	m.newStoreNoEvents(true)
-	m.client.EXPECT().ListMessages(gomock.Any()).Return([]*pmapi.Message{}, 0, nil).AnyTimes()
 
-	// Event loop runs in goroutine and will be stopped by deferred mock clearing.
+	// Event loop runs in goroutine started during store creation (newStoreNoEvents).
-	go m.store.eventLoop.start()
+	// Force to run the next event.
+	m.store.eventLoop.pollNow()
 
 	// More events are processed right away.
 	require.Eventually(t, func() bool {
@@ -78,38 +77,42 @@ func TestEventLoopUpdateMessageFromLoop(t *testing.T) {
 	subject := "old subject"
 	newSubject := "new subject"
 
-	// First sync will add message with old subject to database.
+	m.newStoreNoEvents(true, &pmapi.Message{
-	m.client.EXPECT().GetMessage("msg1").Return(&pmapi.Message{
 		ID:      "msg1",
 		Subject: subject,
-	}, nil)
+	})
-	// Event will update the subject.
-	m.client.EXPECT().GetEvent("latestEventID").Return(&pmapi.Event{
-		EventID: "event1",
-		Messages: []*pmapi.EventMessage{{
-			EventItem: pmapi.EventItem{
-				ID:     "msg1",
-				Action: pmapi.EventUpdate,
-			},
-			Updated: &pmapi.EventMessageUpdated{
-				ID:      "msg1",
-				Subject: &newSubject,
-			},
-		}},
-	}, nil)
 
-	m.newStoreNoEvents(true)
+	eventReceived := make(chan struct{})
+	m.client.EXPECT().GetEvent("latestEventID").DoAndReturn(func(eventID string) (*pmapi.Event, error) {
+		defer close(eventReceived)
+		return &pmapi.Event{
+			EventID: "event1",
+			Messages: []*pmapi.EventMessage{{
+				EventItem: pmapi.EventItem{
+					ID:     "msg1",
+					Action: pmapi.EventUpdate,
+				},
+				Updated: &pmapi.EventMessageUpdated{
+					ID:      "msg1",
+					Subject: &newSubject,
+				},
+			}},
+		}, nil
+	})
 
-	// Event loop runs in goroutine and will be stopped by deferred mock clearing.
+	// Event loop runs in goroutine started during store creation (newStoreNoEvents).
-	go m.store.eventLoop.start()
+	// Force to run the next event.
+	m.store.eventLoop.pollNow()
 
-	var err error
+	select {
-	assert.Eventually(t, func() bool {
+	case <-eventReceived:
-		var msg *pmapi.Message
+	case <-time.After(5 * time.Second):
-		msg, err = m.store.getMessageFromDB("msg1")
+		require.Fail(t, "latestEventID was not processed")
-		return err == nil && msg.Subject == newSubject
+	}
-	}, time.Second, 10*time.Millisecond)
+
+	msg, err := m.store.getMessageFromDB("msg1")
 	require.NoError(t, err)
+	require.Equal(t, newSubject, msg.Subject)
 }
 
 func TestEventLoopUpdateMessage(t *testing.T) {

internal/store/store_test.go

@@ -18,11 +18,12 @@
 package store
 
 import (
+	"fmt"
 	"io/ioutil"
 	"os"
 	"path/filepath"
-	"sync"
 	"testing"
+	"time"
 
 	storemocks "github.com/ProtonMail/proton-bridge/internal/store/mocks"
 	"github.com/ProtonMail/proton-bridge/pkg/pmapi"
@@ -89,7 +90,7 @@ func initMocks(tb testing.TB) (*mocksForStore, func()) {
 	}
 }
 
-func (mocks *mocksForStore) newStoreNoEvents(combinedMode bool) { //nolint[unparam]
+func (mocks *mocksForStore) newStoreNoEvents(combinedMode bool, msgs ...*pmapi.Message) { //nolint[unparam]
 	mocks.user.EXPECT().ID().Return("userID").AnyTimes()
 	mocks.user.EXPECT().IsConnected().Return(true)
 	mocks.user.EXPECT().IsCombinedAddressMode().Return(combinedMode)
@@ -102,20 +103,19 @@ func (mocks *mocksForStore) newStoreNoEvents(combinedMode bool) { //nolint[unpar
 	})
 	mocks.client.EXPECT().ListLabels()
 	mocks.client.EXPECT().CountMessages("")
-	mocks.client.EXPECT().GetEvent(gomock.Any()).
-		Return(&pmapi.Event{
-			EventID: "latestEventID",
-		}, nil).AnyTimes()
 
-	// We want to wait until first sync has finished.
+	// Call to get latest event ID and then to process first event.
-	firstSyncWaiter := sync.WaitGroup{}
+	mocks.client.EXPECT().GetEvent("").Return(&pmapi.Event{
-	firstSyncWaiter.Add(1)
+		EventID: "firstEventID",
-	mocks.client.EXPECT().
+	}, nil)
-		ListMessages(gomock.Any()).
+	mocks.client.EXPECT().GetEvent("firstEventID").Return(&pmapi.Event{
-		DoAndReturn(func(*pmapi.MessagesFilter) ([]*pmapi.Message, int, error) {
+		EventID: "latestEventID",
-			firstSyncWaiter.Done()
+	}, nil)
-			return []*pmapi.Message{}, 0, nil
+
-		})
+	mocks.client.EXPECT().ListMessages(gomock.Any()).Return(msgs, len(msgs), nil).AnyTimes()
+	for _, msg := range msgs {
+		mocks.client.EXPECT().GetMessage(msg.ID).Return(msg, nil).AnyTimes()
+	}
 
 	var err error
 	mocks.store, err = New(
@@ -128,6 +128,16 @@ func (mocks *mocksForStore) newStoreNoEvents(combinedMode bool) { //nolint[unpar
 	)
 	require.NoError(mocks.tb, err)
 
-	// Wait for sync to finish.
+	// We want to wait until first sync has finished.
-	firstSyncWaiter.Wait()
+	require.Eventually(mocks.tb, func() bool {
+		for _, msg := range msgs {
+			_, err := mocks.store.getMessageFromDB(msg.ID)
+			if err != nil {
+				// To see in test result the latest error for debugging.
+				fmt.Println("Sync wait error:", err)
+				return false
+			}
+		}
+		return true
+	}, 5*time.Second, 10*time.Millisecond)
 }

internal/transfer/provider_test.go

@@ -85,7 +85,7 @@ func testTransferFrom(t *testing.T, rules transferRules, provider TargetProvider
 		progress.finish()
 	}()
 
-	maxWait := time.Duration(len(messages)) * time.Second
+	maxWait := time.Duration(len(messages)) * 2 * time.Second
 	a.Eventually(t, func() bool {
 		return progress.updateCh == nil
 	}, maxWait, 10*time.Millisecond, "Waiting for imported messages timed out")

internal/users/user_test.go

@@ -20,8 +20,6 @@ package users
 import (
 	"testing"
 
-	"github.com/ProtonMail/proton-bridge/pkg/pmapi"
-	gomock "github.com/golang/mock/gomock"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -31,12 +29,7 @@ func testNewUser(m mocks) *User {
 	m.clientManager.EXPECT().GetClient("user").Return(m.pmapiClient).MinTimes(1)
 
 	mockConnectedUser(m)
-
+	mockEventLoopNoAction(m)
-	gomock.InOrder(
-		m.pmapiClient.EXPECT().GetEvent("").Return(testPMAPIEvent, nil).MaxTimes(1),
-		m.pmapiClient.EXPECT().GetEvent(testPMAPIEvent.EventID).Return(testPMAPIEvent, nil).MaxTimes(1),
-		m.pmapiClient.EXPECT().ListMessages(gomock.Any()).Return([]*pmapi.Message{}, 0, nil).MaxTimes(1),
-	)
 
 	user, err := newUser(m.PanicHandler, "user", m.eventListener, m.credentialsStore, m.clientManager, m.storeMaker)
 	assert.NoError(m.t, err)
@@ -53,12 +46,7 @@ func testNewUserForLogout(m mocks) *User {
 	m.clientManager.EXPECT().GetClient("user").Return(m.pmapiClient).MinTimes(1)
 
 	mockConnectedUser(m)
-
+	mockEventLoopNoAction(m)
-	gomock.InOrder(
-		m.pmapiClient.EXPECT().GetEvent("").Return(testPMAPIEvent, nil).MaxTimes(1),
-		m.pmapiClient.EXPECT().GetEvent(testPMAPIEvent.EventID).Return(testPMAPIEvent, nil).MaxTimes(1),
-		m.pmapiClient.EXPECT().ListMessages(gomock.Any()).Return([]*pmapi.Message{}, 0, nil).MaxTimes(1),
-	)
 
 	user, err := newUser(m.PanicHandler, "user", m.eventListener, m.credentialsStore, m.clientManager, m.storeMaker)
 	assert.NoError(m.t, err)

internal/users/users_new_test.go

@@ -23,7 +23,6 @@ import (
 
 	"github.com/ProtonMail/proton-bridge/internal/events"
 	"github.com/ProtonMail/proton-bridge/internal/users/credentials"
-	"github.com/ProtonMail/proton-bridge/pkg/pmapi"
 	gomock "github.com/golang/mock/gomock"
 	"github.com/stretchr/testify/assert"
 )
@@ -86,36 +85,6 @@ func TestNewUsersWithConnectedUserWithBadToken(t *testing.T) {
 	checkUsersNew(t, m, []*credentials.Credentials{testCredentialsDisconnected})
 }
 
-func mockConnectedUser(m mocks) {
-	gomock.InOrder(
-		m.credentialsStore.EXPECT().Get("user").Return(testCredentials, nil),
-
-		m.credentialsStore.EXPECT().Get("user").Return(testCredentials, nil),
-		m.pmapiClient.EXPECT().AuthRefresh("token").Return(testAuthRefresh, nil),
-
-		m.pmapiClient.EXPECT().Unlock([]byte(testCredentials.MailboxPassword)).Return(nil),
-
-		// Set up mocks for store initialisation for the authorized user.
-		m.pmapiClient.EXPECT().ListLabels().Return([]*pmapi.Label{}, nil),
-		m.pmapiClient.EXPECT().CountMessages("").Return([]*pmapi.MessagesCount{}, nil),
-		m.pmapiClient.EXPECT().Addresses().Return([]*pmapi.Address{testPMAPIAddress}),
-	)
-}
-
-// mockAuthUpdate simulates users calling UpdateAuthToken on the given user.
-// This would normally be done by users when it receives an auth from the ClientManager,
-// but as we don't have a full users instance here, we do this manually.
-func mockAuthUpdate(user *User, token string, m mocks) {
-	gomock.InOrder(
-		m.credentialsStore.EXPECT().UpdateToken("user", ":"+token).Return(nil),
-		m.credentialsStore.EXPECT().Get("user").Return(credentialsWithToken(token), nil),
-	)
-
-	user.updateAuthToken(refreshWithToken(token))
-
-	waitForEvents()
-}
-
 func TestNewUsersWithConnectedUser(t *testing.T) {
 	m := initMocks(t)
 	defer m.ctrl.Finish()

internal/users/users_test.go

@@ -284,10 +284,37 @@ func TestClearData(t *testing.T) {
 func mockEventLoopNoAction(m mocks) {
 	// Set up mocks for starting the store's event loop (in store.New).
 	// The event loop runs in another goroutine so this might happen at any time.
+	m.pmapiClient.EXPECT().GetEvent("").Return(testPMAPIEvent, nil).AnyTimes()
+	m.pmapiClient.EXPECT().GetEvent(testPMAPIEvent.EventID).Return(testPMAPIEvent, nil).AnyTimes()
+	m.pmapiClient.EXPECT().ListMessages(gomock.Any()).Return([]*pmapi.Message{}, 0, nil).AnyTimes()
+}
+
+func mockConnectedUser(m mocks) {
 	gomock.InOrder(
-		m.pmapiClient.EXPECT().GetEvent("").Return(testPMAPIEvent, nil),
+		m.credentialsStore.EXPECT().Get("user").Return(testCredentials, nil),
-		m.pmapiClient.EXPECT().GetEvent(testPMAPIEvent.EventID).Return(testPMAPIEvent, nil),
+
-		// Set up mocks for performing the initial store sync.
+		m.credentialsStore.EXPECT().Get("user").Return(testCredentials, nil),
-		m.pmapiClient.EXPECT().ListMessages(gomock.Any()).Return([]*pmapi.Message{}, 0, nil),
+		m.pmapiClient.EXPECT().AuthRefresh("token").Return(testAuthRefresh, nil),
+
+		m.pmapiClient.EXPECT().Unlock([]byte(testCredentials.MailboxPassword)).Return(nil),
+
+		// Set up mocks for store initialisation for the authorized user.
+		m.pmapiClient.EXPECT().ListLabels().Return([]*pmapi.Label{}, nil),
+		m.pmapiClient.EXPECT().CountMessages("").Return([]*pmapi.MessagesCount{}, nil),
+		m.pmapiClient.EXPECT().Addresses().Return([]*pmapi.Address{testPMAPIAddress}),
 	)
 }
+
+// mockAuthUpdate simulates users calling UpdateAuthToken on the given user.
+// This would normally be done by users when it receives an auth from the ClientManager,
+// but as we don't have a full users instance here, we do this manually.
+func mockAuthUpdate(user *User, token string, m mocks) {
+	gomock.InOrder(
+		m.credentialsStore.EXPECT().UpdateToken("user", ":"+token).Return(nil),
+		m.credentialsStore.EXPECT().Get("user").Return(credentialsWithToken(token), nil),
+	)
+
+	user.updateAuthToken(refreshWithToken(token))
+
+	waitForEvents()
+}

pkg/pmapi/check_connection_test.go

@@ -43,7 +43,7 @@ func startServer() {
 		_, _ = w.Write([]byte("OK"))
 	})
 	http.HandleFunc("/timeout", func(w http.ResponseWriter, r *http.Request) {
-		time.Sleep(10 * time.Second)
+		time.Sleep(testRequestTimeout + time.Second) // Add extra second to be sure it will timeout.
 		w.WriteHeader(http.StatusOK)
 		_, _ = w.Write([]byte("OK"))
 	})

unreleased.md

@@ -15,3 +15,4 @@ Changelog [format](http://keepachangelog.com/en/1.0.0/)
 
 ### Fixed
 * GODT-135 Support parameters in SMTP `FROM MAIL` command, such as `BODY=7BIT`, or empty value `FROM MAIL:<>` used by some clients.
+* GODT-338 GODT-781 GODT-857 GODT-866 Flaky tests.