Random test failures because of work item creation error (#1676)

Move the `work item number sequence` in its own subpackage, and
create its dedicated repository with 2 methods: `Create` and `NextVal`.
Use a new `ID` column for the primary key, so GORM will issue a proper
`INSERT` query when a new number sequence is created.
Make sure a space has a sequence initialized upon its own creation

Other minor fixes in tests (eg: length of area name, etc.)

Fixes #1676

Signed-off-by: Xavier Coulon <xcoulon@redhat.com>

controller/workitem_blackbox_test.go

@@ -782,12 +782,12 @@ func createOneRandomIteration(ctx context.Context, db *gorm.DB) *iteration.Itera
 	return &itr
 }
 
-func createOneRandomArea(ctx context.Context, db *gorm.DB, testName string) *area.Area {
+func createOneRandomArea(ctx context.Context, db *gorm.DB) *area.Area {
 	areaRepo := area.NewAreaRepository(db)
 	spaceRepo := space.NewRepository(db)
 
 	newSpace := space.Space{
-		Name: fmt.Sprintf("Space area %v %v", testName, uuid.NewV4()),
+		Name: fmt.Sprintf("Space area %v", uuid.NewV4()),
 	}
 	space, err := spaceRepo.Create(ctx, &newSpace)
 	if err != nil {
@@ -1491,7 +1491,7 @@ func (s *WorkItem2Suite) TestWI2ListByStateFilterOKModifiedUsingIfNoneMatchIfMod
 }
 
 func (s *WorkItem2Suite) setupAreaWorkItem(createWorkItem bool) (uuid.UUID, string, *app.WorkItemSingle) {
-	tempArea := createOneRandomArea(s.svc.Context, s.DB, "TestWI2ListByAreaFilter")
+	tempArea := createOneRandomArea(s.svc.Context, s.DB)
 	require.NotNil(s.T(), tempArea)
 	areaID := tempArea.ID.String()
 	c := minimumRequiredCreatePayload()

migration/migration.go

@@ -352,6 +352,9 @@ func GetMigrations() Migrations {
 	// Version 76
 	m = append(m, steps{ExecuteSQLFile("076-drop-space-resources-and-oauth-state.sql")})
 
+	// Version 77
+	m = append(m, steps{ExecuteSQLFile("077-work_item_number_sequence.sql")})
+
 	// Version N
 	//
 	// In order to add an upgrade, simply append an array of MigrationFunc to the

migration/sql-files/077-work_item_number_sequence.sql

@@ -0,0 +1,5 @@
+-- modify the `work_item_number_sequences` to include an independant ID column for the primary key
+alter table work_item_number_sequences drop constraint work_item_number_sequences_pkey;
+alter table work_item_number_sequences add column  id uuid primary key DEFAULT uuid_generate_v4() NOT NULL;
+
+

search/search_repository_whitebox_test.go

@@ -11,16 +11,14 @@ import (
 	"testing"
 
 	"github.com/fabric8-services/fabric8-wit/gormtestsupport"
-	"github.com/fabric8-services/fabric8-wit/models"
 	"github.com/fabric8-services/fabric8-wit/rendering"
 	"github.com/fabric8-services/fabric8-wit/resource"
 	"github.com/fabric8-services/fabric8-wit/space"
 	testsupport "github.com/fabric8-services/fabric8-wit/test"
+	tf "github.com/fabric8-services/fabric8-wit/test/testfixture"
 	"github.com/fabric8-services/fabric8-wit/workitem"
 	"github.com/goadesign/goa"
-	"github.com/jinzhu/gorm"
 	_ "github.com/lib/pq"
-	"github.com/pkg/errors"
 	uuid "github.com/satori/go.uuid"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -52,7 +50,6 @@ type SearchTestDescriptor struct {
 
 func (s *searchRepositoryWhiteboxTest) TestSearchByText() {
 	wir := workitem.NewWorkItemRepository(s.DB)
-
 	testDataSet := []SearchTestDescriptor{
 		{
 			wi: workitem.WorkItem{
@@ -144,90 +141,82 @@ func (s *searchRepositoryWhiteboxTest) TestSearchByText() {
 			minimumResults: 1,
 		},
 	}
+	// create the parent space
+	fxt := tf.NewTestFixture(s.T(), s.DB, tf.Spaces(1))
+	//
+	for _, testData := range testDataSet {
+		workItem := testData.wi
+		searchString := testData.searchString
+		minimumResults := testData.minimumResults
+		workItemURLInSearchString := "http://demo.almighty.io/work-item/list/detail/"
+		req := &http.Request{Host: "localhost"}
+		params := url.Values{}
+		ctx := goa.NewContext(context.Background(), nil, req, params)
+		createdWorkItem, err := wir.Create(ctx, fxt.Spaces[0].ID, workitem.SystemBug, workItem.Fields, s.modifierID)
+		require.Nil(s.T(), err, "failed to create test data")
+
+		// create the URL and use it in the search string
+		workItemURLInSearchString = workItemURLInSearchString + strconv.Itoa(createdWorkItem.Number)
+
+		// had to dynamically create this since I didn't now the URL/ID of the workitem
+		// till the test data was created.
+		searchString = searchString + workItemURLInSearchString
+		searchString = fmt.Sprintf("\"%s\"", searchString)
+		s.T().Log("using search string: " + searchString)
+		sr := NewGormSearchRepository(s.DB)
+		var start, limit int = 0, 100
+		spaceID := fxt.Spaces[0].ID.String()
+		workItemList, _, err := sr.SearchFullText(ctx, searchString, &start, &limit, &spaceID)
+		require.Nil(s.T(), err, "failed to get search result")
+		searchString = strings.Trim(searchString, "\"")
+		// Since this test adds test data, whether or not other workitems exist
+		// there must be at least 1 search result returned.
+		if len(workItemList) == minimumResults && minimumResults == 0 {
+			// no point checking further, we got what we wanted.
+			continue
+		} else if len(workItemList) < minimumResults {
+			s.T().Fatalf("At least %d search result(s) was|were expected ", minimumResults)
+		}
 
-	models.Transactional(s.DB, func(tx *gorm.DB) error {
-
-		for _, testData := range testDataSet {
-			workItem := testData.wi
-			searchString := testData.searchString
-			minimumResults := testData.minimumResults
-			workItemURLInSearchString := "http://demo.almighty.io/work-item/list/detail/"
-			req := &http.Request{Host: "localhost"}
-			params := url.Values{}
-			ctx := goa.NewContext(context.Background(), nil, req, params)
-
-			createdWorkItem, err := wir.Create(ctx, space.SystemSpace, workitem.SystemBug, workItem.Fields, s.modifierID)
-			require.Nil(s.T(), err, "failed to create test data")
-
-			// create the URL and use it in the search string
-			workItemURLInSearchString = workItemURLInSearchString + strconv.Itoa(createdWorkItem.Number)
-
-			// had to dynamically create this since I didn't now the URL/ID of the workitem
-			// till the test data was created.
-			searchString = searchString + workItemURLInSearchString
-			searchString = fmt.Sprintf("\"%s\"", searchString)
-			s.T().Log("using search string: " + searchString)
-			sr := NewGormSearchRepository(tx)
-			var start, limit int = 0, 100
-			workItemList, _, err := sr.SearchFullText(ctx, searchString, &start, &limit, nil)
-			require.Nil(s.T(), err, "failed to get search result")
-			searchString = strings.Trim(searchString, "\"")
-			// Since this test adds test data, whether or not other workitems exist
-			// there must be at least 1 search result returned.
-			if len(workItemList) == minimumResults && minimumResults == 0 {
-				// no point checking further, we got what we wanted.
-				continue
-			} else if len(workItemList) < minimumResults {
-				s.T().Fatalf("At least %d search result(s) was|were expected ", minimumResults)
-			}
+		// These keywords need a match in the textual part.
+		allKeywords := strings.Fields(searchString)
+		allKeywords = append(allKeywords, strconv.Itoa(createdWorkItem.Number))
+		//[]string{workItemURLInSearchString, createdWorkItem.ID, `"Sbose"`, `"deScription"`, `'12345678asdfgh'`}
+
+		// These keywords need a match optionally either as URL string or ID
+		optionalKeywords := []string{workItemURLInSearchString, strconv.Itoa(createdWorkItem.Number)}
+
+		// We will now check the legitimacy of the search results.
+		// Iterate through all search results and see whether they meet the criteria
 
-			// These keywords need a match in the textual part.
-			allKeywords := strings.Fields(searchString)
-			allKeywords = append(allKeywords, strconv.Itoa(createdWorkItem.Number))
-			//[]string{workItemURLInSearchString, createdWorkItem.ID, `"Sbose"`, `"deScription"`, `'12345678asdfgh'`}
-
-			// These keywords need a match optionally either as URL string or ID
-			optionalKeywords := []string{workItemURLInSearchString, strconv.Itoa(createdWorkItem.Number)}
-
-			// We will now check the legitimacy of the search results.
-			// Iterate through all search results and see whether they meet the criteria
-
-			for _, workItemValue := range workItemList {
-				s.T().Log("Found search result  ", workItemValue.ID)
-
-				for _, keyWord := range allKeywords {
-
-					workItemTitle := ""
-					if workItemValue.Fields[workitem.SystemTitle] != nil {
-						workItemTitle = strings.ToLower(workItemValue.Fields[workitem.SystemTitle].(string))
-					}
-					workItemDescription := ""
-					if workItemValue.Fields[workitem.SystemDescription] != nil {
-						descriptionField := workItemValue.Fields[workitem.SystemDescription].(rendering.MarkupContent)
-						workItemDescription = strings.ToLower(descriptionField.Content)
-					}
-					workItemNumber := 0
-					if workItemValue.Fields[workitem.SystemNumber] != nil {
-						workItemNumber = workItemValue.Fields[workitem.SystemNumber].(int)
-					}
-					keyWord = strings.ToLower(keyWord)
-
-					if strings.Contains(workItemTitle, keyWord) || strings.Contains(workItemDescription, keyWord) {
-						// Check if the search keyword is present as text in the title/description
-						s.T().Logf("Found keyword %s in workitem %s", keyWord, workItemValue.ID)
-					} else if stringInSlice(keyWord, optionalKeywords) && strings.Contains(keyWord, strconv.Itoa(workItemValue.Number)) {
-						// If not present in title/description then it should be a URL or ID
-						s.T().Logf("Found keyword '%s' as number '%s' from the URL", keyWord, strconv.Itoa(workItemValue.Number))
-					} else {
-						s.T().Errorf("'%s' neither found in title '%s' nor in the description: '%s' for workitem number %d", keyWord, workItemTitle, workItemDescription, workItemNumber)
-					}
+		for _, workItemValue := range workItemList {
+			s.T().Log("Found search result  ", workItemValue.ID)
+
+			for _, keyWord := range allKeywords {
+
+				workItemTitle := ""
+				if workItemValue.Fields[workitem.SystemTitle] != nil {
+					workItemTitle = strings.ToLower(workItemValue.Fields[workitem.SystemTitle].(string))
+				}
+				workItemDescription := ""
+				if workItemValue.Fields[workitem.SystemDescription] != nil {
+					descriptionField := workItemValue.Fields[workitem.SystemDescription].(rendering.MarkupContent)
+					workItemDescription = strings.ToLower(descriptionField.Content)
+				}
+				keyWord = strings.ToLower(keyWord)
+				if strings.Contains(workItemTitle, keyWord) || strings.Contains(workItemDescription, keyWord) {
+					// Check if the search keyword is present as text in the title/description
+					s.T().Logf("Found keyword %s in workitem %s", keyWord, workItemValue.ID)
+				} else if stringInSlice(keyWord, optionalKeywords) && strings.Contains(keyWord, strconv.Itoa(workItemValue.Number)) {
+					// If not present in title/description then it should be a URL or ID
+					s.T().Logf("Found keyword '%s' as number '%s' from the URL", keyWord, strconv.Itoa(workItemValue.Number))
+				} else {
+					s.T().Errorf("'%s' neither found in title '%s' nor in the description: '%s' for workitem number %d", keyWord, workItemTitle, workItemDescription, workItemValue.Number)
 				}
 			}
-
 		}
-		return nil
 
-	})
+	}
 }
 
 func stringInSlice(str string, list []string) bool {
@@ -240,55 +229,51 @@ func stringInSlice(str string, list []string) bool {
 }
 
 func (s *searchRepositoryWhiteboxTest) TestSearchByID() {
+	req := &http.Request{Host: "localhost"}
+	params := url.Values{}
+	ctx := goa.NewContext(context.Background(), nil, req, params)
 
-	models.Transactional(s.DB, func(tx *gorm.DB) error {
-		req := &http.Request{Host: "localhost"}
-		params := url.Values{}
-		ctx := goa.NewContext(context.Background(), nil, req, params)
-
-		wir := workitem.NewWorkItemRepository(tx)
+	wir := workitem.NewWorkItemRepository(s.DB)
 
-		workItem := workitem.WorkItem{Fields: make(map[string]interface{})}
+	workItem := workitem.WorkItem{Fields: make(map[string]interface{})}
 
-		workItem.Fields = map[string]interface{}{
-			workitem.SystemTitle:       "Search Test Sbose",
-			workitem.SystemDescription: rendering.NewMarkupContentFromLegacy("Description"),
-			workitem.SystemCreator:     "sbose78",
-			workitem.SystemAssignees:   []string{"pranav"},
-			workitem.SystemState:       "closed",
-		}
+	workItem.Fields = map[string]interface{}{
+		workitem.SystemTitle:       "Search Test Sbose",
+		workitem.SystemDescription: rendering.NewMarkupContentFromLegacy("Description"),
+		workitem.SystemCreator:     "sbose78",
+		workitem.SystemAssignees:   []string{"pranav"},
+		workitem.SystemState:       "closed",
+	}
 
-		createdWorkItem, err := wir.Create(ctx, space.SystemSpace, workitem.SystemBug, workItem.Fields, s.modifierID)
-		if err != nil {
-			s.T().Fatalf("Couldn't create test data: %+v", err)
-		}
+	createdWorkItem, err := wir.Create(ctx, space.SystemSpace, workitem.SystemBug, workItem.Fields, s.modifierID)
+	if err != nil {
+		s.T().Fatalf("Couldn't create test data: %+v", err)
+	}
 
-		// Create a new workitem to have the ID in it's title. This should not come
-		// up in search results
+	// Create a new workitem to have the ID in it's title. This should not come
+	// up in search results
 
-		workItem.Fields[workitem.SystemTitle] = "Search test sbose " + createdWorkItem.ID.String()
-		_, err = wir.Create(ctx, space.SystemSpace, workitem.SystemBug, workItem.Fields, s.modifierID)
-		if err != nil {
-			s.T().Fatalf("Couldn't create test data: %+v", err)
-		}
+	workItem.Fields[workitem.SystemTitle] = "Search test sbose " + createdWorkItem.ID.String()
+	_, err = wir.Create(ctx, space.SystemSpace, workitem.SystemBug, workItem.Fields, s.modifierID)
+	if err != nil {
+		s.T().Fatalf("Couldn't create test data: %+v", err)
+	}
 
-		sr := NewGormSearchRepository(tx)
+	sr := NewGormSearchRepository(s.DB)
 
-		var start, limit int = 0, 100
-		searchString := "number:" + strconv.Itoa(createdWorkItem.Number)
-		workItemList, _, err := sr.SearchFullText(ctx, searchString, &start, &limit, nil)
-		if err != nil {
-			s.T().Fatal("Error gettig search result ", err)
-		}
+	var start, limit int = 0, 100
+	searchString := "number:" + strconv.Itoa(createdWorkItem.Number)
+	workItemList, _, err := sr.SearchFullText(ctx, searchString, &start, &limit, nil)
+	if err != nil {
+		s.T().Fatal("Error gettig search result ", err)
+	}
 
-		// ID is unique, hence search result set's length should be 1
-		assert.Equal(s.T(), len(workItemList), 1)
-		for _, workItemValue := range workItemList {
-			s.T().Log("Found search result for ID Search ", workItemValue.ID)
-			assert.Equal(s.T(), createdWorkItem.ID, workItemValue.ID)
-		}
-		return errors.WithStack(err)
-	})
+	// ID is unique, hence search result set's length should be 1
+	assert.Equal(s.T(), len(workItemList), 1)
+	for _, workItemValue := range workItemList {
+		s.T().Log("Found search result for ID Search ", workItemValue.ID)
+		assert.Equal(s.T(), createdWorkItem.ID, workItemValue.ID)
+	}
 }
 
 func TestGenerateSQLSearchStringText(t *testing.T) {

space/space.go

@@ -11,7 +11,7 @@ import (
 	"github.com/fabric8-services/fabric8-wit/errors"
 	"github.com/fabric8-services/fabric8-wit/gormsupport"
 	"github.com/fabric8-services/fabric8-wit/log"
-	workitemnumber "github.com/fabric8-services/fabric8-wit/workitem/number"
+	numbersequence "github.com/fabric8-services/fabric8-wit/workitem/number_sequence"
 
 	"github.com/goadesign/goa"
 	"github.com/jinzhu/gorm"
@@ -94,12 +94,16 @@ type Repository interface {
 
 // NewRepository creates a new space repo
 func NewRepository(db *gorm.DB) *GormRepository {
-	return &GormRepository{db}
+	return &GormRepository{
+		db:   db,
+		winr: numbersequence.NewWorkItemNumberSequenceRepository(db),
+	}
 }
 
 // GormRepository implements SpaceRepository using gorm
 type GormRepository struct {
-	db *gorm.DB
+	db   *gorm.DB
+	winr numbersequence.WorkItemNumberSequenceRepository
 }
 
 // Load returns the space for the given id
@@ -227,11 +231,10 @@ func (r *GormRepository) Create(ctx context.Context, space *Space) (*Space, erro
 		return nil, errors.NewInternalError(ctx, err)
 	}
 	// also, initialize the work_item_number_sequence table for this space
-	numberSequence := workitemnumber.WorkItemNumberSequence{CurrentVal: 0, SpaceID: space.ID}
-	if err := r.db.Save(&numberSequence).Error; err != nil {
-		return nil, errs.Wrapf(err, "failed to create work item with sequence number: `%s`", numberSequence.String())
+	_, err := r.winr.Create(ctx, space.ID)
+	if err != nil {
+		return nil, errors.NewInternalError(ctx, err)
 	}
-
 	log.Info(ctx, map[string]interface{}{
 		"space_id": space.ID,
 	}, "Space created successfully")

workitem/number/workitem_number.go → workitem/number_sequence/workitem_number_sequence.go

@@ -1,4 +1,4 @@
-package number
+package numbersequence
 
 import (
 	"fmt"
@@ -8,7 +8,8 @@ import (
 
 // WorkItemNumberSequence the sequence for work item numbers in a space
 type WorkItemNumberSequence struct {
-	SpaceID    uuid.UUID `sql:"type:uuid" gorm:"primary_key"`
+	ID         uuid.UUID `sql:"type:uuid" gorm:"primary_key"`
+	SpaceID    uuid.UUID `sql:"type:uuid"`
 	CurrentVal int
 }