From 27d530dd0db67235506260132fa3096a5c2f5057 Mon Sep 17 00:00:00 2001 From: Xavier Coulon Date: Mon, 2 Oct 2017 12:56:23 +0200 Subject: [PATCH 01/14] Stabilize/disable tests related to if_modified_since_header (#1655) Using the `Last-Modified` response header when possible Also, making sure that the list of spaces is always ordered by `updated_at DESC` to get the most recents spaces first. Signed-off-by: Xavier Coulon --- controller/search_spaces_blackbox_test.go | 25 ++++++++++--------- controller/space_blackbox_test.go | 4 ++- .../work_item_children_blackbox_test.go | 6 +++-- space/space.go | 2 ++ 4 files changed, 22 insertions(+), 15 deletions(-) diff --git a/controller/search_spaces_blackbox_test.go b/controller/search_spaces_blackbox_test.go index eaa7ed935a..c65a5fe581 100644 --- a/controller/search_spaces_blackbox_test.go +++ b/controller/search_spaces_blackbox_test.go @@ -6,6 +6,7 @@ import ( "strconv" "strings" "testing" + "time" "github.com/fabric8-services/fabric8-wit/app" "github.com/fabric8-services/fabric8-wit/app/test" @@ -42,7 +43,6 @@ type okScenario struct { type TestSearchSpacesREST struct { gormtestsupport.DBTestSuite - db *gormapplication.GormDB clean func() } @@ -73,18 +73,19 @@ func (rest *TestSearchSpacesREST) UnSecuredController() (*goa.Service, *SearchCo func (rest *TestSearchSpacesREST) TestSpacesSearchOK() { // given - idents, err := createTestData(rest.db) + prefix := time.Now().Format("2006_Jan_2_15_04_05_") // using a unique prefix to make sure the test data will not collide with existing, older spaces. + idents, err := createTestData(rest.db, prefix) require.Nil(rest.T(), err) tests := []okScenario{ - {"With uppercase fullname query", args{offset("0"), limit(10), "TEST_AB"}, expects{totalCount(1)}}, - {"With lowercase fullname query", args{offset("0"), limit(10), "TEST_AB"}, expects{totalCount(1)}}, - {"With uppercase description query", args{offset("0"), limit(10), "DESCRIPTION FOR TEST_AB"}, expects{totalCount(1)}}, - {"With lowercase description query", args{offset("0"), limit(10), "description for test_ab"}, expects{totalCount(1)}}, + {"With uppercase fullname query", args{offset("0"), limit(10), prefix + "TEST_AB"}, expects{totalCount(1)}}, + {"With lowercase fullname query", args{offset("0"), limit(10), prefix + "TEST_AB"}, expects{totalCount(1)}}, + {"With uppercase description query", args{offset("0"), limit(10), "DESCRIPTION FOR " + prefix + "TEST_AB"}, expects{totalCount(1)}}, + {"With lowercase description query", args{offset("0"), limit(10), "description for " + prefix + "test_ab"}, expects{totalCount(1)}}, {"with special chars", args{offset("0"), limit(10), "&:\n!#%?*"}, expects{totalCount(0)}}, {"with * to list all", args{offset("0"), limit(10), "*"}, expects{totalCountAtLeast(len(idents))}}, - {"with multi page", args{offset("0"), limit(10), "TEST"}, expects{hasLinks("Next")}}, - {"with last page", args{offset(strconv.Itoa(len(idents) - 1)), limit(10), "TEST"}, expects{hasNoLinks("Next"), hasLinks("Prev")}}, - {"with different values", args{offset("0"), limit(10), "TEST"}, expects{differentValues()}}, + {"with multi page", args{offset("0"), limit(10), prefix + "TEST"}, expects{hasLinks("Next")}}, + {"with last page", args{offset(strconv.Itoa(len(idents) - 1)), limit(10), prefix + "TEST"}, expects{hasNoLinks("Next"), hasLinks("Prev")}}, + {"with different values", args{offset("0"), limit(10), prefix + "TEST"}, expects{differentValues()}}, } svc, ctrl := rest.UnSecuredController() // when/then @@ -96,10 +97,10 @@ func (rest *TestSearchSpacesREST) TestSpacesSearchOK() { } } -func createTestData(db application.DB) ([]space.Space, error) { - names := []string{"TEST_A", "TEST_AB", "TEST_B", "TEST_C"} +func createTestData(db application.DB, prefix string) ([]space.Space, error) { + names := []string{prefix + "TEST_A", prefix + "TEST_AB", prefix + "TEST_B", prefix + "TEST_C"} for i := 0; i < 20; i++ { - names = append(names, "TEST_"+strconv.Itoa(i)) + names = append(names, prefix+"TEST_"+strconv.Itoa(i)) } spaces := []space.Space{} diff --git a/controller/space_blackbox_test.go b/controller/space_blackbox_test.go index 9d8f5cc86e..13c441a6ab 100644 --- a/controller/space_blackbox_test.go +++ b/controller/space_blackbox_test.go @@ -550,9 +550,11 @@ func (rest *TestSpaceREST) TestListSpacesOKUsingExpiredIfModifiedSinceHeader() { p := minimumRequiredCreateSpace() p.Data.Attributes.Name = &name svc, ctrl := rest.SecuredController(testsupport.TestIdentity) - test.CreateSpaceCreated(rest.T(), svc.Context, svc, ctrl, p) + _, createdSpace := test.CreateSpaceCreated(rest.T(), svc.Context, svc, ctrl, p) // when ifModifiedSince := app.ToHTTPTime(time.Now().Add(-1 * time.Hour)) + rest.T().Logf("space created at=%s", createdSpace.Data.Attributes.CreatedAt.UTC().String()) + rest.T().Logf("requesting with `If-Modified-Since`=%s", ifModifiedSince) _, list := test.ListSpaceOK(rest.T(), svc.Context, svc, ctrl, nil, nil, &ifModifiedSince, nil) // then require.NotNil(rest.T(), list) diff --git a/controller/work_item_children_blackbox_test.go b/controller/work_item_children_blackbox_test.go index c171fe94a2..5fc3c4aa83 100644 --- a/controller/work_item_children_blackbox_test.go +++ b/controller/work_item_children_blackbox_test.go @@ -276,9 +276,11 @@ func (s *workItemChildSuite) TestChildren() { assertResponseHeaders(t, res) }) s.T().Run("not modified using if modified since header", func(t *testing.T) { + // given + res, _ := test.ListChildrenWorkitemOK(t, s.svc.Context, s.svc, s.workItemCtrl, *s.bug1.Data.ID, nil, nil, nil, nil) + ifModifiedSince := res.Header()[app.LastModified][0] // when - ifModifiedSince := app.ToHTTPTime(s.bug3.Data.Attributes[workitem.SystemUpdatedAt].(time.Time)) - res := test.ListChildrenWorkitemNotModified(t, s.svc.Context, s.svc, s.workItemCtrl, *s.bug1.Data.ID, nil, nil, &ifModifiedSince, nil) + res = test.ListChildrenWorkitemNotModified(t, s.svc.Context, s.svc, s.workItemCtrl, *s.bug1.Data.ID, nil, nil, &ifModifiedSince, nil) // then assertResponseHeaders(t, res) }) diff --git a/space/space.go b/space/space.go index 192168d708..8441e06c75 100644 --- a/space/space.go +++ b/space/space.go @@ -258,6 +258,8 @@ func (r *GormRepository) listSpaceFromDB(ctx context.Context, q *string, userID db = db.Where("spaces.owner_id=?", userID) } + // ensure that the result list is always ordered in the same manner + db = db.Order("spaces.updated_at DESC") rows, err := db.Rows() if err != nil { return nil, 0, errs.WithStack(err) From 3481ff7d629626d6aa0dc7365f60f4f2c47a8829 Mon Sep 17 00:00:00 2001 From: Xavier Coulon Date: Mon, 2 Oct 2017 14:24:03 +0200 Subject: [PATCH 02/14] 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 --- controller/workitem_blackbox_test.go | 6 +- migration/migration.go | 3 + .../077-work_item_number_sequence.sql | 5 + search/search_repository_whitebox_test.go | 229 ++++++++---------- space/space.go | 15 +- .../workitem_number_sequence.go} | 11 +- .../workitem_number_sequence_repository.go | 56 +++++ workitem/workitem_repository.go | 29 ++- 8 files changed, 209 insertions(+), 145 deletions(-) create mode 100644 migration/sql-files/077-work_item_number_sequence.sql rename workitem/{workitem_number.go => number_sequence/workitem_number_sequence.go} (58%) create mode 100644 workitem/number_sequence/workitem_number_sequence_repository.go diff --git a/controller/workitem_blackbox_test.go b/controller/workitem_blackbox_test.go index 0272aa9898..48d25c47e1 100644 --- a/controller/workitem_blackbox_test.go +++ b/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() diff --git a/migration/migration.go b/migration/migration.go index 7ad5f8267d..12114dbd5e 100644 --- a/migration/migration.go +++ b/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 diff --git a/migration/sql-files/077-work_item_number_sequence.sql b/migration/sql-files/077-work_item_number_sequence.sql new file mode 100644 index 0000000000..fb41b2879b --- /dev/null +++ b/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; + + diff --git a/search/search_repository_whitebox_test.go b/search/search_repository_whitebox_test.go index 879c663fe8..a96e17aab4 100644 --- a/search/search_repository_whitebox_test.go +++ b/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) { diff --git a/space/space.go b/space/space.go index 8441e06c75..c2b84383f9 100644 --- a/space/space.go +++ b/space/space.go @@ -11,6 +11,7 @@ import ( "github.com/fabric8-services/fabric8-wit/errors" "github.com/fabric8-services/fabric8-wit/gormsupport" "github.com/fabric8-services/fabric8-wit/log" + numbersequence "github.com/fabric8-services/fabric8-wit/workitem/number_sequence" "github.com/goadesign/goa" "github.com/jinzhu/gorm" @@ -93,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 @@ -225,7 +230,11 @@ 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 + _, 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") diff --git a/workitem/workitem_number.go b/workitem/number_sequence/workitem_number_sequence.go similarity index 58% rename from workitem/workitem_number.go rename to workitem/number_sequence/workitem_number_sequence.go index fd686163a0..5a062e905f 100644 --- a/workitem/workitem_number.go +++ b/workitem/number_sequence/workitem_number_sequence.go @@ -1,12 +1,15 @@ -package workitem +package numbersequence import ( + "fmt" + uuid "github.com/satori/go.uuid" ) // 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 } @@ -18,3 +21,7 @@ const ( func (w WorkItemNumberSequence) TableName() string { return workitemNumberTableName } + +func (w *WorkItemNumberSequence) String() string { + return fmt.Sprintf("SpaceId=%s Number=%d", w.SpaceID.String(), w.CurrentVal) +} diff --git a/workitem/number_sequence/workitem_number_sequence_repository.go b/workitem/number_sequence/workitem_number_sequence_repository.go new file mode 100644 index 0000000000..b08b57b305 --- /dev/null +++ b/workitem/number_sequence/workitem_number_sequence_repository.go @@ -0,0 +1,56 @@ +package numbersequence + +import ( + "context" + + "github.com/fabric8-services/fabric8-wit/log" + "github.com/jinzhu/gorm" + errs "github.com/pkg/errors" + uuid "github.com/satori/go.uuid" +) + +// WorkItemNumberSequenceRepository the interface for the work item number sequence repository +type WorkItemNumberSequenceRepository interface { + Create(ctx context.Context, spaceID uuid.UUID) (*WorkItemNumberSequence, error) + NextVal(ctx context.Context, spaceID uuid.UUID) (*WorkItemNumberSequence, error) +} + +// NewWorkItemNumberSequenceRepository creates a GormWorkItemNumberSequenceRepository +func NewWorkItemNumberSequenceRepository(db *gorm.DB) *GormWorkItemNumberSequenceRepository { + repository := &GormWorkItemNumberSequenceRepository{db} + return repository +} + +// GormWorkItemNumberSequenceRepository implements WorkItemNumberSequenceRepository using gorm +type GormWorkItemNumberSequenceRepository struct { + db *gorm.DB +} + +// Create returns the next work item sequence number for the given space ID. Creates an entry in the DB if none was found before +func (r *GormWorkItemNumberSequenceRepository) Create(ctx context.Context, spaceID uuid.UUID) (*WorkItemNumberSequence, error) { + // retrieve the current issue number in the given space + numberSequence := WorkItemNumberSequence{SpaceID: spaceID, CurrentVal: 0} + 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()) + } + log.Warn(nil, map[string]interface{}{"Sequence": numberSequence.String()}, "Creating sequence") + return &numberSequence, nil +} + +// NextVal returns the next work item sequence number for the given space ID. Creates an entry in the DB if none was found before +func (r *GormWorkItemNumberSequenceRepository) NextVal(ctx context.Context, spaceID uuid.UUID) (*WorkItemNumberSequence, error) { + // retrieve the current issue number in the given space + numberSequence := WorkItemNumberSequence{} + tx := r.db.Model(&WorkItemNumberSequence{}).Set("gorm:query_option", "FOR UPDATE").Where("space_id = ?", spaceID).First(&numberSequence) + if tx.RecordNotFound() { + numberSequence.SpaceID = spaceID + numberSequence.CurrentVal = 1 + } else { + numberSequence.CurrentVal++ + } + if err := r.db.Save(&numberSequence).Error; err != nil { + return nil, errs.Wrapf(err, "failed to update work item with sequence number: `%s`", numberSequence.String()) + } + log.Warn(nil, map[string]interface{}{"Sequence": numberSequence.String()}, "computing nextVal") + return &numberSequence, nil +} diff --git a/workitem/workitem_repository.go b/workitem/workitem_repository.go index dfdeaa3582..ed17077efc 100644 --- a/workitem/workitem_repository.go +++ b/workitem/workitem_repository.go @@ -15,6 +15,7 @@ import ( "github.com/fabric8-services/fabric8-wit/log" "github.com/fabric8-services/fabric8-wit/rendering" "github.com/fabric8-services/fabric8-wit/space" + numbersequence "github.com/fabric8-services/fabric8-wit/workitem/number_sequence" "github.com/goadesign/goa" "github.com/jinzhu/gorm" @@ -53,13 +54,19 @@ type WorkItemRepository interface { // NewWorkItemRepository creates a GormWorkItemRepository func NewWorkItemRepository(db *gorm.DB) *GormWorkItemRepository { - repository := &GormWorkItemRepository{db, &GormWorkItemTypeRepository{db}, &GormRevisionRepository{db}} + repository := &GormWorkItemRepository{ + db, + numbersequence.NewWorkItemNumberSequenceRepository(db), + &GormWorkItemTypeRepository{db}, + &GormRevisionRepository{db}, + } return repository } // GormWorkItemRepository implements WorkItemRepository using gorm type GormWorkItemRepository struct { db *gorm.DB + winr *numbersequence.GormWorkItemNumberSequenceRepository witr *GormWorkItemTypeRepository wirr *GormRevisionRepository } @@ -551,18 +558,6 @@ func (r *GormWorkItemRepository) Create(ctx context.Context, spaceID uuid.UUID, if err != nil { return nil, errors.NewBadParameterError("typeID", typeID) } - // retrieve the current issue number in the given space - numberSequence := WorkItemNumberSequence{} - tx := r.db.Model(&WorkItemNumberSequence{}).Set("gorm:query_option", "FOR UPDATE").Where("space_id = ?", spaceID).First(&numberSequence) - if tx.RecordNotFound() { - numberSequence.SpaceID = spaceID - numberSequence.CurrentVal = 1 - } else { - numberSequence.CurrentVal++ - } - if err = r.db.Save(&numberSequence).Error; err != nil { - return nil, errs.Wrapf(err, "failed to create work item") - } // The order of workitems are spaced by a factor of 1000. pos, err := r.LoadHighestOrder(ctx, spaceID) @@ -570,12 +565,16 @@ func (r *GormWorkItemRepository) Create(ctx context.Context, spaceID uuid.UUID, return nil, errors.NewInternalError(ctx, err) } pos = pos + orderValue + wiNumberSequence, err := r.winr.NextVal(ctx, spaceID) + if err != nil { + return nil, errors.NewInternalError(ctx, err) + } wi := WorkItemStorage{ Type: typeID, Fields: Fields{}, ExecutionOrder: pos, SpaceID: spaceID, - Number: numberSequence.CurrentVal, + Number: wiNumberSequence.CurrentVal, } fields[SystemCreator] = creatorID.String() for fieldName, fieldDef := range wiType.Fields { @@ -608,7 +607,7 @@ func (r *GormWorkItemRepository) Create(ctx context.Context, spaceID uuid.UUID, if err != nil { return nil, errs.Wrapf(err, "error while creating work item") } - log.Debug(ctx, map[string]interface{}{"pkg": "workitem", "wi_id": wi.ID}, "Work item created successfully!") + log.Debug(ctx, map[string]interface{}{"pkg": "workitem", "wi_id": wi.ID, "number": wi.Number}, "Work item created successfully!") return witem, nil } From 79ce30a1f270461b42a49f623acd4588392a1291 Mon Sep 17 00:00:00 2001 From: Xavier Coulon Date: Mon, 2 Oct 2017 19:24:29 +0200 Subject: [PATCH 03/14] Taking comments into account Signed-off-by: Xavier Coulon --- controller/search_spaces_blackbox_test.go | 4 ++-- search/search_repository_whitebox_test.go | 22 ++++++++++++---------- 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/controller/search_spaces_blackbox_test.go b/controller/search_spaces_blackbox_test.go index c65a5fe581..f919610fe3 100644 --- a/controller/search_spaces_blackbox_test.go +++ b/controller/search_spaces_blackbox_test.go @@ -6,7 +6,6 @@ import ( "strconv" "strings" "testing" - "time" "github.com/fabric8-services/fabric8-wit/app" "github.com/fabric8-services/fabric8-wit/app/test" @@ -18,6 +17,7 @@ import ( "github.com/fabric8-services/fabric8-wit/resource" "github.com/fabric8-services/fabric8-wit/space" testsupport "github.com/fabric8-services/fabric8-wit/test" + uuid "github.com/satori/go.uuid" "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" @@ -73,7 +73,7 @@ func (rest *TestSearchSpacesREST) UnSecuredController() (*goa.Service, *SearchCo func (rest *TestSearchSpacesREST) TestSpacesSearchOK() { // given - prefix := time.Now().Format("2006_Jan_2_15_04_05_") // using a unique prefix to make sure the test data will not collide with existing, older spaces. + prefix := uuid.NewV4().String() // using a unique prefix to make sure the test data will not collide with existing, older spaces. idents, err := createTestData(rest.db, prefix) require.Nil(rest.T(), err) tests := []okScenario{ diff --git a/search/search_repository_whitebox_test.go b/search/search_repository_whitebox_test.go index a96e17aab4..06dd0743cc 100644 --- a/search/search_repository_whitebox_test.go +++ b/search/search_repository_whitebox_test.go @@ -229,22 +229,24 @@ func stringInSlice(str string, list []string) bool { } func (s *searchRepositoryWhiteboxTest) TestSearchByID() { + // given req := &http.Request{Host: "localhost"} params := url.Values{} ctx := goa.NewContext(context.Background(), nil, req, params) - - wir := workitem.NewWorkItemRepository(s.DB) - 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", - } + fxt, error := tf.NewFixture(s.DB, tf.WorkItems(1, func(fxt *tf.TestFixture, idx int) error { + // fxt.WorkItems[idx] + 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", + } + return nil + })) 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) From 461eeca2e160c2f5561d23c1b02c30436faaba86 Mon Sep 17 00:00:00 2001 From: Xavier Coulon Date: Tue, 3 Oct 2017 18:04:30 +0200 Subject: [PATCH 04/14] Refactor the `SearchByText` test Refactored the test to use the TestFixture to initialise the work items from the data set. Remove the `optionalKeywords` which contained the work item number but failed when the 3rd work item was retrieved because its number ('3') would match the description. This worked in the past when the number did not exist and the search was based on the ID (UUID). Also, searching on a URL did not seem to make sense since there was no such value in the work item fields. Also, add a test to search by number using the `number:%d` query. Signed-off-by: Xavier Coulon --- search/search_repository.go | 3 +- search/search_repository_whitebox_test.go | 263 +++++++++--------- .../workitem_number_sequence_repository.go | 4 +- 3 files changed, 143 insertions(+), 127 deletions(-) diff --git a/search/search_repository.go b/search/search_repository.go index 0d0917899c..343f576ce4 100644 --- a/search/search_repository.go +++ b/search/search_repository.go @@ -481,7 +481,7 @@ func generateSQLSearchInfo(keywords searchKeyword) (sqlParameter string) { // extracted this function from List() in order to close the rows object with "defer" for more readability // workaround for https://github.com/lib/pq/issues/81 func (r *GormSearchRepository) search(ctx context.Context, sqlSearchQueryParameter string, workItemTypes []uuid.UUID, start *int, limit *int, spaceID *string) ([]workitem.WorkItemStorage, uint64, error) { - log.Info(ctx, nil, "Searching work items...") + defer r.db.LogMode(false) db := r.db.Model(workitem.WorkItemStorage{}).Where("tsv @@ query") if start != nil { if *start < 0 { @@ -572,6 +572,7 @@ func (r *GormSearchRepository) SearchFullText(ctx context.Context, rawSearchStri } sqlSearchQueryParameter := generateSQLSearchInfo(parsedSearchDict) + log.Warn(ctx, map[string]interface{}{"search query": sqlSearchQueryParameter}, "searching for work items") var rows []workitem.WorkItemStorage rows, count, err := r.search(ctx, sqlSearchQueryParameter, parsedSearchDict.workItemTypes, start, limit, spaceID) if err != nil { diff --git a/search/search_repository_whitebox_test.go b/search/search_repository_whitebox_test.go index 06dd0743cc..48424d0b16 100644 --- a/search/search_repository_whitebox_test.go +++ b/search/search_repository_whitebox_test.go @@ -13,7 +13,6 @@ import ( "github.com/fabric8-services/fabric8-wit/gormtestsupport" "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" @@ -43,132 +42,123 @@ func (s *searchRepositoryWhiteboxTest) SetupTest() { } type SearchTestDescriptor struct { - wi workitem.WorkItem + fields map[string]interface{} searchString string minimumResults int } -func (s *searchRepositoryWhiteboxTest) TestSearchByText() { - wir := workitem.NewWorkItemRepository(s.DB) +func (s *searchRepositoryWhiteboxTest) setupTestDataSet() ([]SearchTestDescriptor, *tf.TestFixture) { + // given testDataSet := []SearchTestDescriptor{ { - wi: workitem.WorkItem{ - Fields: map[string]interface{}{ - workitem.SystemTitle: "test sbose title '12345678asdfgh'", - workitem.SystemDescription: rendering.NewMarkupContentFromLegacy(`"description" for search test`), - workitem.SystemCreator: "sbose78", - workitem.SystemAssignees: []string{"pranav"}, - workitem.SystemState: "closed", - }, + fields: map[string]interface{}{ + workitem.SystemTitle: "test sbose title '12345678asdfgh'", + workitem.SystemDescription: rendering.NewMarkupContentFromLegacy(`"description" for search test`), + workitem.SystemCreator: "sbose78", + workitem.SystemAssignees: []string{"pranav"}, + workitem.SystemState: "closed", }, searchString: `Sbose "deScription" '12345678asdfgh' `, minimumResults: 1, }, { - wi: workitem.WorkItem{ - Fields: map[string]interface{}{ - workitem.SystemTitle: "add new error types in models/errors.go'", - workitem.SystemDescription: rendering.NewMarkupContentFromLegacy(`Make sure remoteworkitem can access..`), - workitem.SystemCreator: "sbose78", - workitem.SystemAssignees: []string{"pranav"}, - workitem.SystemState: "closed", - }, + fields: map[string]interface{}{ + workitem.SystemTitle: "add new error types in models/errors.go'", + workitem.SystemDescription: rendering.NewMarkupContentFromLegacy(`Make sure remoteworkitem can access..`), + workitem.SystemCreator: "sbose78", + workitem.SystemAssignees: []string{"pranav"}, + workitem.SystemState: "closed", }, searchString: `models/errors.go remoteworkitem `, minimumResults: 1, }, { - wi: workitem.WorkItem{ - Fields: map[string]interface{}{ - workitem.SystemTitle: "test sbose title '12345678asdfgh'", - workitem.SystemDescription: rendering.NewMarkupContentFromLegacy(`"description" for search test`), - workitem.SystemCreator: "sbose78", - workitem.SystemAssignees: []string{"pranav"}, - workitem.SystemState: "closed", - }, + fields: map[string]interface{}{ + workitem.SystemTitle: "test sbose title '12345678asdfgh'", + workitem.SystemDescription: rendering.NewMarkupContentFromLegacy(`"description" for search test`), + workitem.SystemCreator: "sbose78", + workitem.SystemAssignees: []string{"pranav"}, + workitem.SystemState: "closed", }, searchString: `Sbose "deScription" '12345678asdfgh' `, minimumResults: 1, }, { - wi: workitem.WorkItem{ - // will test behaviour when null fields are present. In this case, "system.description" is nil - Fields: map[string]interface{}{ - workitem.SystemTitle: "test nofield sbose title '12345678asdfgh'", - workitem.SystemCreator: "sbose78", - workitem.SystemAssignees: []string{"pranav"}, - workitem.SystemState: "closed", - }, + // will test behaviour when null fields are present. In this case, "system.description" is nil + fields: map[string]interface{}{ + workitem.SystemTitle: "test nofield sbose title '12345678asdfgh'", + workitem.SystemCreator: "sbose78", + workitem.SystemAssignees: []string{"pranav"}, + workitem.SystemState: "closed", }, searchString: `sbose nofield `, minimumResults: 1, }, { - wi: workitem.WorkItem{ - // will test behaviour when null fields are present. In this case, "system.description" is nil - Fields: map[string]interface{}{ - workitem.SystemTitle: "test should return 0 results'", - workitem.SystemCreator: "sbose78", - workitem.SystemAssignees: []string{"pranav"}, - workitem.SystemState: "closed", - }, + // will test behaviour when null fields are present. In this case, "system.description" is nil + fields: map[string]interface{}{ + workitem.SystemTitle: "test should return 0 results'", + workitem.SystemCreator: "sbose78", + workitem.SystemAssignees: []string{"pranav"}, + workitem.SystemState: "closed", }, searchString: `negative case `, minimumResults: 0, }, { - wi: workitem.WorkItem{ - // search stirng with braces should be acceptable case - Fields: map[string]interface{}{ - workitem.SystemTitle: "Bug reported by administrator for input = (value)", - workitem.SystemCreator: "pgore", - workitem.SystemAssignees: []string{"pranav"}, - workitem.SystemState: "new", - }, + // search stirng with braces should be acceptable case + fields: map[string]interface{}{ + workitem.SystemTitle: "Bug reported by administrator for input = (value)", + workitem.SystemCreator: "pgore", + workitem.SystemAssignees: []string{"pranav"}, + workitem.SystemState: "new", }, searchString: `(value) `, minimumResults: 1, }, { - wi: workitem.WorkItem{ - // search stirng with surrounding braces should be acceptable case - Fields: map[string]interface{}{ - workitem.SystemTitle: "trial for braces (pranav) {shoubhik} [aslak]", - workitem.SystemCreator: "pgore", - workitem.SystemAssignees: []string{"pranav"}, - workitem.SystemState: "new", - }, + // search stirng with surrounding braces should be acceptable case + fields: map[string]interface{}{ + workitem.SystemTitle: "trial for braces (pranav) {shoubhik} [aslak]", + workitem.SystemCreator: "pgore", + workitem.SystemAssignees: []string{"pranav"}, + workitem.SystemState: "new", }, searchString: `(pranav) {shoubhik} [aslak] `, minimumResults: 1, }, } - // create the parent space - fxt := tf.NewTestFixture(s.T(), s.DB, tf.Spaces(1)) + fxt := tf.NewTestFixture(s.T(), s.DB, tf.Identities(2), tf.WorkItems(len(testDataSet), func(fxt *tf.TestFixture, idx int) error { + fxt.WorkItems[idx].SpaceID = fxt.Spaces[0].ID + fxt.WorkItems[idx].Type = fxt.WorkItemTypes[0].ID + fxt.WorkItems[idx].Fields = testDataSet[idx].fields + fxt.WorkItems[idx].Fields[workitem.SystemCreator] = fxt.Identities[0].ID.String() + fxt.WorkItems[idx].Fields[workitem.SystemAssignees] = []string{fxt.Identities[1].ID.String()} + return nil + })) + return testDataSet, fxt +} + +// TestSearchByText verifies search on title or description +func (s *searchRepositoryWhiteboxTest) TestSearchByText() { + // given + testDataSet, fxt := s.setupTestDataSet() // - for _, testData := range testDataSet { - workItem := testData.wi + for idx, testData := range testDataSet { 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, "\"") + s.T().Logf("TestData #%d: using search string: %s -> %d matches", idx, searchString, len(workItemList)) // 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 { @@ -180,20 +170,15 @@ func (s *searchRepositoryWhiteboxTest) TestSearchByText() { // 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)} + //[]string{createdWorkItem.ID, `"Sbose"`, `"deScription"`, `'12345678asdfgh'`} // 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) - + s.T().Logf("Examining workitem id=`%v` number=`%d` using keywords %v", workItemValue.ID, workItemValue.Number, allKeywords) for _, keyWord := range allKeywords { - + keyWord = strings.ToLower(keyWord) + s.T().Logf("Verifying workitem id=`%v` number=`%d` for keyword `%s`...", workItemValue.ID, workItemValue.Number, keyWord) workItemTitle := "" if workItemValue.Fields[workitem.SystemTitle] != nil { workItemTitle = strings.ToLower(workItemValue.Fields[workitem.SystemTitle].(string)) @@ -203,16 +188,59 @@ func (s *searchRepositoryWhiteboxTest) TestSearchByText() { descriptionField := workItemValue.Fields[workitem.SystemDescription].(rendering.MarkupContent) workItemDescription = strings.ToLower(descriptionField.Content) } + assert.True(s.T(), + strings.Contains(workItemTitle, keyWord) || strings.Contains(workItemDescription, keyWord), + "`%s` neither found in title `%s` nor in the description `%s` for workitem #%d", keyWord, workItemTitle, workItemDescription, workItemValue.Number) + } + } + + } +} + +// TestSearchByText verifies search on number +func (s *searchRepositoryWhiteboxTest) TestSearchByNumber() { + // given + testDataSet, fxt := s.setupTestDataSet() + // + for idx, testData := range testDataSet { + number := fxt.WorkItems[idx].Number + minimumResults := testData.minimumResults + req := &http.Request{Host: "localhost"} + params := url.Values{} + ctx := goa.NewContext(context.Background(), nil, req, params) + + // had to dynamically create this since I didn't now the URL/ID of the workitem + // till the test data was created. + searchString := fmt.Sprintf("\"number:%d\"", number) + 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, "\"") + s.T().Logf("TestData #%d: using search string: %s -> %d matches", idx, searchString, len(workItemList)) + // 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) + //[]string{createdWorkItem.ID, `"Sbose"`, `"deScription"`, `'12345678asdfgh'`} + + // 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().Logf("Examining workitem id=`%v` number=`%d` using keywords %v", workItemValue.ID, workItemValue.Number, allKeywords) + for _, keyWord := range allKeywords { 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) - } + s.T().Logf("Verifying workitem id=`%v` number=`%d` for keyword `%s`...", workItemValue.ID, workItemValue.Number, keyWord) + assert.Equal(s.T(), number, workItemValue.Number, + "workitem #%d did not have the expected number", workItemValue.Number, number) } } @@ -233,49 +261,36 @@ func (s *searchRepositoryWhiteboxTest) TestSearchByID() { req := &http.Request{Host: "localhost"} params := url.Values{} ctx := goa.NewContext(context.Background(), nil, req, params) - workItem := workitem.WorkItem{Fields: make(map[string]interface{})} - - fxt, error := tf.NewFixture(s.DB, tf.WorkItems(1, func(fxt *tf.TestFixture, idx int) error { - // fxt.WorkItems[idx] - workItem.Fields = map[string]interface{}{ + // create 2 work items, the second one having the number of the first one in its title + fxt, err := tf.NewFixture(s.DB, tf.WorkItems(2, func(fxt *tf.TestFixture, idx int) error { + fxt.WorkItems[idx].Type = workitem.SystemBug + fxt.WorkItems[idx].Fields = map[string]interface{}{ workitem.SystemTitle: "Search Test Sbose", workitem.SystemDescription: rendering.NewMarkupContentFromLegacy("Description"), - workitem.SystemCreator: "sbose78", + workitem.SystemCreator: s.modifierID.String(), workitem.SystemAssignees: []string{"pranav"}, workitem.SystemState: "closed", } - + fxt.WorkItems[idx].SpaceID = fxt.Spaces[0].ID + // for the second work item, use the number of the first work item + if idx == 1 { + fxt.WorkItems[idx].Fields[workitem.SystemTitle] = "Search Test Sbose" + strconv.Itoa(fxt.WorkItems[0].Number) + } return nil })) - 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 - - 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) - } - + require.Nil(s.T(), err, "Couldn't create test data") sr := NewGormSearchRepository(s.DB) - + // when 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) - } + searchString := "number:" + strconv.Itoa(fxt.WorkItems[0].Number) + spaceID := fxt.Spaces[0].ID.String() // make sure the search is limited to the space to avoid collision with other existing data + workItemList, _, err := sr.SearchFullText(ctx, searchString, &start, &limit, &spaceID) + // then + require.Nil(s.T(), err) + // Number is unique, hence search result set's length should be 1 + require.Equal(s.T(), len(workItemList), 1) + s.T().Log("Found search result for ID Search ", workItemList[0].ID) + assert.Equal(s.T(), fxt.WorkItems[0].ID, workItemList[0].ID) } func TestGenerateSQLSearchStringText(t *testing.T) { diff --git a/workitem/number_sequence/workitem_number_sequence_repository.go b/workitem/number_sequence/workitem_number_sequence_repository.go index b08b57b305..292282495f 100644 --- a/workitem/number_sequence/workitem_number_sequence_repository.go +++ b/workitem/number_sequence/workitem_number_sequence_repository.go @@ -33,7 +33,7 @@ func (r *GormWorkItemNumberSequenceRepository) Create(ctx context.Context, space 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()) } - log.Warn(nil, map[string]interface{}{"Sequence": numberSequence.String()}, "Creating sequence") + log.Debug(nil, map[string]interface{}{"Sequence": numberSequence.String()}, "Creating sequence") return &numberSequence, nil } @@ -51,6 +51,6 @@ func (r *GormWorkItemNumberSequenceRepository) NextVal(ctx context.Context, spac if err := r.db.Save(&numberSequence).Error; err != nil { return nil, errs.Wrapf(err, "failed to update work item with sequence number: `%s`", numberSequence.String()) } - log.Warn(nil, map[string]interface{}{"Sequence": numberSequence.String()}, "computing nextVal") + log.Debug(nil, map[string]interface{}{"Sequence": numberSequence.String()}, "computing nextVal") return &numberSequence, nil } From afe48ecb0255df82a3aebcbab368312eb2fee1d6 Mon Sep 17 00:00:00 2001 From: Xavier Coulon Date: Fri, 6 Oct 2017 11:59:34 +0200 Subject: [PATCH 05/14] Addressing review comments - using an `UPSERT` statement to avoid creating then updating the work item number sequence, which also avoids putting a `LOCK` on the row (using the `... FOR UPDATE` option in the `SELECT` statement) - restored search test on URL, and added some weight on the work item number (`*A`) when the URL matches a known one and the number can be extracted. Signed-off-by: Xavier Coulon --- glide.lock | 2 + glide.yaml | 3 ++ migration/migration.go | 3 -- .../077-work_item_number_sequence.sql | 5 --- search/search_repository.go | 15 +++---- search/search_repository_whitebox_test.go | 24 +++++++---- space/space.go | 5 --- .../workitem_number_sequence.go | 3 +- .../workitem_number_sequence_repository.go | 40 ++++++------------- workitem/workitem_repository.go | 6 +-- 10 files changed, 44 insertions(+), 62 deletions(-) delete mode 100644 migration/sql-files/077-work_item_number_sequence.sql diff --git a/glide.lock b/glide.lock index 236c8c3559..714cbb900f 100644 --- a/glide.lock +++ b/glide.lock @@ -166,6 +166,8 @@ imports: version: 1dba4b3954bc059efc3991ec364f9f9a35f597d2 - name: github.com/Sirupsen/logrus version: c078b1e43f58d563c74cebe63c85789e76ddb627 + repo: https://github.com/sirupsen/logrus + vcs: git - name: github.com/sourcegraph/annotate version: f4cad6c6324d3f584e1743d8b3e0e017a5f3a636 - name: github.com/sourcegraph/syntaxhighlight diff --git a/glide.yaml b/glide.yaml index 83e8ecae3d..7c7256e949 100644 --- a/glide.yaml +++ b/glide.yaml @@ -106,6 +106,9 @@ import: - golint - package: github.com/fzipp/gocyclo - package: github.com/Sirupsen/logrus + version: c078b1e43f58d563c74cebe63c85789e76ddb627 + repo: https://github.com/sirupsen/logrus + vcs: git - package: github.com/sourcegraph/syntaxhighlight - package: github.com/jstemmer/go-junit-report - package: github.com/sourcegraph/annotate diff --git a/migration/migration.go b/migration/migration.go index 12114dbd5e..7ad5f8267d 100644 --- a/migration/migration.go +++ b/migration/migration.go @@ -352,9 +352,6 @@ 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 diff --git a/migration/sql-files/077-work_item_number_sequence.sql b/migration/sql-files/077-work_item_number_sequence.sql deleted file mode 100644 index fb41b2879b..0000000000 --- a/migration/sql-files/077-work_item_number_sequence.sql +++ /dev/null @@ -1,5 +0,0 @@ --- 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; - - diff --git a/search/search_repository.go b/search/search_repository.go index 343f576ce4..6a018f896e 100644 --- a/search/search_repository.go +++ b/search/search_repository.go @@ -174,7 +174,7 @@ func getSearchQueryFromURLPattern(patternName, stringToMatch string) string { searchQueryString = fmt.Sprintf("%s:*", searchQueryString) if result["id"] != "" { // Look for pattern's ID field, if exists update searchQueryString - searchQueryString = fmt.Sprintf("(%v:* | %v)", result["id"], searchQueryString) + searchQueryString = fmt.Sprintf("(%v:*A | %v)", result["id"], searchQueryString) // searchQueryString = "(" + result["id"] + ":*" + " | " + searchQueryString + ")" } return searchQueryString @@ -239,9 +239,11 @@ func parseSearchString(ctx context.Context, rawSearchString string) (searchKeywo } res.workItemTypes = append(res.workItemTypes, typeID) } else if govalidator.IsURL(part) { + log.Debug(ctx, map[string]interface{}{"url": part}, "found a URL in the query string") part := strings.ToLower(part) part = trimProtocolFromURLString(part) searchQueryFromURL := getSearchQueryFromURLString(part) + log.Debug(ctx, map[string]interface{}{"url": part, "search_query": searchQueryFromURL}, "found a URL in the query string") res.words = append(res.words, searchQueryFromURL) } else { part := strings.ToLower(part) @@ -446,7 +448,6 @@ func (q Query) generateExpression() (criteria.Expression, error) { // parseFilterString accepts a raw string and generates a criteria expression func parseFilterString(ctx context.Context, rawSearchString string) (criteria.Expression, error) { - fm := map[string]interface{}{} // Parsing/Unmarshalling JSON encoding/json err := json.Unmarshal([]byte(rawSearchString), &fm) @@ -481,6 +482,7 @@ func generateSQLSearchInfo(keywords searchKeyword) (sqlParameter string) { // extracted this function from List() in order to close the rows object with "defer" for more readability // workaround for https://github.com/lib/pq/issues/81 func (r *GormSearchRepository) search(ctx context.Context, sqlSearchQueryParameter string, workItemTypes []uuid.UUID, start *int, limit *int, spaceID *string) ([]workitem.WorkItemStorage, uint64, error) { + r.db.LogMode(true) defer r.db.LogMode(false) db := r.db.Model(workitem.WorkItemStorage{}).Where("tsv @@ query") if start != nil { @@ -572,8 +574,8 @@ func (r *GormSearchRepository) SearchFullText(ctx context.Context, rawSearchStri } sqlSearchQueryParameter := generateSQLSearchInfo(parsedSearchDict) - log.Warn(ctx, map[string]interface{}{"search query": sqlSearchQueryParameter}, "searching for work items") var rows []workitem.WorkItemStorage + log.Debug(ctx, map[string]interface{}{"search query": sqlSearchQueryParameter}, "searching for work items") rows, count, err := r.search(ctx, sqlSearchQueryParameter, parsedSearchDict.workItemTypes, start, limit, spaceID) if err != nil { return nil, 0, errs.WithStack(err) @@ -742,10 +744,3 @@ func (r *GormSearchRepository) Filter(ctx context.Context, rawFilterString strin } return res, count, nil } - -func init() { - // While registering URLs do not include protocol because it will be removed before scanning starts - // Please do not include trailing slashes because it will be removed before scanning starts - RegisterAsKnownURL("test-work-item-list-details", `(?Pdemo.almighty.io)(?P/work-item/list/detail/)(?P\d*)`) - RegisterAsKnownURL("test-work-item-board-details", `(?Pdemo.almighty.io)(?P/work-item/board/detail/)(?P\d*)`) -} diff --git a/search/search_repository_whitebox_test.go b/search/search_repository_whitebox_test.go index 48424d0b16..1a21c30cc1 100644 --- a/search/search_repository_whitebox_test.go +++ b/search/search_repository_whitebox_test.go @@ -34,6 +34,15 @@ type searchRepositoryWhiteboxTest struct { modifierID uuid.UUID } +func (s *searchRepositoryWhiteboxTest) SetupSuite() { + s.DBTestSuite.SetupSuite() + // While registering URLs do not include protocol because it will be removed before scanning starts + // Please do not include trailing slashes because it will be removed before scanning starts + RegisterAsKnownURL("test-work-item-list-details", `(?Pdemo.openshift.io)(?P/work-item/list/detail/)(?P\d*)`) + RegisterAsKnownURL("test-work-item-board-details", `(?Pdemo.openshift.io)(?P/work-item/board/detail/)(?P\d*)`) + +} + func (s *searchRepositoryWhiteboxTest) SetupTest() { s.DBTestSuite.SetupTest() testIdentity, err := testsupport.CreateTestIdentity(s.DB, "jdoe", "test") @@ -143,22 +152,22 @@ func (s *searchRepositoryWhiteboxTest) TestSearchByText() { testDataSet, fxt := s.setupTestDataSet() // for idx, testData := range testDataSet { - searchString := testData.searchString minimumResults := testData.minimumResults req := &http.Request{Host: "localhost"} params := url.Values{} ctx := goa.NewContext(context.Background(), nil, req, params) - // had to dynamically create this since I didn't now the URL/ID of the workitem // till the test data was created. - searchString = fmt.Sprintf("\"%s\"", searchString) + searchString := testData.searchString + workItemURLInSearchString := fmt.Sprintf("%s%d", "http://demo.openshift.io/work-item/list/detail/", fxt.WorkItems[idx].Number) + searchString = fmt.Sprintf("\"%s %s\"", searchString, workItemURLInSearchString) 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, "\"") - s.T().Logf("TestData #%d: using search string: %s -> %d matches", idx, searchString, len(workItemList)) + s.T().Logf("TestData #%d: using search string: %s -> %d matches", (idx + 1), searchString, len(workItemList)) // 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 { @@ -170,8 +179,8 @@ func (s *searchRepositoryWhiteboxTest) TestSearchByText() { // These keywords need a match in the textual part. allKeywords := strings.Fields(searchString) - //[]string{createdWorkItem.ID, `"Sbose"`, `"deScription"`, `'12345678asdfgh'`} - + // These keywords need a match optionally either as URL string or ID + keyWord = strings.ToLower(keyWord) + optionalKeywords := []string{workItemURLInSearchString, strconv.Itoa(fxt.WorkItems[idx].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 { @@ -189,7 +198,8 @@ func (s *searchRepositoryWhiteboxTest) TestSearchByText() { workItemDescription = strings.ToLower(descriptionField.Content) } assert.True(s.T(), - strings.Contains(workItemTitle, keyWord) || strings.Contains(workItemDescription, keyWord), + strings.Contains(workItemTitle, keyWord) || strings.Contains(workItemDescription, keyWord) || + (stringInSlice(keyWord, optionalKeywords) && strings.Contains(keyWord, strconv.Itoa(workItemValue.Number))), "`%s` neither found in title `%s` nor in the description `%s` for workitem #%d", keyWord, workItemTitle, workItemDescription, workItemValue.Number) } } diff --git a/space/space.go b/space/space.go index c2b84383f9..6d87fe4b22 100644 --- a/space/space.go +++ b/space/space.go @@ -230,11 +230,6 @@ 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 - _, 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") diff --git a/workitem/number_sequence/workitem_number_sequence.go b/workitem/number_sequence/workitem_number_sequence.go index 5a062e905f..c72a4d3e33 100644 --- a/workitem/number_sequence/workitem_number_sequence.go +++ b/workitem/number_sequence/workitem_number_sequence.go @@ -8,8 +8,7 @@ import ( // WorkItemNumberSequence the sequence for work item numbers in a space type WorkItemNumberSequence struct { - ID uuid.UUID `sql:"type:uuid" gorm:"primary_key"` - SpaceID uuid.UUID `sql:"type:uuid"` + SpaceID uuid.UUID `sql:"type:uuid" gorm:"primary_key"` CurrentVal int } diff --git a/workitem/number_sequence/workitem_number_sequence_repository.go b/workitem/number_sequence/workitem_number_sequence_repository.go index 292282495f..c54ce4d3f3 100644 --- a/workitem/number_sequence/workitem_number_sequence_repository.go +++ b/workitem/number_sequence/workitem_number_sequence_repository.go @@ -2,6 +2,7 @@ package numbersequence import ( "context" + "fmt" "github.com/fabric8-services/fabric8-wit/log" "github.com/jinzhu/gorm" @@ -11,8 +12,7 @@ import ( // WorkItemNumberSequenceRepository the interface for the work item number sequence repository type WorkItemNumberSequenceRepository interface { - Create(ctx context.Context, spaceID uuid.UUID) (*WorkItemNumberSequence, error) - NextVal(ctx context.Context, spaceID uuid.UUID) (*WorkItemNumberSequence, error) + NextVal(ctx context.Context, spaceID uuid.UUID) (*int, error) } // NewWorkItemNumberSequenceRepository creates a GormWorkItemNumberSequenceRepository @@ -26,31 +26,17 @@ type GormWorkItemNumberSequenceRepository struct { db *gorm.DB } -// Create returns the next work item sequence number for the given space ID. Creates an entry in the DB if none was found before -func (r *GormWorkItemNumberSequenceRepository) Create(ctx context.Context, spaceID uuid.UUID) (*WorkItemNumberSequence, error) { - // retrieve the current issue number in the given space - numberSequence := WorkItemNumberSequence{SpaceID: spaceID, CurrentVal: 0} - 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()) - } - log.Debug(nil, map[string]interface{}{"Sequence": numberSequence.String()}, "Creating sequence") - return &numberSequence, nil -} - // NextVal returns the next work item sequence number for the given space ID. Creates an entry in the DB if none was found before -func (r *GormWorkItemNumberSequenceRepository) NextVal(ctx context.Context, spaceID uuid.UUID) (*WorkItemNumberSequence, error) { - // retrieve the current issue number in the given space - numberSequence := WorkItemNumberSequence{} - tx := r.db.Model(&WorkItemNumberSequence{}).Set("gorm:query_option", "FOR UPDATE").Where("space_id = ?", spaceID).First(&numberSequence) - if tx.RecordNotFound() { - numberSequence.SpaceID = spaceID - numberSequence.CurrentVal = 1 - } else { - numberSequence.CurrentVal++ - } - if err := r.db.Save(&numberSequence).Error; err != nil { - return nil, errs.Wrapf(err, "failed to update work item with sequence number: `%s`", numberSequence.String()) +func (r *GormWorkItemNumberSequenceRepository) NextVal(ctx context.Context, spaceID uuid.UUID) (*int, error) { + // upsert the next val, retrieves full row + upsertStmt := fmt.Sprintf(`INSERT INTO %[1]s (space_id, current_val) VALUES ($1,1) + ON CONFLICT (space_id) DO UPDATE SET current_val = %[1]s.current_val + EXCLUDED.current_val + RETURNING current_val`, WorkItemNumberSequence{}.TableName()) + var currentVal int + err := r.db.CommonDB().QueryRow(upsertStmt, spaceID).Scan(¤tVal) + if err != nil { + return nil, errs.Wrapf(err, "failed to obtain next val for space with ID=`%s`", spaceID.String()) } - log.Debug(nil, map[string]interface{}{"Sequence": numberSequence.String()}, "computing nextVal") - return &numberSequence, nil + log.Debug(nil, map[string]interface{}{"space_id": spaceID, "next_val": currentVal}, "computed nextVal") + return ¤tVal, nil } diff --git a/workitem/workitem_repository.go b/workitem/workitem_repository.go index ed17077efc..43b660f8d3 100644 --- a/workitem/workitem_repository.go +++ b/workitem/workitem_repository.go @@ -15,7 +15,7 @@ import ( "github.com/fabric8-services/fabric8-wit/log" "github.com/fabric8-services/fabric8-wit/rendering" "github.com/fabric8-services/fabric8-wit/space" - numbersequence "github.com/fabric8-services/fabric8-wit/workitem/number_sequence" + "github.com/fabric8-services/fabric8-wit/workitem/number_sequence" "github.com/goadesign/goa" "github.com/jinzhu/gorm" @@ -565,7 +565,7 @@ func (r *GormWorkItemRepository) Create(ctx context.Context, spaceID uuid.UUID, return nil, errors.NewInternalError(ctx, err) } pos = pos + orderValue - wiNumberSequence, err := r.winr.NextVal(ctx, spaceID) + number, err := r.winr.NextVal(ctx, spaceID) if err != nil { return nil, errors.NewInternalError(ctx, err) } @@ -574,7 +574,7 @@ func (r *GormWorkItemRepository) Create(ctx context.Context, spaceID uuid.UUID, Fields: Fields{}, ExecutionOrder: pos, SpaceID: spaceID, - Number: wiNumberSequence.CurrentVal, + Number: *number, } fields[SystemCreator] = creatorID.String() for fieldName, fieldDef := range wiType.Fields { From 4477a20a7b53b8c968fdb32a9d5e7364ce132538 Mon Sep 17 00:00:00 2001 From: Xavier Coulon Date: Fri, 6 Oct 2017 15:45:54 +0200 Subject: [PATCH 06/14] Fix test failures (register URL pattern) Signed-off-by: Xavier Coulon --- search/search_repository.go | 2 -- search/search_repository_whitebox_test.go | 34 ++++++++++++----------- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/search/search_repository.go b/search/search_repository.go index 6a018f896e..471c6b1e5c 100644 --- a/search/search_repository.go +++ b/search/search_repository.go @@ -482,8 +482,6 @@ func generateSQLSearchInfo(keywords searchKeyword) (sqlParameter string) { // extracted this function from List() in order to close the rows object with "defer" for more readability // workaround for https://github.com/lib/pq/issues/81 func (r *GormSearchRepository) search(ctx context.Context, sqlSearchQueryParameter string, workItemTypes []uuid.UUID, start *int, limit *int, spaceID *string) ([]workitem.WorkItemStorage, uint64, error) { - r.db.LogMode(true) - defer r.db.LogMode(false) db := r.db.Model(workitem.WorkItemStorage{}).Where("tsv @@ query") if start != nil { if *start < 0 { diff --git a/search/search_repository_whitebox_test.go b/search/search_repository_whitebox_test.go index 1a21c30cc1..03a6c73d64 100644 --- a/search/search_repository_whitebox_test.go +++ b/search/search_repository_whitebox_test.go @@ -24,6 +24,11 @@ import ( "github.com/stretchr/testify/suite" ) +func init() { + RegisterAsKnownURL("test-work-item-list-details", `(?Pdemo.openshift.io)(?P/work-item/list/detail/)(?P\d*)`) + RegisterAsKnownURL("test-work-item-board-details", `(?Pdemo.openshift.io)(?P/work-item/board/detail/)(?P\d*)`) +} + func TestRunSearchRepositoryWhiteboxTest(t *testing.T) { resource.Require(t, resource.Database) suite.Run(t, &searchRepositoryWhiteboxTest{DBTestSuite: gormtestsupport.NewDBTestSuite("../config.yaml")}) @@ -38,9 +43,6 @@ func (s *searchRepositoryWhiteboxTest) SetupSuite() { s.DBTestSuite.SetupSuite() // While registering URLs do not include protocol because it will be removed before scanning starts // Please do not include trailing slashes because it will be removed before scanning starts - RegisterAsKnownURL("test-work-item-list-details", `(?Pdemo.openshift.io)(?P/work-item/list/detail/)(?P\d*)`) - RegisterAsKnownURL("test-work-item-board-details", `(?Pdemo.openshift.io)(?P/work-item/board/detail/)(?P\d*)`) - } func (s *searchRepositoryWhiteboxTest) SetupTest() { @@ -351,22 +353,22 @@ func TestParseSearchStringURL(t *testing.T) { t.Parallel() resource.Require(t, resource.UnitTest) inputSet := []searchTestData{{ - query: "http://demo.almighty.io/work-item/list/detail/100", + query: "http://demo.openshift.io/work-item/list/detail/100", expected: searchKeyword{ number: nil, - words: []string{"(100:* | demo.almighty.io/work-item/list/detail/100:*)"}, + words: []string{"(100:*A | demo.openshift.io/work-item/list/detail/100:*)"}, }, }, { - query: "http://demo.almighty.io/work-item/board/detail/100", + query: "http://demo.openshift.io/work-item/board/detail/100", expected: searchKeyword{ number: nil, - words: []string{"(100:* | demo.almighty.io/work-item/board/detail/100:*)"}, + words: []string{"(100:*A | demo.openshift.io/work-item/board/detail/100:*)"}, }, }} for _, input := range inputSet { op, _ := parseSearchString(context.Background(), input.query) - assert.True(t, assert.ObjectsAreEqualValues(input.expected, op)) + assert.Equal(t, input.expected, op) } } @@ -374,16 +376,16 @@ func TestParseSearchStringURLWithouID(t *testing.T) { t.Parallel() resource.Require(t, resource.UnitTest) inputSet := []searchTestData{{ - query: "http://demo.almighty.io/work-item/list/detail/", + query: "http://demo.openshift.io/work-item/list/detail/", expected: searchKeyword{ number: nil, - words: []string{"demo.almighty.io/work-item/list/detail:*"}, + words: []string{"demo.openshift.io/work-item/list/detail:*"}, }, }, { - query: "http://demo.almighty.io/work-item/board/detail/", + query: "http://demo.openshift.io/work-item/board/detail/", expected: searchKeyword{ number: nil, - words: []string{"demo.almighty.io/work-item/board/detail:*"}, + words: []string{"demo.openshift.io/work-item/board/detail:*"}, }, }} @@ -411,11 +413,11 @@ func TestParseSearchStringCombination(t *testing.T) { resource.Require(t, resource.UnitTest) // do combination of ID, full text and URLs // check if it works as expected. - input := "http://general.url.io http://demo.almighty.io/work-item/list/detail/100 number:300 golang book and number:900 \t \n unwanted" + input := "http://general.url.io http://demo.openshift.io/work-item/list/detail/100 number:300 golang book and number:900 \t \n unwanted" op, _ := parseSearchString(context.Background(), input) expectedSearchRes := searchKeyword{ number: []string{"300:*A", "900:*A"}, - words: []string{"general.url.io:*", "(100:* | demo.almighty.io/work-item/list/detail/100:*)", "golang:*", "book:*", "and:*", "unwanted:*"}, + words: []string{"general.url.io:*", "(100:*A | demo.openshift.io/work-item/list/detail/100:*)", "golang:*", "book:*", "and:*", "unwanted:*"}, } assert.True(t, assert.ObjectsAreEqualValues(expectedSearchRes, op)) } @@ -468,7 +470,7 @@ func TestGetSearchQueryFromURLPattern(t *testing.T) { RegisterAsKnownURL(routeName, urlRegex) searchQuery := getSearchQueryFromURLPattern(routeName, "google.me.io/everything/100") - assert.Equal(t, "(100:* | google.me.io/everything/100:*)", searchQuery) + assert.Equal(t, "(100:*A | google.me.io/everything/100:*)", searchQuery) searchQuery = getSearchQueryFromURLPattern(routeName, "google.me.io/everything/") assert.Equal(t, "google.me.io/everything/:*", searchQuery) @@ -492,5 +494,5 @@ func TestGetSearchQueryFromURLString(t *testing.T) { assert.Equal(t, "google.me.io/everything/:*", searchQuery) searchQuery = getSearchQueryFromURLString("google.me.io/everything/100") - assert.Equal(t, "(100:* | google.me.io/everything/100:*)", searchQuery) + assert.Equal(t, "(100:*A | google.me.io/everything/100:*)", searchQuery) } From 1cb3ca8261fb66e3964c812c3b94bc5ca3cdf0fe Mon Sep 17 00:00:00 2001 From: Xavier Coulon Date: Fri, 6 Oct 2017 17:56:16 +0200 Subject: [PATCH 07/14] Adding a test to create work items concurrently Signed-off-by: Xavier Coulon --- workitem/workitem_repository_blackbox_test.go | 64 +++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/workitem/workitem_repository_blackbox_test.go b/workitem/workitem_repository_blackbox_test.go index b28da197fa..78771d88b5 100644 --- a/workitem/workitem_repository_blackbox_test.go +++ b/workitem/workitem_repository_blackbox_test.go @@ -1,11 +1,15 @@ package workitem_test import ( + "context" + "fmt" "testing" "time" + "github.com/fabric8-services/fabric8-wit/application" "github.com/fabric8-services/fabric8-wit/codebase" "github.com/fabric8-services/fabric8-wit/errors" + "github.com/fabric8-services/fabric8-wit/gormapplication" "github.com/fabric8-services/fabric8-wit/gormtestsupport" "github.com/fabric8-services/fabric8-wit/rendering" "github.com/fabric8-services/fabric8-wit/resource" @@ -302,3 +306,63 @@ func (s *workItemRepoBlackBoxTest) TestLookupIDByNamedSpaceAndNumberStaleSpace() require.NotNil(s.T(), spaceID2) assert.Equal(s.T(), wi2.SpaceID, *spaceID2) } + +func (s *workItemRepoBlackBoxTest) TestConcurrentWorkItemCreations() { + // given + fxt := tf.NewTestFixture(s.T(), s.DB, + tf.Identities(1, func(fxt *tf.TestFixture, idx int) error { + fxt.Identities[idx].Username = "TestConcurrentCreate-" + uuid.NewV4().String() + return nil + }), + tf.Spaces(1, func(fxt *tf.TestFixture, idx int) error { + fxt.Spaces[idx].OwnerId = fxt.Identities[idx].ID + fxt.Spaces[idx].Name = "TestConcurrentCreate-" + uuid.NewV4().String() + return nil + }), + tf.WorkItemTypes(1, func(fxt *tf.TestFixture, idx int) error { + fxt.WorkItemTypes[idx].SpaceID = fxt.Spaces[0].ID + return nil + })) + type Report struct { + id int + total int + failures int + } + done := make(chan Report) + + routines := 10 + itemsPerRoutine := 50 + // when running concurrent go routines simultaneously + for i := 0; i < routines; i++ { + // in each go rountine, run 10 creations + go func(rountineID int) { + report := Report{id: rountineID} + for j := 0; j < itemsPerRoutine; j++ { + if err := application.Transactional(gormapplication.NewGormDB(s.DB), func(app application.Application) error { + + fields := map[string]interface{}{ + workitem.SystemTitle: uuid.NewV4().String(), + workitem.SystemState: workitem.SystemStateNew, + } + _, err := s.repo.Create(context.Background(), fxt.Spaces[0].ID, fxt.WorkItemTypes[0].ID, fields, fxt.Identities[0].ID) + + return err + }); err != nil { + s.T().Logf("Creation failed: %s", err.Error()) + report.failures++ + } + report.total++ + } + done <- report + }(i) + } + // wait for all items to be created + for i := 0; i < routines; i++ { + report := <-done + fmt.Printf("Routine #%d done: %d creations, including %d failure(s)\n", report.id, report.total, report.failures) + assert.Equal(s.T(), itemsPerRoutine, report.total) + assert.Equal(s.T(), 0, report.failures) + } + + // then +} From d07b413eadd77cfafcb72e60ca05d3620a7559d0 Mon Sep 17 00:00:00 2001 From: Xavier Coulon Date: Mon, 9 Oct 2017 15:25:16 +0200 Subject: [PATCH 08/14] Refactor identities creation in test using UUIDs of created identities instead of usernames also, move a comment to `init()` where it belongs also, comment on use of `:*A` in fulltext search Signed-off-by: Xavier Coulon --- search/search_repository.go | 2 + search/search_repository_whitebox_test.go | 70 ++++++++----------- .../testfixture/customize_entity_callbacks.go | 13 ++++ 3 files changed, 44 insertions(+), 41 deletions(-) diff --git a/search/search_repository.go b/search/search_repository.go index 471c6b1e5c..886fc651b2 100644 --- a/search/search_repository.go +++ b/search/search_repository.go @@ -174,6 +174,8 @@ func getSearchQueryFromURLPattern(patternName, stringToMatch string) string { searchQueryString = fmt.Sprintf("%s:*", searchQueryString) if result["id"] != "" { // Look for pattern's ID field, if exists update searchQueryString + // `*A` is used to add sme weight to the work item number in the search results. + // See https://www.postgresql.org/docs/9.6/static/textsearch-controls.html searchQueryString = fmt.Sprintf("(%v:*A | %v)", result["id"], searchQueryString) // searchQueryString = "(" + result["id"] + ":*" + " | " + searchQueryString + ")" } diff --git a/search/search_repository_whitebox_test.go b/search/search_repository_whitebox_test.go index 03a6c73d64..63312a123e 100644 --- a/search/search_repository_whitebox_test.go +++ b/search/search_repository_whitebox_test.go @@ -25,6 +25,8 @@ import ( ) func init() { + // While registering URLs do not include protocol because it will be removed before scanning starts + // Please do not include trailing slashes because it will be removed before scanning starts RegisterAsKnownURL("test-work-item-list-details", `(?Pdemo.openshift.io)(?P/work-item/list/detail/)(?P\d*)`) RegisterAsKnownURL("test-work-item-board-details", `(?Pdemo.openshift.io)(?P/work-item/board/detail/)(?P\d*)`) } @@ -41,8 +43,6 @@ type searchRepositoryWhiteboxTest struct { func (s *searchRepositoryWhiteboxTest) SetupSuite() { s.DBTestSuite.SetupSuite() - // While registering URLs do not include protocol because it will be removed before scanning starts - // Please do not include trailing slashes because it will be removed before scanning starts } func (s *searchRepositoryWhiteboxTest) SetupTest() { @@ -65,8 +65,6 @@ func (s *searchRepositoryWhiteboxTest) setupTestDataSet() ([]SearchTestDescripto fields: map[string]interface{}{ workitem.SystemTitle: "test sbose title '12345678asdfgh'", workitem.SystemDescription: rendering.NewMarkupContentFromLegacy(`"description" for search test`), - workitem.SystemCreator: "sbose78", - workitem.SystemAssignees: []string{"pranav"}, workitem.SystemState: "closed", }, searchString: `Sbose "deScription" '12345678asdfgh' `, @@ -76,8 +74,6 @@ func (s *searchRepositoryWhiteboxTest) setupTestDataSet() ([]SearchTestDescripto fields: map[string]interface{}{ workitem.SystemTitle: "add new error types in models/errors.go'", workitem.SystemDescription: rendering.NewMarkupContentFromLegacy(`Make sure remoteworkitem can access..`), - workitem.SystemCreator: "sbose78", - workitem.SystemAssignees: []string{"pranav"}, workitem.SystemState: "closed", }, searchString: `models/errors.go remoteworkitem `, @@ -87,8 +83,6 @@ func (s *searchRepositoryWhiteboxTest) setupTestDataSet() ([]SearchTestDescripto fields: map[string]interface{}{ workitem.SystemTitle: "test sbose title '12345678asdfgh'", workitem.SystemDescription: rendering.NewMarkupContentFromLegacy(`"description" for search test`), - workitem.SystemCreator: "sbose78", - workitem.SystemAssignees: []string{"pranav"}, workitem.SystemState: "closed", }, searchString: `Sbose "deScription" '12345678asdfgh' `, @@ -97,10 +91,8 @@ func (s *searchRepositoryWhiteboxTest) setupTestDataSet() ([]SearchTestDescripto { // will test behaviour when null fields are present. In this case, "system.description" is nil fields: map[string]interface{}{ - workitem.SystemTitle: "test nofield sbose title '12345678asdfgh'", - workitem.SystemCreator: "sbose78", - workitem.SystemAssignees: []string{"pranav"}, - workitem.SystemState: "closed", + workitem.SystemTitle: "test nofield sbose title '12345678asdfgh'", + workitem.SystemState: "closed", }, searchString: `sbose nofield `, minimumResults: 1, @@ -108,36 +100,30 @@ func (s *searchRepositoryWhiteboxTest) setupTestDataSet() ([]SearchTestDescripto { // will test behaviour when null fields are present. In this case, "system.description" is nil fields: map[string]interface{}{ - workitem.SystemTitle: "test should return 0 results'", - workitem.SystemCreator: "sbose78", - workitem.SystemAssignees: []string{"pranav"}, - workitem.SystemState: "closed", + workitem.SystemTitle: "test should return 0 results'", + workitem.SystemState: "closed", }, searchString: `negative case `, minimumResults: 0, }, { // search stirng with braces should be acceptable case fields: map[string]interface{}{ - workitem.SystemTitle: "Bug reported by administrator for input = (value)", - workitem.SystemCreator: "pgore", - workitem.SystemAssignees: []string{"pranav"}, - workitem.SystemState: "new", + workitem.SystemTitle: "Bug reported by administrator for input = (value)", + workitem.SystemState: "new", }, searchString: `(value) `, minimumResults: 1, }, { // search stirng with surrounding braces should be acceptable case fields: map[string]interface{}{ - workitem.SystemTitle: "trial for braces (pranav) {shoubhik} [aslak]", - workitem.SystemCreator: "pgore", - workitem.SystemAssignees: []string{"pranav"}, - workitem.SystemState: "new", + workitem.SystemTitle: "trial for braces (pranav) {shoubhik} [aslak]", + workitem.SystemState: "new", }, searchString: `(pranav) {shoubhik} [aslak] `, minimumResults: 1, }, } - fxt := tf.NewTestFixture(s.T(), s.DB, tf.Identities(2), tf.WorkItems(len(testDataSet), func(fxt *tf.TestFixture, idx int) error { + fxt := tf.NewTestFixture(s.T(), s.DB, tf.Identities(2, tf.SetIdentityUsernamesFromString([]string{"creator", "assignee"})), tf.WorkItems(len(testDataSet), func(fxt *tf.TestFixture, idx int) error { fxt.WorkItems[idx].SpaceID = fxt.Spaces[0].ID fxt.WorkItems[idx].Type = fxt.WorkItemTypes[0].ID fxt.WorkItems[idx].Fields = testDataSet[idx].fields @@ -274,22 +260,24 @@ func (s *searchRepositoryWhiteboxTest) TestSearchByID() { params := url.Values{} ctx := goa.NewContext(context.Background(), nil, req, params) // create 2 work items, the second one having the number of the first one in its title - fxt, err := tf.NewFixture(s.DB, tf.WorkItems(2, func(fxt *tf.TestFixture, idx int) error { - fxt.WorkItems[idx].Type = workitem.SystemBug - fxt.WorkItems[idx].Fields = map[string]interface{}{ - workitem.SystemTitle: "Search Test Sbose", - workitem.SystemDescription: rendering.NewMarkupContentFromLegacy("Description"), - workitem.SystemCreator: s.modifierID.String(), - workitem.SystemAssignees: []string{"pranav"}, - workitem.SystemState: "closed", - } - fxt.WorkItems[idx].SpaceID = fxt.Spaces[0].ID - // for the second work item, use the number of the first work item - if idx == 1 { - fxt.WorkItems[idx].Fields[workitem.SystemTitle] = "Search Test Sbose" + strconv.Itoa(fxt.WorkItems[0].Number) - } - return nil - })) + fxt, err := tf.NewFixture(s.DB, + tf.Identities(2, tf.SetIdentityUsernamesFromString([]string{"creator", "assignee"})), + tf.WorkItems(2, func(fxt *tf.TestFixture, idx int) error { + fxt.WorkItems[idx].Type = workitem.SystemBug + fxt.WorkItems[idx].Fields = map[string]interface{}{ + workitem.SystemTitle: "Search Test creator", + workitem.SystemDescription: rendering.NewMarkupContentFromLegacy("Description"), + workitem.SystemCreator: fxt.Identities[0].ID.String(), + workitem.SystemAssignees: []string{fxt.Identities[1].ID.String()}, + workitem.SystemState: "closed", + } + fxt.WorkItems[idx].SpaceID = fxt.Spaces[0].ID + // for the second work item, use the number of the first work item + if idx == 1 { + fxt.WorkItems[idx].Fields[workitem.SystemTitle] = "Search Test Sbose" + strconv.Itoa(fxt.WorkItems[0].Number) + } + return nil + })) require.Nil(s.T(), err, "Couldn't create test data") sr := NewGormSearchRepository(s.DB) // when diff --git a/test/testfixture/customize_entity_callbacks.go b/test/testfixture/customize_entity_callbacks.go index 6348edcf39..401b30d57a 100644 --- a/test/testfixture/customize_entity_callbacks.go +++ b/test/testfixture/customize_entity_callbacks.go @@ -191,3 +191,16 @@ func SetWorkItemLinkTypeNames(names []string) CustomizeWorkItemLinkTypeFunc { return nil } } + +// SetIdentityUsernamesFromString takes the given usernames and uses them during creation +// of identities. The length of requested identities and the +// number of usernames must match or the NewFixture call will return an error. +func SetIdentityUsernamesFromString(usernames []string) CustomizeIdentityFunc { + return func(fxt *TestFixture, idx int) error { + if len(fxt.Identities) != len(usernames) { + return errs.Errorf("number of usernames (%d) must match number of identities to create (%d)", len(usernames), len(fxt.Identities)) + } + fxt.Identities[idx].Username = usernames[idx] + return nil + } +} From c74f9b0e3f65de40d01e0d5ed2adda3930c5098f Mon Sep 17 00:00:00 2001 From: Xavier Coulon Date: Mon, 9 Oct 2017 16:56:28 +0200 Subject: [PATCH 09/14] Refactor TestSearch (again) Signed-off-by: Xavier Coulon --- search/search_repository_whitebox_test.go | 438 +++++++++--------- .../testfixture/customize_entity_callbacks.go | 13 - 2 files changed, 222 insertions(+), 229 deletions(-) diff --git a/search/search_repository_whitebox_test.go b/search/search_repository_whitebox_test.go index 63312a123e..b98db79639 100644 --- a/search/search_repository_whitebox_test.go +++ b/search/search_repository_whitebox_test.go @@ -6,7 +6,6 @@ import ( "net/http" "net/url" "regexp" - "strconv" "strings" "testing" @@ -58,190 +57,236 @@ type SearchTestDescriptor struct { minimumResults int } -func (s *searchRepositoryWhiteboxTest) setupTestDataSet() ([]SearchTestDescriptor, *tf.TestFixture) { - // given - testDataSet := []SearchTestDescriptor{ - { - fields: map[string]interface{}{ - workitem.SystemTitle: "test sbose title '12345678asdfgh'", - workitem.SystemDescription: rendering.NewMarkupContentFromLegacy(`"description" for search test`), - workitem.SystemState: "closed", - }, - searchString: `Sbose "deScription" '12345678asdfgh' `, - minimumResults: 1, - }, - { - fields: map[string]interface{}{ - workitem.SystemTitle: "add new error types in models/errors.go'", - workitem.SystemDescription: rendering.NewMarkupContentFromLegacy(`Make sure remoteworkitem can access..`), - workitem.SystemState: "closed", - }, - searchString: `models/errors.go remoteworkitem `, - minimumResults: 1, - }, - { - fields: map[string]interface{}{ - workitem.SystemTitle: "test sbose title '12345678asdfgh'", - workitem.SystemDescription: rendering.NewMarkupContentFromLegacy(`"description" for search test`), - workitem.SystemState: "closed", - }, - searchString: `Sbose "deScription" '12345678asdfgh' `, - minimumResults: 1, - }, - { - // will test behaviour when null fields are present. In this case, "system.description" is nil - fields: map[string]interface{}{ - workitem.SystemTitle: "test nofield sbose title '12345678asdfgh'", - workitem.SystemState: "closed", - }, - searchString: `sbose nofield `, - minimumResults: 1, - }, - { - // will test behaviour when null fields are present. In this case, "system.description" is nil - fields: map[string]interface{}{ - workitem.SystemTitle: "test should return 0 results'", - workitem.SystemState: "closed", - }, - searchString: `negative case `, - minimumResults: 0, - }, { - // search stirng with braces should be acceptable case - fields: map[string]interface{}{ - workitem.SystemTitle: "Bug reported by administrator for input = (value)", - workitem.SystemState: "new", - }, - searchString: `(value) `, - minimumResults: 1, - }, { - // search stirng with surrounding braces should be acceptable case - fields: map[string]interface{}{ - workitem.SystemTitle: "trial for braces (pranav) {shoubhik} [aslak]", - workitem.SystemState: "new", - }, - searchString: `(pranav) {shoubhik} [aslak] `, - minimumResults: 1, - }, - } - fxt := tf.NewTestFixture(s.T(), s.DB, tf.Identities(2, tf.SetIdentityUsernamesFromString([]string{"creator", "assignee"})), tf.WorkItems(len(testDataSet), func(fxt *tf.TestFixture, idx int) error { - fxt.WorkItems[idx].SpaceID = fxt.Spaces[0].ID - fxt.WorkItems[idx].Type = fxt.WorkItemTypes[0].ID - fxt.WorkItems[idx].Fields = testDataSet[idx].fields - fxt.WorkItems[idx].Fields[workitem.SystemCreator] = fxt.Identities[0].ID.String() - fxt.WorkItems[idx].Fields[workitem.SystemAssignees] = []string{fxt.Identities[1].ID.String()} - return nil - })) - return testDataSet, fxt -} - -// TestSearchByText verifies search on title or description -func (s *searchRepositoryWhiteboxTest) TestSearchByText() { - // given - testDataSet, fxt := s.setupTestDataSet() - // - for idx, testData := range testDataSet { - minimumResults := testData.minimumResults - req := &http.Request{Host: "localhost"} - params := url.Values{} - ctx := goa.NewContext(context.Background(), nil, req, params) - // had to dynamically create this since I didn't now the URL/ID of the workitem - // till the test data was created. - searchString := testData.searchString - workItemURLInSearchString := fmt.Sprintf("%s%d", "http://demo.openshift.io/work-item/list/detail/", fxt.WorkItems[idx].Number) - searchString = fmt.Sprintf("\"%s %s\"", searchString, workItemURLInSearchString) - 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, "\"") - s.T().Logf("TestData #%d: using search string: %s -> %d matches", (idx + 1), searchString, len(workItemList)) - // 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) - // These keywords need a match optionally either as URL string or ID + keyWord = strings.ToLower(keyWord) - optionalKeywords := []string{workItemURLInSearchString, strconv.Itoa(fxt.WorkItems[idx].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().Logf("Examining workitem id=`%v` number=`%d` using keywords %v", workItemValue.ID, workItemValue.Number, allKeywords) - for _, keyWord := range allKeywords { - keyWord = strings.ToLower(keyWord) - s.T().Logf("Verifying workitem id=`%v` number=`%d` for keyword `%s`...", workItemValue.ID, workItemValue.Number, keyWord) - workItemTitle := "" - if workItemValue.Fields[workitem.SystemTitle] != nil { - workItemTitle = strings.ToLower(workItemValue.Fields[workitem.SystemTitle].(string)) +func (s *searchRepositoryWhiteboxTest) TestSearch() { + // create 2 work items, the second one having the number of the first one in its title + baseFxt, err := tf.NewFixture(s.DB, tf.Identities(2, tf.SetIdentityUsernames([]string{"creator", "assignee"})), + tf.Spaces(1, func(fxt *tf.TestFixture, idx int) error { + fxt.Spaces[idx].OwnerId = fxt.Identities[0].ID + return nil + }), tf.WorkItemTypes(1)) + require.Nil(s.T(), err) + s.T().Run("Text search", func(t *testing.T) { + + t.Run("Search accross title and description", func(t *testing.T) { + // given + tf.NewTestFixture(t, s.DB, tf.WorkItems(1, func(fxt *tf.TestFixture, idx int) error { + fxt.WorkItems[idx].SpaceID = baseFxt.Spaces[0].ID + fxt.WorkItems[idx].Type = baseFxt.WorkItemTypes[0].ID + fxt.WorkItems[idx].Fields = map[string]interface{}{ + workitem.SystemTitle: "test sbose title '12345678asdfgh'", + workitem.SystemDescription: rendering.NewMarkupContentFromLegacy(`"description" for search test`), + workitem.SystemState: "closed", + workitem.SystemCreator: baseFxt.Identities[0].ID.String(), + workitem.SystemAssignees: []string{baseFxt.Identities[1].ID.String()}, } - workItemDescription := "" - if workItemValue.Fields[workitem.SystemDescription] != nil { - descriptionField := workItemValue.Fields[workitem.SystemDescription].(rendering.MarkupContent) - workItemDescription = strings.ToLower(descriptionField.Content) + return nil + })) + // when + searchQuery := `Sbose "deScription" '12345678asdfgh'` + searchResults, err := s.searchFor(baseFxt.Spaces[0].ID, searchQuery) + // then + require.Nil(t, err) + verify(t, searchQuery, searchResults, 1) + }) + + t.Run("Search accross title and description with slash", func(t *testing.T) { + // given + tf.NewTestFixture(t, s.DB, tf.WorkItems(1, func(fxt *tf.TestFixture, idx int) error { + fxt.WorkItems[idx].SpaceID = baseFxt.Spaces[0].ID + fxt.WorkItems[idx].Type = baseFxt.WorkItemTypes[0].ID + fxt.WorkItems[idx].Fields = map[string]interface{}{ + workitem.SystemTitle: "add new error types in models/errors.go'", + workitem.SystemDescription: rendering.NewMarkupContentFromLegacy(`Make sure remoteworkitem can access..`), + workitem.SystemState: "closed", + workitem.SystemCreator: baseFxt.Identities[0].ID.String(), + workitem.SystemAssignees: []string{baseFxt.Identities[1].ID.String()}, } - assert.True(s.T(), - strings.Contains(workItemTitle, keyWord) || strings.Contains(workItemDescription, keyWord) || - (stringInSlice(keyWord, optionalKeywords) && strings.Contains(keyWord, strconv.Itoa(workItemValue.Number))), - "`%s` neither found in title `%s` nor in the description `%s` for workitem #%d", keyWord, workItemTitle, workItemDescription, workItemValue.Number) - } - } + return nil + })) + // when + searchQuery := `models/errors.go remoteworkitem` + searchResults, err := s.searchFor(baseFxt.Spaces[0].ID, searchQuery) + // then + require.Nil(t, err) + verify(t, searchQuery, searchResults, 1) + }) + + t.Run("Search accross title and description with braces", func(t *testing.T) { + // given + tf.NewTestFixture(t, s.DB, tf.WorkItems(1, func(fxt *tf.TestFixture, idx int) error { + fxt.WorkItems[idx].SpaceID = baseFxt.Spaces[0].ID + fxt.WorkItems[idx].Type = baseFxt.WorkItemTypes[0].ID + fxt.WorkItems[idx].Fields = map[string]interface{}{ + workitem.SystemTitle: "Bug reported by administrator for input = (value)", + workitem.SystemState: "new", + workitem.SystemCreator: baseFxt.Identities[0].ID.String(), + workitem.SystemAssignees: []string{baseFxt.Identities[1].ID.String()}, + } + return nil + })) + // when + searchQuery := `(value)` + searchResults, err := s.searchFor(baseFxt.Spaces[0].ID, searchQuery) + // then + require.Nil(t, err) + verify(t, searchQuery, searchResults, 1) + + }) + + t.Run("Search accross title and description with braces and brackets", func(t *testing.T) { + // given + tf.NewTestFixture(t, s.DB, tf.WorkItems(1, func(fxt *tf.TestFixture, idx int) error { + fxt.WorkItems[idx].SpaceID = baseFxt.Spaces[0].ID + fxt.WorkItems[idx].Type = baseFxt.WorkItemTypes[0].ID + fxt.WorkItems[idx].Fields = map[string]interface{}{ + workitem.SystemTitle: "trial for braces (pranav) {shoubhik} [aslak]", + workitem.SystemState: "new", + workitem.SystemCreator: baseFxt.Identities[0].ID.String(), + workitem.SystemAssignees: []string{baseFxt.Identities[1].ID.String()}, + } + return nil + })) + // when + searchQuery := `(pranav) {shoubhik} [aslak]` + searchResults, err := s.searchFor(baseFxt.Spaces[0].ID, searchQuery) + // then + require.Nil(t, err) + verify(t, searchQuery, searchResults, 1) + + }) + t.Run("Search accross title and description undefined", func(t *testing.T) { + // given + tf.NewTestFixture(t, s.DB, tf.WorkItems(1, func(fxt *tf.TestFixture, idx int) error { + fxt.WorkItems[idx].SpaceID = baseFxt.Spaces[0].ID + fxt.WorkItems[idx].Type = baseFxt.WorkItemTypes[0].ID + fxt.WorkItems[idx].Fields = map[string]interface{}{ + workitem.SystemTitle: "test nofield sbose title '12345678asdfgh'", + workitem.SystemState: "closed", + workitem.SystemCreator: baseFxt.Identities[0].ID.String(), + workitem.SystemAssignees: []string{baseFxt.Identities[1].ID.String()}, + } + return nil + })) + // when + searchQuery := `sbose nofield` + searchResults, err := s.searchFor(baseFxt.Spaces[0].ID, searchQuery) + // then + require.Nil(t, err) + verify(t, searchQuery, searchResults, 1) + }) + + t.Run("Search accross title and description undefined and no match", func(t *testing.T) { + // given + tf.NewTestFixture(t, s.DB, tf.WorkItems(1, func(fxt *tf.TestFixture, idx int) error { + fxt.WorkItems[idx].SpaceID = baseFxt.Spaces[0].ID + fxt.WorkItems[idx].Type = baseFxt.WorkItemTypes[0].ID + fxt.WorkItems[idx].Fields = map[string]interface{}{ + workitem.SystemTitle: "test should return 0 results'", + workitem.SystemState: "closed", + workitem.SystemCreator: baseFxt.Identities[0].ID.String(), + workitem.SystemAssignees: []string{baseFxt.Identities[1].ID.String()}, + } + return nil + })) + // when + searchQuery := `negative case` + searchResults, err := s.searchFor(baseFxt.Spaces[0].ID, searchQuery) + // then + require.Nil(t, err) + verify(t, searchQuery, searchResults, 0) + }) + + t.Run("Search by number", func(t *testing.T) { + // given + fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(1, func(fxt *tf.TestFixture, idx int) error { + fxt.WorkItems[idx].SpaceID = baseFxt.Spaces[0].ID + fxt.WorkItems[idx].Type = baseFxt.WorkItemTypes[0].ID + fxt.WorkItems[idx].Fields = map[string]interface{}{ + workitem.SystemTitle: "test nofield sbose title '12345678asdfgh'", + workitem.SystemState: "closed", + workitem.SystemCreator: baseFxt.Identities[0].ID.String(), + workitem.SystemAssignees: []string{baseFxt.Identities[1].ID.String()}, + } + return nil + })) + // when + searchQuery := fmt.Sprintf("number:%d", fxt.WorkItems[0].Number) + searchResults, err := s.searchFor(baseFxt.Spaces[0].ID, searchQuery) + // then + require.Nil(t, err) + require.Len(t, searchResults, 1) + assert.Equal(t, fxt.WorkItems[0].ID, searchResults[0].ID) + }) + + t.Run("Search by URL", func(t *testing.T) { + // given + fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(1, func(fxt *tf.TestFixture, idx int) error { + fxt.WorkItems[idx].SpaceID = baseFxt.Spaces[0].ID + fxt.WorkItems[idx].Type = baseFxt.WorkItemTypes[0].ID + fxt.WorkItems[idx].Fields = map[string]interface{}{ + workitem.SystemTitle: "test nofield sbose title '12345678asdfgh'", + workitem.SystemState: "closed", + workitem.SystemCreator: baseFxt.Identities[0].ID.String(), + workitem.SystemAssignees: []string{baseFxt.Identities[1].ID.String()}, + } + return nil + })) + // when + searchQuery := fmt.Sprintf("%s%d", "http://demo.openshift.io/work-item/list/detail/", fxt.WorkItems[0].Number) + searchResults, err := s.searchFor(baseFxt.Spaces[0].ID, searchQuery) + // then + require.Nil(t, err) + require.Len(t, searchResults, 1) + assert.Equal(t, fxt.WorkItems[0].ID, searchResults[0].ID) + }) + }) - } } -// TestSearchByText verifies search on number -func (s *searchRepositoryWhiteboxTest) TestSearchByNumber() { - // given - testDataSet, fxt := s.setupTestDataSet() - // - for idx, testData := range testDataSet { - number := fxt.WorkItems[idx].Number - minimumResults := testData.minimumResults - req := &http.Request{Host: "localhost"} - params := url.Values{} - ctx := goa.NewContext(context.Background(), nil, req, params) - - // had to dynamically create this since I didn't now the URL/ID of the workitem - // till the test data was created. - searchString := fmt.Sprintf("\"number:%d\"", number) - 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, "\"") - s.T().Logf("TestData #%d: using search string: %s -> %d matches", idx, searchString, len(workItemList)) - // 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) - } +func (s *searchRepositoryWhiteboxTest) searchFor(spaceID uuid.UUID, searchQuery string) ([]workitem.WorkItem, error) { + req := &http.Request{Host: "localhost"} + params := url.Values{} + ctx := goa.NewContext(context.Background(), nil, req, params) + sr := NewGormSearchRepository(s.DB) + var start, limit int = 0, 100 + spaceIDStr := spaceID.String() + workItemList, _, err := sr.SearchFullText(ctx, fmt.Sprintf("\"%s\"", searchQuery), &start, &limit, &spaceIDStr) + return workItemList, err +} - // These keywords need a match in the textual part. - allKeywords := strings.Fields(searchString) - //[]string{createdWorkItem.ID, `"Sbose"`, `"deScription"`, `'12345678asdfgh'`} - - // 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().Logf("Examining workitem id=`%v` number=`%d` using keywords %v", workItemValue.ID, workItemValue.Number, allKeywords) - for _, keyWord := range allKeywords { - keyWord = strings.ToLower(keyWord) - s.T().Logf("Verifying workitem id=`%v` number=`%d` for keyword `%s`...", workItemValue.ID, workItemValue.Number, keyWord) - assert.Equal(s.T(), number, workItemValue.Number, - "workitem #%d did not have the expected number", workItemValue.Number, number) +func verify(t *testing.T, searchQuery string, searchResults []workitem.WorkItem, expectedCount int) { + // Since this test adds test data, whether or not other workitems exist + // there must be at least 1 search result returned. + if len(searchResults) == expectedCount && expectedCount == 0 { + // no point checking further, we got what we wanted. + return + } + require.Equal(t, expectedCount, len(searchResults), "invalid number of results in the search") + + // These keywords need a match in the textual part. + allKeywords := strings.Fields(searchQuery) + // These keywords need a match optionally either as URL string or ID + keyWord = strings.ToLower(keyWord) + // optionalKeywords := []string{workItemURLInSearchString, strconv.Itoa(fxt.WorkItems[idx].Number)} + // We will now check the legitimacy of the search results. + // Iterate through all search results and see whether they meet the criteria + for _, searchResult := range searchResults { + t.Logf("Examining workitem id=`%v` number=`%d` using keywords %v", searchResult.ID, searchResult.Number, allKeywords) + for _, keyWord := range allKeywords { + keyWord = strings.ToLower(keyWord) + t.Logf("Verifying workitem id=`%v` number=`%d` for keyword `%s`...", searchResult.ID, searchResult.Number, keyWord) + workItemTitle := "" + if searchResult.Fields[workitem.SystemTitle] != nil { + workItemTitle = strings.ToLower(searchResult.Fields[workitem.SystemTitle].(string)) + } + workItemDescription := "" + if searchResult.Fields[workitem.SystemDescription] != nil { + descriptionField := searchResult.Fields[workitem.SystemDescription].(rendering.MarkupContent) + workItemDescription = strings.ToLower(descriptionField.Content) } + assert.True(t, + strings.Contains(workItemTitle, keyWord) || strings.Contains(workItemDescription, keyWord), + "`%s` neither found in title `%s` nor in the description `%s` for workitem #%d", keyWord, workItemTitle, workItemDescription, searchResult.Number) } - } } @@ -254,45 +299,6 @@ func stringInSlice(str string, list []string) bool { return false } -func (s *searchRepositoryWhiteboxTest) TestSearchByID() { - // given - req := &http.Request{Host: "localhost"} - params := url.Values{} - ctx := goa.NewContext(context.Background(), nil, req, params) - // create 2 work items, the second one having the number of the first one in its title - fxt, err := tf.NewFixture(s.DB, - tf.Identities(2, tf.SetIdentityUsernamesFromString([]string{"creator", "assignee"})), - tf.WorkItems(2, func(fxt *tf.TestFixture, idx int) error { - fxt.WorkItems[idx].Type = workitem.SystemBug - fxt.WorkItems[idx].Fields = map[string]interface{}{ - workitem.SystemTitle: "Search Test creator", - workitem.SystemDescription: rendering.NewMarkupContentFromLegacy("Description"), - workitem.SystemCreator: fxt.Identities[0].ID.String(), - workitem.SystemAssignees: []string{fxt.Identities[1].ID.String()}, - workitem.SystemState: "closed", - } - fxt.WorkItems[idx].SpaceID = fxt.Spaces[0].ID - // for the second work item, use the number of the first work item - if idx == 1 { - fxt.WorkItems[idx].Fields[workitem.SystemTitle] = "Search Test Sbose" + strconv.Itoa(fxt.WorkItems[0].Number) - } - return nil - })) - require.Nil(s.T(), err, "Couldn't create test data") - sr := NewGormSearchRepository(s.DB) - // when - var start, limit int = 0, 100 - searchString := "number:" + strconv.Itoa(fxt.WorkItems[0].Number) - spaceID := fxt.Spaces[0].ID.String() // make sure the search is limited to the space to avoid collision with other existing data - workItemList, _, err := sr.SearchFullText(ctx, searchString, &start, &limit, &spaceID) - // then - require.Nil(s.T(), err) - // Number is unique, hence search result set's length should be 1 - require.Equal(s.T(), len(workItemList), 1) - s.T().Log("Found search result for ID Search ", workItemList[0].ID) - assert.Equal(s.T(), fxt.WorkItems[0].ID, workItemList[0].ID) -} - func TestGenerateSQLSearchStringText(t *testing.T) { t.Parallel() resource.Require(t, resource.UnitTest) diff --git a/test/testfixture/customize_entity_callbacks.go b/test/testfixture/customize_entity_callbacks.go index 401b30d57a..6348edcf39 100644 --- a/test/testfixture/customize_entity_callbacks.go +++ b/test/testfixture/customize_entity_callbacks.go @@ -191,16 +191,3 @@ func SetWorkItemLinkTypeNames(names []string) CustomizeWorkItemLinkTypeFunc { return nil } } - -// SetIdentityUsernamesFromString takes the given usernames and uses them during creation -// of identities. The length of requested identities and the -// number of usernames must match or the NewFixture call will return an error. -func SetIdentityUsernamesFromString(usernames []string) CustomizeIdentityFunc { - return func(fxt *TestFixture, idx int) error { - if len(fxt.Identities) != len(usernames) { - return errs.Errorf("number of usernames (%d) must match number of identities to create (%d)", len(usernames), len(fxt.Identities)) - } - fxt.Identities[idx].Username = usernames[idx] - return nil - } -} From 15a49f89f29f34b3b8d579f43597d422fc2ec354 Mon Sep 17 00:00:00 2001 From: Xavier Coulon Date: Mon, 9 Oct 2017 17:11:34 +0200 Subject: [PATCH 10/14] Using constants for work item states Signed-off-by: Xavier Coulon --- search/search_repository_whitebox_test.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/search/search_repository_whitebox_test.go b/search/search_repository_whitebox_test.go index b98db79639..25b05dcdc6 100644 --- a/search/search_repository_whitebox_test.go +++ b/search/search_repository_whitebox_test.go @@ -75,7 +75,7 @@ func (s *searchRepositoryWhiteboxTest) TestSearch() { fxt.WorkItems[idx].Fields = map[string]interface{}{ workitem.SystemTitle: "test sbose title '12345678asdfgh'", workitem.SystemDescription: rendering.NewMarkupContentFromLegacy(`"description" for search test`), - workitem.SystemState: "closed", + workitem.SystemState: workitem.SystemStateClosed, workitem.SystemCreator: baseFxt.Identities[0].ID.String(), workitem.SystemAssignees: []string{baseFxt.Identities[1].ID.String()}, } @@ -97,7 +97,7 @@ func (s *searchRepositoryWhiteboxTest) TestSearch() { fxt.WorkItems[idx].Fields = map[string]interface{}{ workitem.SystemTitle: "add new error types in models/errors.go'", workitem.SystemDescription: rendering.NewMarkupContentFromLegacy(`Make sure remoteworkitem can access..`), - workitem.SystemState: "closed", + workitem.SystemState: workitem.SystemStateClosed, workitem.SystemCreator: baseFxt.Identities[0].ID.String(), workitem.SystemAssignees: []string{baseFxt.Identities[1].ID.String()}, } @@ -118,7 +118,7 @@ func (s *searchRepositoryWhiteboxTest) TestSearch() { fxt.WorkItems[idx].Type = baseFxt.WorkItemTypes[0].ID fxt.WorkItems[idx].Fields = map[string]interface{}{ workitem.SystemTitle: "Bug reported by administrator for input = (value)", - workitem.SystemState: "new", + workitem.SystemState: workitem.SystemStateNew, workitem.SystemCreator: baseFxt.Identities[0].ID.String(), workitem.SystemAssignees: []string{baseFxt.Identities[1].ID.String()}, } @@ -140,7 +140,7 @@ func (s *searchRepositoryWhiteboxTest) TestSearch() { fxt.WorkItems[idx].Type = baseFxt.WorkItemTypes[0].ID fxt.WorkItems[idx].Fields = map[string]interface{}{ workitem.SystemTitle: "trial for braces (pranav) {shoubhik} [aslak]", - workitem.SystemState: "new", + workitem.SystemState: workitem.SystemStateNew, workitem.SystemCreator: baseFxt.Identities[0].ID.String(), workitem.SystemAssignees: []string{baseFxt.Identities[1].ID.String()}, } @@ -161,7 +161,7 @@ func (s *searchRepositoryWhiteboxTest) TestSearch() { fxt.WorkItems[idx].Type = baseFxt.WorkItemTypes[0].ID fxt.WorkItems[idx].Fields = map[string]interface{}{ workitem.SystemTitle: "test nofield sbose title '12345678asdfgh'", - workitem.SystemState: "closed", + workitem.SystemState: workitem.SystemStateClosed, workitem.SystemCreator: baseFxt.Identities[0].ID.String(), workitem.SystemAssignees: []string{baseFxt.Identities[1].ID.String()}, } @@ -182,7 +182,7 @@ func (s *searchRepositoryWhiteboxTest) TestSearch() { fxt.WorkItems[idx].Type = baseFxt.WorkItemTypes[0].ID fxt.WorkItems[idx].Fields = map[string]interface{}{ workitem.SystemTitle: "test should return 0 results'", - workitem.SystemState: "closed", + workitem.SystemState: workitem.SystemStateClosed, workitem.SystemCreator: baseFxt.Identities[0].ID.String(), workitem.SystemAssignees: []string{baseFxt.Identities[1].ID.String()}, } @@ -203,7 +203,7 @@ func (s *searchRepositoryWhiteboxTest) TestSearch() { fxt.WorkItems[idx].Type = baseFxt.WorkItemTypes[0].ID fxt.WorkItems[idx].Fields = map[string]interface{}{ workitem.SystemTitle: "test nofield sbose title '12345678asdfgh'", - workitem.SystemState: "closed", + workitem.SystemState: workitem.SystemStateClosed, workitem.SystemCreator: baseFxt.Identities[0].ID.String(), workitem.SystemAssignees: []string{baseFxt.Identities[1].ID.String()}, } @@ -225,7 +225,7 @@ func (s *searchRepositoryWhiteboxTest) TestSearch() { fxt.WorkItems[idx].Type = baseFxt.WorkItemTypes[0].ID fxt.WorkItems[idx].Fields = map[string]interface{}{ workitem.SystemTitle: "test nofield sbose title '12345678asdfgh'", - workitem.SystemState: "closed", + workitem.SystemState: workitem.SystemStateClosed, workitem.SystemCreator: baseFxt.Identities[0].ID.String(), workitem.SystemAssignees: []string{baseFxt.Identities[1].ID.String()}, } From 9df5af22af9a68897ca7ec0e7c02658b9629d217 Mon Sep 17 00:00:00 2001 From: Xavier Coulon Date: Tue, 10 Oct 2017 09:51:30 +0200 Subject: [PATCH 11/14] Simplify tests Remove the `baseFxt`, and let each work item be created with the default TestFixture settings (ie, each one in its own space with its own creator and state) Signed-off-by: Xavier Coulon --- search/search_repository_whitebox_test.go | 296 +++++++++------------- test/testfixture/make_functions.go | 4 +- 2 files changed, 118 insertions(+), 182 deletions(-) diff --git a/search/search_repository_whitebox_test.go b/search/search_repository_whitebox_test.go index 25b05dcdc6..64d8d1317a 100644 --- a/search/search_repository_whitebox_test.go +++ b/search/search_repository_whitebox_test.go @@ -58,187 +58,123 @@ type SearchTestDescriptor struct { } func (s *searchRepositoryWhiteboxTest) TestSearch() { - // create 2 work items, the second one having the number of the first one in its title - baseFxt, err := tf.NewFixture(s.DB, tf.Identities(2, tf.SetIdentityUsernames([]string{"creator", "assignee"})), - tf.Spaces(1, func(fxt *tf.TestFixture, idx int) error { - fxt.Spaces[idx].OwnerId = fxt.Identities[0].ID + + s.T().Run("Search accross title and description", func(t *testing.T) { + // given + fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(1, func(fxt *tf.TestFixture, idx int) error { + fxt.WorkItems[idx].Fields[workitem.SystemTitle] = "test sbose title '12345678asdfgh'" + fxt.WorkItems[idx].Fields[workitem.SystemDescription] = rendering.NewMarkupContentFromLegacy(`"description" for search test`) return nil - }), tf.WorkItemTypes(1)) - require.Nil(s.T(), err) - s.T().Run("Text search", func(t *testing.T) { - - t.Run("Search accross title and description", func(t *testing.T) { - // given - tf.NewTestFixture(t, s.DB, tf.WorkItems(1, func(fxt *tf.TestFixture, idx int) error { - fxt.WorkItems[idx].SpaceID = baseFxt.Spaces[0].ID - fxt.WorkItems[idx].Type = baseFxt.WorkItemTypes[0].ID - fxt.WorkItems[idx].Fields = map[string]interface{}{ - workitem.SystemTitle: "test sbose title '12345678asdfgh'", - workitem.SystemDescription: rendering.NewMarkupContentFromLegacy(`"description" for search test`), - workitem.SystemState: workitem.SystemStateClosed, - workitem.SystemCreator: baseFxt.Identities[0].ID.String(), - workitem.SystemAssignees: []string{baseFxt.Identities[1].ID.String()}, - } - return nil - })) - // when - searchQuery := `Sbose "deScription" '12345678asdfgh'` - searchResults, err := s.searchFor(baseFxt.Spaces[0].ID, searchQuery) - // then - require.Nil(t, err) - verify(t, searchQuery, searchResults, 1) - }) - - t.Run("Search accross title and description with slash", func(t *testing.T) { - // given - tf.NewTestFixture(t, s.DB, tf.WorkItems(1, func(fxt *tf.TestFixture, idx int) error { - fxt.WorkItems[idx].SpaceID = baseFxt.Spaces[0].ID - fxt.WorkItems[idx].Type = baseFxt.WorkItemTypes[0].ID - fxt.WorkItems[idx].Fields = map[string]interface{}{ - workitem.SystemTitle: "add new error types in models/errors.go'", - workitem.SystemDescription: rendering.NewMarkupContentFromLegacy(`Make sure remoteworkitem can access..`), - workitem.SystemState: workitem.SystemStateClosed, - workitem.SystemCreator: baseFxt.Identities[0].ID.String(), - workitem.SystemAssignees: []string{baseFxt.Identities[1].ID.String()}, - } - return nil - })) - // when - searchQuery := `models/errors.go remoteworkitem` - searchResults, err := s.searchFor(baseFxt.Spaces[0].ID, searchQuery) - // then - require.Nil(t, err) - verify(t, searchQuery, searchResults, 1) - }) - - t.Run("Search accross title and description with braces", func(t *testing.T) { - // given - tf.NewTestFixture(t, s.DB, tf.WorkItems(1, func(fxt *tf.TestFixture, idx int) error { - fxt.WorkItems[idx].SpaceID = baseFxt.Spaces[0].ID - fxt.WorkItems[idx].Type = baseFxt.WorkItemTypes[0].ID - fxt.WorkItems[idx].Fields = map[string]interface{}{ - workitem.SystemTitle: "Bug reported by administrator for input = (value)", - workitem.SystemState: workitem.SystemStateNew, - workitem.SystemCreator: baseFxt.Identities[0].ID.String(), - workitem.SystemAssignees: []string{baseFxt.Identities[1].ID.String()}, - } - return nil - })) - // when - searchQuery := `(value)` - searchResults, err := s.searchFor(baseFxt.Spaces[0].ID, searchQuery) - // then - require.Nil(t, err) - verify(t, searchQuery, searchResults, 1) - - }) - - t.Run("Search accross title and description with braces and brackets", func(t *testing.T) { - // given - tf.NewTestFixture(t, s.DB, tf.WorkItems(1, func(fxt *tf.TestFixture, idx int) error { - fxt.WorkItems[idx].SpaceID = baseFxt.Spaces[0].ID - fxt.WorkItems[idx].Type = baseFxt.WorkItemTypes[0].ID - fxt.WorkItems[idx].Fields = map[string]interface{}{ - workitem.SystemTitle: "trial for braces (pranav) {shoubhik} [aslak]", - workitem.SystemState: workitem.SystemStateNew, - workitem.SystemCreator: baseFxt.Identities[0].ID.String(), - workitem.SystemAssignees: []string{baseFxt.Identities[1].ID.String()}, - } - return nil - })) - // when - searchQuery := `(pranav) {shoubhik} [aslak]` - searchResults, err := s.searchFor(baseFxt.Spaces[0].ID, searchQuery) - // then - require.Nil(t, err) - verify(t, searchQuery, searchResults, 1) - - }) - t.Run("Search accross title and description undefined", func(t *testing.T) { - // given - tf.NewTestFixture(t, s.DB, tf.WorkItems(1, func(fxt *tf.TestFixture, idx int) error { - fxt.WorkItems[idx].SpaceID = baseFxt.Spaces[0].ID - fxt.WorkItems[idx].Type = baseFxt.WorkItemTypes[0].ID - fxt.WorkItems[idx].Fields = map[string]interface{}{ - workitem.SystemTitle: "test nofield sbose title '12345678asdfgh'", - workitem.SystemState: workitem.SystemStateClosed, - workitem.SystemCreator: baseFxt.Identities[0].ID.String(), - workitem.SystemAssignees: []string{baseFxt.Identities[1].ID.String()}, - } - return nil - })) - // when - searchQuery := `sbose nofield` - searchResults, err := s.searchFor(baseFxt.Spaces[0].ID, searchQuery) - // then - require.Nil(t, err) - verify(t, searchQuery, searchResults, 1) - }) - - t.Run("Search accross title and description undefined and no match", func(t *testing.T) { - // given - tf.NewTestFixture(t, s.DB, tf.WorkItems(1, func(fxt *tf.TestFixture, idx int) error { - fxt.WorkItems[idx].SpaceID = baseFxt.Spaces[0].ID - fxt.WorkItems[idx].Type = baseFxt.WorkItemTypes[0].ID - fxt.WorkItems[idx].Fields = map[string]interface{}{ - workitem.SystemTitle: "test should return 0 results'", - workitem.SystemState: workitem.SystemStateClosed, - workitem.SystemCreator: baseFxt.Identities[0].ID.String(), - workitem.SystemAssignees: []string{baseFxt.Identities[1].ID.String()}, - } - return nil - })) - // when - searchQuery := `negative case` - searchResults, err := s.searchFor(baseFxt.Spaces[0].ID, searchQuery) - // then - require.Nil(t, err) - verify(t, searchQuery, searchResults, 0) - }) - - t.Run("Search by number", func(t *testing.T) { - // given - fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(1, func(fxt *tf.TestFixture, idx int) error { - fxt.WorkItems[idx].SpaceID = baseFxt.Spaces[0].ID - fxt.WorkItems[idx].Type = baseFxt.WorkItemTypes[0].ID - fxt.WorkItems[idx].Fields = map[string]interface{}{ - workitem.SystemTitle: "test nofield sbose title '12345678asdfgh'", - workitem.SystemState: workitem.SystemStateClosed, - workitem.SystemCreator: baseFxt.Identities[0].ID.String(), - workitem.SystemAssignees: []string{baseFxt.Identities[1].ID.String()}, - } - return nil - })) - // when - searchQuery := fmt.Sprintf("number:%d", fxt.WorkItems[0].Number) - searchResults, err := s.searchFor(baseFxt.Spaces[0].ID, searchQuery) - // then - require.Nil(t, err) - require.Len(t, searchResults, 1) - assert.Equal(t, fxt.WorkItems[0].ID, searchResults[0].ID) - }) - - t.Run("Search by URL", func(t *testing.T) { - // given - fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(1, func(fxt *tf.TestFixture, idx int) error { - fxt.WorkItems[idx].SpaceID = baseFxt.Spaces[0].ID - fxt.WorkItems[idx].Type = baseFxt.WorkItemTypes[0].ID - fxt.WorkItems[idx].Fields = map[string]interface{}{ - workitem.SystemTitle: "test nofield sbose title '12345678asdfgh'", - workitem.SystemState: workitem.SystemStateClosed, - workitem.SystemCreator: baseFxt.Identities[0].ID.String(), - workitem.SystemAssignees: []string{baseFxt.Identities[1].ID.String()}, - } - return nil - })) - // when - searchQuery := fmt.Sprintf("%s%d", "http://demo.openshift.io/work-item/list/detail/", fxt.WorkItems[0].Number) - searchResults, err := s.searchFor(baseFxt.Spaces[0].ID, searchQuery) - // then - require.Nil(t, err) - require.Len(t, searchResults, 1) - assert.Equal(t, fxt.WorkItems[0].ID, searchResults[0].ID) - }) + })) + // when + searchQuery := `Sbose "deScription" '12345678asdfgh'` + searchResults, err := s.searchFor(fxt.Spaces[0].ID, searchQuery) + // then + require.Nil(t, err) + verify(t, searchQuery, searchResults, 1) + }) + + s.T().Run("Search accross title and description undefined", func(t *testing.T) { + // given + fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(1, func(fxt *tf.TestFixture, idx int) error { + fxt.WorkItems[idx].Fields[workitem.SystemTitle] = "test nofield sbose title '12345678asdfgh'" + return nil + })) + // when + searchQuery := `sbose nofield` + searchResults, err := s.searchFor(fxt.Spaces[0].ID, searchQuery) + // then + require.Nil(t, err) + verify(t, searchQuery, searchResults, 1) + }) + + s.T().Run("Search accross title with slash", func(t *testing.T) { + // given + fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(1, func(fxt *tf.TestFixture, idx int) error { + fxt.WorkItems[idx].Fields[workitem.SystemTitle] = "add new error types in models/errors.go'" + fxt.WorkItems[idx].Fields[workitem.SystemDescription] = rendering.NewMarkupContentFromLegacy(`Make sure remoteworkitem can access..`) + return nil + })) + // when + searchQuery := `models/errors.go remoteworkitem` + searchResults, err := s.searchFor(fxt.Spaces[0].ID, searchQuery) + // then + require.Nil(t, err) + verify(t, searchQuery, searchResults, 1) + }) + + s.T().Run("Search accross title with braces", func(t *testing.T) { + // given + fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(1, func(fxt *tf.TestFixture, idx int) error { + fxt.WorkItems[idx].Fields[workitem.SystemTitle] = "Bug reported by administrator for input = (value)" + return nil + })) + // when + searchQuery := `(value)` + searchResults, err := s.searchFor(fxt.Spaces[0].ID, searchQuery) + // then + require.Nil(t, err) + verify(t, searchQuery, searchResults, 1) + + }) + + s.T().Run("Search accross title with braces and brackets", func(t *testing.T) { + // given + fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(1, func(fxt *tf.TestFixture, idx int) error { + fxt.WorkItems[idx].Fields[workitem.SystemTitle] = "trial for braces (pranav) {shoubhik} [aslak]" + return nil + })) + // when + searchQuery := `(pranav) {shoubhik} [aslak]` + searchResults, err := s.searchFor(fxt.Spaces[0].ID, searchQuery) + // then + require.Nil(t, err) + verify(t, searchQuery, searchResults, 1) + + }) + + s.T().Run("Search accross title and description undefined and no match", func(t *testing.T) { + // given + fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(1, func(fxt *tf.TestFixture, idx int) error { + fxt.WorkItems[idx].Fields[workitem.SystemTitle] = "test should return 0 results'" + return nil + })) + // when + searchQuery := `negative case` + searchResults, err := s.searchFor(fxt.Spaces[0].ID, searchQuery) + // then + require.Nil(t, err) + verify(t, searchQuery, searchResults, 0) + }) + + s.T().Run("Search by number", func(t *testing.T) { + // given + fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(1, func(fxt *tf.TestFixture, idx int) error { + fxt.WorkItems[idx].Fields[workitem.SystemTitle] = "test nofield sbose title '12345678asdfgh'" + return nil + })) + // when + searchQuery := fmt.Sprintf("number:%d", fxt.WorkItems[0].Number) + searchResults, err := s.searchFor(fxt.Spaces[0].ID, searchQuery) + // then + require.Nil(t, err) + require.Len(t, searchResults, 1) + assert.Equal(t, fxt.WorkItems[0].ID, searchResults[0].ID) + }) + + s.T().Run("Search by URL", func(t *testing.T) { + // given + fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(1, func(fxt *tf.TestFixture, idx int) error { + fxt.WorkItems[idx].Fields[workitem.SystemTitle] = "test nofield sbose title '12345678asdfgh'" + return nil + })) + // when + searchQuery := fmt.Sprintf("%s%d", "http://demo.openshift.io/work-item/list/detail/", fxt.WorkItems[0].Number) + searchResults, err := s.searchFor(fxt.Spaces[0].ID, searchQuery) + // then + require.Nil(t, err) + require.Len(t, searchResults, 1) + assert.Equal(t, fxt.WorkItems[0].ID, searchResults[0].ID) }) } diff --git a/test/testfixture/make_functions.go b/test/testfixture/make_functions.go index 4ad73d7e67..1efa5a77f5 100644 --- a/test/testfixture/make_functions.go +++ b/test/testfixture/make_functions.go @@ -339,11 +339,11 @@ func makeWorkItems(fxt *TestFixture) error { } creatorIDStr, ok := fxt.WorkItems[i].Fields[workitem.SystemCreator].(string) if !ok { - return errs.Errorf("failed to convert \"%s\" field to string in %+v", workitem.SystemCreator, fxt.WorkItems[i].Fields) + return errs.Errorf("failed to convert \"%s\" field to string in %+v: %v", workitem.SystemCreator, fxt.WorkItems[i].Fields, fxt.WorkItems[i].Fields[workitem.SystemCreator]) } creatorID, err := uuid.FromString(creatorIDStr) if err != nil { - return errs.Wrapf(err, "failed to convert \"%s\" field to uuid.UUID", workitem.SystemCreator) + return errs.Wrapf(err, "failed to convert \"%s\" field to uuid.UUID: %v", workitem.SystemCreator, fxt.WorkItems[i].Fields[workitem.SystemCreator]) } wi, err := wiRepo.Create(fxt.ctx, fxt.WorkItems[i].SpaceID, fxt.WorkItems[i].Type, fxt.WorkItems[i].Fields, creatorID) From fb7379e6cde895fc2bde6f3b8f7cfd9e3885377d Mon Sep 17 00:00:00 2001 From: Xavier Coulon Date: Tue, 10 Oct 2017 18:29:56 +0200 Subject: [PATCH 12/14] More review comments Signed-off-by: Xavier Coulon --- search/search_repository_whitebox_test.go | 50 ++++++------- ...rk_item_number_sequence_repository_test.go | 75 +++++++++++++++++++ .../workitem_number_sequence.go | 2 +- workitem/workitem_repository.go | 8 +- workitem/workitem_repository_blackbox_test.go | 35 +++------ 5 files changed, 114 insertions(+), 56 deletions(-) create mode 100644 workitem/number_sequence/work_item_number_sequence_repository_test.go diff --git a/search/search_repository_whitebox_test.go b/search/search_repository_whitebox_test.go index 64d8d1317a..4cd50801e7 100644 --- a/search/search_repository_whitebox_test.go +++ b/search/search_repository_whitebox_test.go @@ -3,8 +3,6 @@ package search import ( "context" "fmt" - "net/http" - "net/url" "regexp" "strings" "testing" @@ -12,12 +10,9 @@ import ( "github.com/fabric8-services/fabric8-wit/gormtestsupport" "github.com/fabric8-services/fabric8-wit/rendering" "github.com/fabric8-services/fabric8-wit/resource" - 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/lib/pq" - uuid "github.com/satori/go.uuid" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" @@ -37,18 +32,16 @@ func TestRunSearchRepositoryWhiteboxTest(t *testing.T) { type searchRepositoryWhiteboxTest struct { gormtestsupport.DBTestSuite - modifierID uuid.UUID + sr *GormSearchRepository } func (s *searchRepositoryWhiteboxTest) SetupSuite() { s.DBTestSuite.SetupSuite() + s.sr = NewGormSearchRepository(s.DB) } func (s *searchRepositoryWhiteboxTest) SetupTest() { s.DBTestSuite.SetupTest() - testIdentity, err := testsupport.CreateTestIdentity(s.DB, "jdoe", "test") - require.Nil(s.T(), err) - s.modifierID = testIdentity.ID } type SearchTestDescriptor struct { @@ -59,6 +52,8 @@ type SearchTestDescriptor struct { func (s *searchRepositoryWhiteboxTest) TestSearch() { + var start, limit int = 0, 100 + s.T().Run("Search accross title and description", func(t *testing.T) { // given fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(1, func(fxt *tf.TestFixture, idx int) error { @@ -68,7 +63,8 @@ func (s *searchRepositoryWhiteboxTest) TestSearch() { })) // when searchQuery := `Sbose "deScription" '12345678asdfgh'` - searchResults, err := s.searchFor(fxt.Spaces[0].ID, searchQuery) + spaceID := fxt.Spaces[0].ID.String() + searchResults, _, err := s.sr.SearchFullText(context.Background(), searchQuery, &start, &limit, &spaceID) // then require.Nil(t, err) verify(t, searchQuery, searchResults, 1) @@ -82,7 +78,8 @@ func (s *searchRepositoryWhiteboxTest) TestSearch() { })) // when searchQuery := `sbose nofield` - searchResults, err := s.searchFor(fxt.Spaces[0].ID, searchQuery) + spaceID := fxt.Spaces[0].ID.String() + searchResults, _, err := s.sr.SearchFullText(context.Background(), searchQuery, &start, &limit, &spaceID) // then require.Nil(t, err) verify(t, searchQuery, searchResults, 1) @@ -97,7 +94,8 @@ func (s *searchRepositoryWhiteboxTest) TestSearch() { })) // when searchQuery := `models/errors.go remoteworkitem` - searchResults, err := s.searchFor(fxt.Spaces[0].ID, searchQuery) + spaceID := fxt.Spaces[0].ID.String() + searchResults, _, err := s.sr.SearchFullText(context.Background(), searchQuery, &start, &limit, &spaceID) // then require.Nil(t, err) verify(t, searchQuery, searchResults, 1) @@ -111,7 +109,8 @@ func (s *searchRepositoryWhiteboxTest) TestSearch() { })) // when searchQuery := `(value)` - searchResults, err := s.searchFor(fxt.Spaces[0].ID, searchQuery) + spaceID := fxt.Spaces[0].ID.String() + searchResults, _, err := s.sr.SearchFullText(context.Background(), searchQuery, &start, &limit, &spaceID) // then require.Nil(t, err) verify(t, searchQuery, searchResults, 1) @@ -126,7 +125,8 @@ func (s *searchRepositoryWhiteboxTest) TestSearch() { })) // when searchQuery := `(pranav) {shoubhik} [aslak]` - searchResults, err := s.searchFor(fxt.Spaces[0].ID, searchQuery) + spaceID := fxt.Spaces[0].ID.String() + searchResults, _, err := s.sr.SearchFullText(context.Background(), searchQuery, &start, &limit, &spaceID) // then require.Nil(t, err) verify(t, searchQuery, searchResults, 1) @@ -141,7 +141,8 @@ func (s *searchRepositoryWhiteboxTest) TestSearch() { })) // when searchQuery := `negative case` - searchResults, err := s.searchFor(fxt.Spaces[0].ID, searchQuery) + spaceID := fxt.Spaces[0].ID.String() + searchResults, _, err := s.sr.SearchFullText(context.Background(), searchQuery, &start, &limit, &spaceID) // then require.Nil(t, err) verify(t, searchQuery, searchResults, 0) @@ -155,7 +156,8 @@ func (s *searchRepositoryWhiteboxTest) TestSearch() { })) // when searchQuery := fmt.Sprintf("number:%d", fxt.WorkItems[0].Number) - searchResults, err := s.searchFor(fxt.Spaces[0].ID, searchQuery) + spaceID := fxt.Spaces[0].ID.String() + searchResults, _, err := s.sr.SearchFullText(context.Background(), searchQuery, &start, &limit, &spaceID) // then require.Nil(t, err) require.Len(t, searchResults, 1) @@ -170,7 +172,8 @@ func (s *searchRepositoryWhiteboxTest) TestSearch() { })) // when searchQuery := fmt.Sprintf("%s%d", "http://demo.openshift.io/work-item/list/detail/", fxt.WorkItems[0].Number) - searchResults, err := s.searchFor(fxt.Spaces[0].ID, searchQuery) + spaceID := fxt.Spaces[0].ID.String() + searchResults, _, err := s.sr.SearchFullText(context.Background(), searchQuery, &start, &limit, &spaceID) // then require.Nil(t, err) require.Len(t, searchResults, 1) @@ -179,17 +182,8 @@ func (s *searchRepositoryWhiteboxTest) TestSearch() { } -func (s *searchRepositoryWhiteboxTest) searchFor(spaceID uuid.UUID, searchQuery string) ([]workitem.WorkItem, error) { - req := &http.Request{Host: "localhost"} - params := url.Values{} - ctx := goa.NewContext(context.Background(), nil, req, params) - sr := NewGormSearchRepository(s.DB) - var start, limit int = 0, 100 - spaceIDStr := spaceID.String() - workItemList, _, err := sr.SearchFullText(ctx, fmt.Sprintf("\"%s\"", searchQuery), &start, &limit, &spaceIDStr) - return workItemList, err -} - +// verify verifies that the search results match with the expected count and that the title or description contain all +// the terms of the search query func verify(t *testing.T, searchQuery string, searchResults []workitem.WorkItem, expectedCount int) { // Since this test adds test data, whether or not other workitems exist // there must be at least 1 search result returned. diff --git a/workitem/number_sequence/work_item_number_sequence_repository_test.go b/workitem/number_sequence/work_item_number_sequence_repository_test.go new file mode 100644 index 0000000000..b4dae7a7ca --- /dev/null +++ b/workitem/number_sequence/work_item_number_sequence_repository_test.go @@ -0,0 +1,75 @@ +package numbersequence_test + +import ( + "context" + "fmt" + "sync" + "testing" + + "github.com/fabric8-services/fabric8-wit/application" + "github.com/fabric8-services/fabric8-wit/gormapplication" + "github.com/fabric8-services/fabric8-wit/gormtestsupport" + "github.com/fabric8-services/fabric8-wit/resource" + tf "github.com/fabric8-services/fabric8-wit/test/testfixture" + . "github.com/fabric8-services/fabric8-wit/workitem/number_sequence" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/suite" +) + +type workItemNumberSequenceTest struct { + gormtestsupport.DBTestSuite + repo WorkItemNumberSequenceRepository +} + +func TestWorkItemNumberSequenceTest(t *testing.T) { + resource.Require(t, resource.Database) + suite.Run(t, &workItemNumberSequenceTest{DBTestSuite: gormtestsupport.NewDBTestSuite("../../config.yaml")}) +} + +func (s *workItemNumberSequenceTest) SetupTest() { + s.DBTestSuite.SetupTest() + s.repo = NewWorkItemNumberSequenceRepository(s.DB) +} + +func (s *workItemNumberSequenceTest) TestConcurrentNextVal() { + // given + fxt := tf.NewTestFixture(s.T(), s.DB, tf.Spaces(1)) + type Report struct { + id int + total int + failures int + } + routines := 10 + itemsPerRoutine := 50 + reports := make([]Report, routines) + // when running concurrent go routines simultaneously + var wg sync.WaitGroup + for i := 0; i < routines; i++ { + // in each go rountine, run 10 creations + go func(routineID int) { + wg.Add(1) + defer wg.Done() + report := Report{id: routineID} + for j := 0; j < itemsPerRoutine; j++ { + if err := application.Transactional(gormapplication.NewGormDB(s.DB), func(app application.Application) error { + _, err := s.repo.NextVal(context.Background(), fxt.Spaces[0].ID) + return err + }); err != nil { + s.T().Logf("Creation failed: %s", err.Error()) + report.failures++ + } + report.total++ + } + reports[routineID] = report + }(i) + } + wg.Wait() + // then + // wait for all items to be created + for _, report := range reports { + fmt.Printf("Routine #%d done: %d creations, including %d failure(s)\n", report.id, report.total, report.failures) + assert.Equal(s.T(), itemsPerRoutine, report.total) + assert.Equal(s.T(), 0, report.failures) + } + +} diff --git a/workitem/number_sequence/workitem_number_sequence.go b/workitem/number_sequence/workitem_number_sequence.go index c72a4d3e33..d8aca1b536 100644 --- a/workitem/number_sequence/workitem_number_sequence.go +++ b/workitem/number_sequence/workitem_number_sequence.go @@ -22,5 +22,5 @@ func (w WorkItemNumberSequence) TableName() string { } func (w *WorkItemNumberSequence) String() string { - return fmt.Sprintf("SpaceId=%s Number=%d", w.SpaceID.String(), w.CurrentVal) + return fmt.Sprintf("SpaceID=%s Number=%d", w.SpaceID.String(), w.CurrentVal) } diff --git a/workitem/workitem_repository.go b/workitem/workitem_repository.go index 43b660f8d3..d6823e44cb 100644 --- a/workitem/workitem_repository.go +++ b/workitem/workitem_repository.go @@ -55,10 +55,10 @@ type WorkItemRepository interface { // NewWorkItemRepository creates a GormWorkItemRepository func NewWorkItemRepository(db *gorm.DB) *GormWorkItemRepository { repository := &GormWorkItemRepository{ - db, - numbersequence.NewWorkItemNumberSequenceRepository(db), - &GormWorkItemTypeRepository{db}, - &GormRevisionRepository{db}, + db: db, + winr: numbersequence.NewWorkItemNumberSequenceRepository(db), + witr: &GormWorkItemTypeRepository{db}, + wirr: &GormRevisionRepository{db}, } return repository } diff --git a/workitem/workitem_repository_blackbox_test.go b/workitem/workitem_repository_blackbox_test.go index 78771d88b5..bd108801fc 100644 --- a/workitem/workitem_repository_blackbox_test.go +++ b/workitem/workitem_repository_blackbox_test.go @@ -3,6 +3,7 @@ package workitem_test import ( "context" "fmt" + "sync" "testing" "time" @@ -309,34 +310,23 @@ func (s *workItemRepoBlackBoxTest) TestLookupIDByNamedSpaceAndNumberStaleSpace() func (s *workItemRepoBlackBoxTest) TestConcurrentWorkItemCreations() { // given - fxt := tf.NewTestFixture(s.T(), s.DB, - tf.Identities(1, func(fxt *tf.TestFixture, idx int) error { - fxt.Identities[idx].Username = "TestConcurrentCreate-" + uuid.NewV4().String() - return nil - }), - tf.Spaces(1, func(fxt *tf.TestFixture, idx int) error { - fxt.Spaces[idx].OwnerId = fxt.Identities[idx].ID - fxt.Spaces[idx].Name = "TestConcurrentCreate-" + uuid.NewV4().String() - return nil - }), - tf.WorkItemTypes(1, func(fxt *tf.TestFixture, idx int) error { - fxt.WorkItemTypes[idx].SpaceID = fxt.Spaces[0].ID - return nil - })) + fxt := tf.NewTestFixture(s.T(), s.DB, tf.CreateWorkItemEnvironment()) type Report struct { id int total int failures int } - done := make(chan Report) - routines := 10 itemsPerRoutine := 50 + reports := make([]Report, routines) // when running concurrent go routines simultaneously + var wg sync.WaitGroup for i := 0; i < routines; i++ { // in each go rountine, run 10 creations - go func(rountineID int) { - report := Report{id: rountineID} + go func(routineID int) { + wg.Add(1) + defer wg.Done() + report := Report{id: routineID} for j := 0; j < itemsPerRoutine; j++ { if err := application.Transactional(gormapplication.NewGormDB(s.DB), func(app application.Application) error { @@ -353,16 +343,15 @@ func (s *workItemRepoBlackBoxTest) TestConcurrentWorkItemCreations() { } report.total++ } - done <- report + reports[routineID] = report }(i) } + wg.Wait() + // then // wait for all items to be created - for i := 0; i < routines; i++ { - report := <-done + for _, report := range reports { fmt.Printf("Routine #%d done: %d creations, including %d failure(s)\n", report.id, report.total, report.failures) assert.Equal(s.T(), itemsPerRoutine, report.total) assert.Equal(s.T(), 0, report.failures) } - - // then } From 0cfb7920ff49ccd7db46ac4d36d54adf2fdd4f22 Mon Sep 17 00:00:00 2001 From: Xavier Coulon Date: Wed, 11 Oct 2017 12:12:15 +0200 Subject: [PATCH 13/14] Create 10 work items per test This highlighted some cases where searching by number or URL can match more than 1 work item. Also: move some log statements to `DEBUG` instead of `INFO` to reduce the noise during test executions Signed-off-by: Xavier Coulon --- account/identity.go | 4 +- gormsupport/cleaner/db_clean.go | 2 +- search/search_repository_whitebox_test.go | 111 +++++++++++++----- space/space.go | 4 +- test/account.go | 2 - workitem/link/link_revision_repository.go | 2 +- ...rk_item_number_sequence_repository_test.go | 7 +- workitem/workitem_repository_blackbox_test.go | 17 +-- workitem/workitem_revision_repository.go | 2 +- workitem/workitemtype_repository.go | 4 +- 10 files changed, 95 insertions(+), 60 deletions(-) diff --git a/account/identity.go b/account/identity.go index 5bb0905c77..bedb3507f8 100644 --- a/account/identity.go +++ b/account/identity.go @@ -161,9 +161,9 @@ func (m *GormIdentityRepository) Create(ctx context.Context, model *Identity) er }, "unable to create the identity") return errs.WithStack(err) } - log.Info(ctx, map[string]interface{}{ + log.Debug(ctx, map[string]interface{}{ "identity_id": model.ID, - }, "Identity created!") + }, "Identity created") return nil } diff --git a/gormsupport/cleaner/db_clean.go b/gormsupport/cleaner/db_clean.go index 7cd9cb19fa..abbfd9fa4c 100644 --- a/gormsupport/cleaner/db_clean.go +++ b/gormsupport/cleaner/db_clean.go @@ -65,7 +65,7 @@ func DeleteCreatedEntities(db *gorm.DB) func() { } for i := len(entires) - 1; i >= 0; i-- { entry := entires[i] - log.Info(nil, map[string]interface{}{ + log.Debug(nil, map[string]interface{}{ "table": entry.table, "key": entry.key, "hook_name": hookName, diff --git a/search/search_repository_whitebox_test.go b/search/search_repository_whitebox_test.go index 4cd50801e7..c81572c366 100644 --- a/search/search_repository_whitebox_test.go +++ b/search/search_repository_whitebox_test.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "regexp" + "strconv" "strings" "testing" @@ -40,10 +41,6 @@ func (s *searchRepositoryWhiteboxTest) SetupSuite() { s.sr = NewGormSearchRepository(s.DB) } -func (s *searchRepositoryWhiteboxTest) SetupTest() { - s.DBTestSuite.SetupTest() -} - type SearchTestDescriptor struct { fields map[string]interface{} searchString string @@ -56,9 +53,11 @@ func (s *searchRepositoryWhiteboxTest) TestSearch() { s.T().Run("Search accross title and description", func(t *testing.T) { // given - fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(1, func(fxt *tf.TestFixture, idx int) error { - fxt.WorkItems[idx].Fields[workitem.SystemTitle] = "test sbose title '12345678asdfgh'" - fxt.WorkItems[idx].Fields[workitem.SystemDescription] = rendering.NewMarkupContentFromLegacy(`"description" for search test`) + fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(10, func(fxt *tf.TestFixture, idx int) error { + if idx == 0 { + fxt.WorkItems[idx].Fields[workitem.SystemTitle] = "test sbose title '12345678asdfgh'" + fxt.WorkItems[idx].Fields[workitem.SystemDescription] = rendering.NewMarkupContentFromLegacy(`"description" for search test`) + } return nil })) // when @@ -72,8 +71,10 @@ func (s *searchRepositoryWhiteboxTest) TestSearch() { s.T().Run("Search accross title and description undefined", func(t *testing.T) { // given - fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(1, func(fxt *tf.TestFixture, idx int) error { - fxt.WorkItems[idx].Fields[workitem.SystemTitle] = "test nofield sbose title '12345678asdfgh'" + fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(10, func(fxt *tf.TestFixture, idx int) error { + if idx == 0 { + fxt.WorkItems[idx].Fields[workitem.SystemTitle] = "test nofield sbose title '12345678asdfgh'" + } return nil })) // when @@ -87,9 +88,11 @@ func (s *searchRepositoryWhiteboxTest) TestSearch() { s.T().Run("Search accross title with slash", func(t *testing.T) { // given - fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(1, func(fxt *tf.TestFixture, idx int) error { - fxt.WorkItems[idx].Fields[workitem.SystemTitle] = "add new error types in models/errors.go'" - fxt.WorkItems[idx].Fields[workitem.SystemDescription] = rendering.NewMarkupContentFromLegacy(`Make sure remoteworkitem can access..`) + fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(10, func(fxt *tf.TestFixture, idx int) error { + if idx == 0 { + fxt.WorkItems[idx].Fields[workitem.SystemTitle] = "add new error types in models/errors.go'" + fxt.WorkItems[idx].Fields[workitem.SystemDescription] = rendering.NewMarkupContentFromLegacy(`Make sure remoteworkitem can access..`) + } return nil })) // when @@ -103,8 +106,10 @@ func (s *searchRepositoryWhiteboxTest) TestSearch() { s.T().Run("Search accross title with braces", func(t *testing.T) { // given - fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(1, func(fxt *tf.TestFixture, idx int) error { - fxt.WorkItems[idx].Fields[workitem.SystemTitle] = "Bug reported by administrator for input = (value)" + fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(10, func(fxt *tf.TestFixture, idx int) error { + if idx == 0 { + fxt.WorkItems[idx].Fields[workitem.SystemTitle] = "Bug reported by administrator for input = (value)" + } return nil })) // when @@ -119,8 +124,10 @@ func (s *searchRepositoryWhiteboxTest) TestSearch() { s.T().Run("Search accross title with braces and brackets", func(t *testing.T) { // given - fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(1, func(fxt *tf.TestFixture, idx int) error { - fxt.WorkItems[idx].Fields[workitem.SystemTitle] = "trial for braces (pranav) {shoubhik} [aslak]" + fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(10, func(fxt *tf.TestFixture, idx int) error { + if idx == 0 { + fxt.WorkItems[idx].Fields[workitem.SystemTitle] = "trial for braces (pranav) {shoubhik} [aslak]" + } return nil })) // when @@ -135,8 +142,10 @@ func (s *searchRepositoryWhiteboxTest) TestSearch() { s.T().Run("Search accross title and description undefined and no match", func(t *testing.T) { // given - fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(1, func(fxt *tf.TestFixture, idx int) error { - fxt.WorkItems[idx].Fields[workitem.SystemTitle] = "test should return 0 results'" + fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(10, func(fxt *tf.TestFixture, idx int) error { + if idx == 0 { + fxt.WorkItems[idx].Fields[workitem.SystemTitle] = "test should return 0 results'" + } return nil })) // when @@ -148,36 +157,76 @@ func (s *searchRepositoryWhiteboxTest) TestSearch() { verify(t, searchQuery, searchResults, 0) }) - s.T().Run("Search by number", func(t *testing.T) { + s.T().Run("Search by number - single match", func(t *testing.T) { + // given + fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(10)) + queryNumber := fxt.WorkItems[2].Number + // when looking for `number:3` + searchQuery := fmt.Sprintf("number:%d", queryNumber) + spaceID := fxt.Spaces[0].ID.String() + searchResults, _, err := s.sr.SearchFullText(context.Background(), searchQuery, &start, &limit, &spaceID) + // then there should be a single match + require.Nil(t, err) + require.Len(t, searchResults, 1) + assert.Equal(t, queryNumber, searchResults[0].Number) + }) + + s.T().Run("Search by number - multiple matches", func(t *testing.T) { + // given + fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(10)) + queryNumber := fxt.WorkItems[0].Number + // when looking for `number:1` + searchQuery := fmt.Sprintf("number:%d", queryNumber) + spaceID := fxt.Spaces[0].ID.String() + searchResults, _, err := s.sr.SearchFullText(context.Background(), searchQuery, &start, &limit, &spaceID) + // then there should be 2 matches: `1` and `10` + require.Nil(t, err) + require.Len(t, searchResults, 2) + for _, searchResult := range searchResults { + // verifies that the number in the search result contains the query number + assert.Contains(t, strconv.Itoa(searchResult.Number), strconv.Itoa(queryNumber)) + } + }) + + s.T().Run("Search by URL - single match", func(t *testing.T) { // given - fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(1, func(fxt *tf.TestFixture, idx int) error { - fxt.WorkItems[idx].Fields[workitem.SystemTitle] = "test nofield sbose title '12345678asdfgh'" + fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(10, func(fxt *tf.TestFixture, idx int) error { + if idx == 0 { + fxt.WorkItems[idx].Fields[workitem.SystemTitle] = "test nofield sbose title '12345678asdfgh'" + } return nil })) - // when - searchQuery := fmt.Sprintf("number:%d", fxt.WorkItems[0].Number) + // when looking for `http://.../3` there should be a single match + queryNumber := fxt.WorkItems[2].Number + searchQuery := fmt.Sprintf("%s%d", "http://demo.openshift.io/work-item/list/detail/", queryNumber) spaceID := fxt.Spaces[0].ID.String() searchResults, _, err := s.sr.SearchFullText(context.Background(), searchQuery, &start, &limit, &spaceID) // then require.Nil(t, err) require.Len(t, searchResults, 1) - assert.Equal(t, fxt.WorkItems[0].ID, searchResults[0].ID) + assert.Equal(t, queryNumber, searchResults[0].Number) }) - s.T().Run("Search by URL", func(t *testing.T) { + s.T().Run("Search by URL - multiple matches", func(t *testing.T) { // given - fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(1, func(fxt *tf.TestFixture, idx int) error { - fxt.WorkItems[idx].Fields[workitem.SystemTitle] = "test nofield sbose title '12345678asdfgh'" + fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(10, func(fxt *tf.TestFixture, idx int) error { + if idx == 0 { + fxt.WorkItems[idx].Fields[workitem.SystemTitle] = "test nofield sbose title '12345678asdfgh'" + } return nil })) - // when - searchQuery := fmt.Sprintf("%s%d", "http://demo.openshift.io/work-item/list/detail/", fxt.WorkItems[0].Number) + // when looking for `http://.../1` there should be a 2 matchs: `http://.../1` and `http://.../10`` + queryNumber := fxt.WorkItems[0].Number + searchQuery := fmt.Sprintf("%s%d", "http://demo.openshift.io/work-item/list/detail/", queryNumber) spaceID := fxt.Spaces[0].ID.String() searchResults, _, err := s.sr.SearchFullText(context.Background(), searchQuery, &start, &limit, &spaceID) // then require.Nil(t, err) - require.Len(t, searchResults, 1) - assert.Equal(t, fxt.WorkItems[0].ID, searchResults[0].ID) + require.Len(t, searchResults, 2) + for _, searchResult := range searchResults { + // verifies that the number in the search result contains the query number + assert.Contains(t, strconv.Itoa(searchResult.Number), strconv.Itoa(queryNumber)) + } }) } diff --git a/space/space.go b/space/space.go index 6d87fe4b22..1f71f80672 100644 --- a/space/space.go +++ b/space/space.go @@ -230,9 +230,9 @@ func (r *GormRepository) Create(ctx context.Context, space *Space) (*Space, erro } return nil, errors.NewInternalError(ctx, err) } - log.Info(ctx, map[string]interface{}{ + log.Debug(ctx, map[string]interface{}{ "space_id": space.ID, - }, "Space created successfully") + }, "Space created") return space, nil } diff --git a/test/account.go b/test/account.go index 7e5b20ee14..e431f60795 100644 --- a/test/account.go +++ b/test/account.go @@ -70,8 +70,6 @@ func CreateTestIdentityForAccountIdentity(db *gorm.DB, identity *account.Identit "err": err, "identity": identity, }, "unable to create identity") - } else { - log.Info(nil, map[string]interface{}{"identity_id": identity.ID}, "created identity") } return err } diff --git a/workitem/link/link_revision_repository.go b/workitem/link/link_revision_repository.go index 37434f797e..aa45e9946e 100644 --- a/workitem/link/link_revision_repository.go +++ b/workitem/link/link_revision_repository.go @@ -33,7 +33,7 @@ type GormWorkItemLinkRevisionRepository struct { // Create stores a new revision for the given work item link. func (r *GormWorkItemLinkRevisionRepository) Create(ctx context.Context, modifierID uuid.UUID, revisionType RevisionType, l WorkItemLink) error { - log.Info(nil, map[string]interface{}{ + log.Debug(nil, map[string]interface{}{ "modifier_id": modifierID, "revision_type": revisionType, }, "Storing a revision after operation on work item link.") diff --git a/workitem/number_sequence/work_item_number_sequence_repository_test.go b/workitem/number_sequence/work_item_number_sequence_repository_test.go index b4dae7a7ca..20bf792dfa 100644 --- a/workitem/number_sequence/work_item_number_sequence_repository_test.go +++ b/workitem/number_sequence/work_item_number_sequence_repository_test.go @@ -6,8 +6,6 @@ import ( "sync" "testing" - "github.com/fabric8-services/fabric8-wit/application" - "github.com/fabric8-services/fabric8-wit/gormapplication" "github.com/fabric8-services/fabric8-wit/gormtestsupport" "github.com/fabric8-services/fabric8-wit/resource" tf "github.com/fabric8-services/fabric8-wit/test/testfixture" @@ -51,10 +49,7 @@ func (s *workItemNumberSequenceTest) TestConcurrentNextVal() { defer wg.Done() report := Report{id: routineID} for j := 0; j < itemsPerRoutine; j++ { - if err := application.Transactional(gormapplication.NewGormDB(s.DB), func(app application.Application) error { - _, err := s.repo.NextVal(context.Background(), fxt.Spaces[0].ID) - return err - }); err != nil { + if _, err := s.repo.NextVal(context.Background(), fxt.Spaces[0].ID); err != nil { s.T().Logf("Creation failed: %s", err.Error()) report.failures++ } diff --git a/workitem/workitem_repository_blackbox_test.go b/workitem/workitem_repository_blackbox_test.go index bd108801fc..2503c92cf0 100644 --- a/workitem/workitem_repository_blackbox_test.go +++ b/workitem/workitem_repository_blackbox_test.go @@ -7,10 +7,8 @@ import ( "testing" "time" - "github.com/fabric8-services/fabric8-wit/application" "github.com/fabric8-services/fabric8-wit/codebase" "github.com/fabric8-services/fabric8-wit/errors" - "github.com/fabric8-services/fabric8-wit/gormapplication" "github.com/fabric8-services/fabric8-wit/gormtestsupport" "github.com/fabric8-services/fabric8-wit/rendering" "github.com/fabric8-services/fabric8-wit/resource" @@ -328,16 +326,11 @@ func (s *workItemRepoBlackBoxTest) TestConcurrentWorkItemCreations() { defer wg.Done() report := Report{id: routineID} for j := 0; j < itemsPerRoutine; j++ { - if err := application.Transactional(gormapplication.NewGormDB(s.DB), func(app application.Application) error { - - fields := map[string]interface{}{ - workitem.SystemTitle: uuid.NewV4().String(), - workitem.SystemState: workitem.SystemStateNew, - } - _, err := s.repo.Create(context.Background(), fxt.Spaces[0].ID, fxt.WorkItemTypes[0].ID, fields, fxt.Identities[0].ID) - - return err - }); err != nil { + fields := map[string]interface{}{ + workitem.SystemTitle: uuid.NewV4().String(), + workitem.SystemState: workitem.SystemStateNew, + } + if _, err := s.repo.Create(context.Background(), fxt.Spaces[0].ID, fxt.WorkItemTypes[0].ID, fields, fxt.Identities[0].ID); err != nil { s.T().Logf("Creation failed: %s", err.Error()) report.failures++ } diff --git a/workitem/workitem_revision_repository.go b/workitem/workitem_revision_repository.go index d2ae7c3860..1c3ca042de 100644 --- a/workitem/workitem_revision_repository.go +++ b/workitem/workitem_revision_repository.go @@ -33,7 +33,7 @@ type GormRevisionRepository struct { // Create stores a new revision for the given work item. func (r *GormRevisionRepository) Create(ctx context.Context, modifierID uuid.UUID, revisionType RevisionType, workitem WorkItemStorage) error { - log.Info(nil, map[string]interface{}{ + log.Debug(nil, map[string]interface{}{ "modifier_id": modifierID, "revision_type": revisionType, }, "Storing a revision after operation on work item.") diff --git a/workitem/workitemtype_repository.go b/workitem/workitemtype_repository.go index 946aa47b18..5e14e9516a 100644 --- a/workitem/workitemtype_repository.go +++ b/workitem/workitemtype_repository.go @@ -52,7 +52,7 @@ func (r *GormWorkItemTypeRepository) LoadByID(ctx context.Context, id uuid.UUID) // returns NotFoundError, InternalError func (r *GormWorkItemTypeRepository) Load(ctx context.Context, spaceID uuid.UUID, id uuid.UUID) (*WorkItemType, error) { defer goa.MeasureSince([]string{"goa", "db", "workitemtype", "load"}, time.Now()) - log.Info(ctx, map[string]interface{}{ + log.Debug(ctx, map[string]interface{}{ "wit_id": id, "space_id": spaceID, }, "Loading work item type") @@ -100,7 +100,7 @@ func (r *GormWorkItemTypeRepository) CheckExists(ctx context.Context, id string) // LoadTypeFromDB return work item type for the given id func (r *GormWorkItemTypeRepository) LoadTypeFromDB(ctx context.Context, id uuid.UUID) (*WorkItemType, error) { - log.Info(ctx, map[string]interface{}{ + log.Debug(ctx, map[string]interface{}{ "wit_id": id, }, "Loading work item type") res, ok := cache.Get(id) From 6cc2e9686b736599edea132fe4fc44f58c775ae1 Mon Sep 17 00:00:00 2001 From: Xavier Coulon Date: Wed, 11 Oct 2017 14:58:21 +0200 Subject: [PATCH 14/14] refactor the search tests using subtests, and fix race condition in concurrency tests Signed-off-by: Xavier Coulon --- search/search_repository_whitebox_test.go | 347 +++++++++--------- ...rk_item_number_sequence_repository_test.go | 9 +- workitem/workitem_repository_blackbox_test.go | 5 +- 3 files changed, 188 insertions(+), 173 deletions(-) diff --git a/search/search_repository_whitebox_test.go b/search/search_repository_whitebox_test.go index c81572c366..78deffe037 100644 --- a/search/search_repository_whitebox_test.go +++ b/search/search_repository_whitebox_test.go @@ -41,6 +41,10 @@ func (s *searchRepositoryWhiteboxTest) SetupSuite() { s.sr = NewGormSearchRepository(s.DB) } +func (s *searchRepositoryWhiteboxTest) SetupTest() { + s.DBTestSuite.SetupTest() +} + type SearchTestDescriptor struct { fields map[string]interface{} searchString string @@ -52,183 +56,190 @@ func (s *searchRepositoryWhiteboxTest) TestSearch() { var start, limit int = 0, 100 s.T().Run("Search accross title and description", func(t *testing.T) { - // given - fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(10, func(fxt *tf.TestFixture, idx int) error { - if idx == 0 { - fxt.WorkItems[idx].Fields[workitem.SystemTitle] = "test sbose title '12345678asdfgh'" - fxt.WorkItems[idx].Fields[workitem.SystemDescription] = rendering.NewMarkupContentFromLegacy(`"description" for search test`) - } - return nil - })) - // when - searchQuery := `Sbose "deScription" '12345678asdfgh'` - spaceID := fxt.Spaces[0].ID.String() - searchResults, _, err := s.sr.SearchFullText(context.Background(), searchQuery, &start, &limit, &spaceID) - // then - require.Nil(t, err) - verify(t, searchQuery, searchResults, 1) - }) - - s.T().Run("Search accross title and description undefined", func(t *testing.T) { - // given - fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(10, func(fxt *tf.TestFixture, idx int) error { - if idx == 0 { - fxt.WorkItems[idx].Fields[workitem.SystemTitle] = "test nofield sbose title '12345678asdfgh'" - } - return nil - })) - // when - searchQuery := `sbose nofield` - spaceID := fxt.Spaces[0].ID.String() - searchResults, _, err := s.sr.SearchFullText(context.Background(), searchQuery, &start, &limit, &spaceID) - // then - require.Nil(t, err) - verify(t, searchQuery, searchResults, 1) - }) - - s.T().Run("Search accross title with slash", func(t *testing.T) { - // given - fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(10, func(fxt *tf.TestFixture, idx int) error { - if idx == 0 { - fxt.WorkItems[idx].Fields[workitem.SystemTitle] = "add new error types in models/errors.go'" - fxt.WorkItems[idx].Fields[workitem.SystemDescription] = rendering.NewMarkupContentFromLegacy(`Make sure remoteworkitem can access..`) - } - return nil - })) - // when - searchQuery := `models/errors.go remoteworkitem` - spaceID := fxt.Spaces[0].ID.String() - searchResults, _, err := s.sr.SearchFullText(context.Background(), searchQuery, &start, &limit, &spaceID) - // then - require.Nil(t, err) - verify(t, searchQuery, searchResults, 1) - }) - - s.T().Run("Search accross title with braces", func(t *testing.T) { - // given - fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(10, func(fxt *tf.TestFixture, idx int) error { - if idx == 0 { - fxt.WorkItems[idx].Fields[workitem.SystemTitle] = "Bug reported by administrator for input = (value)" - } - return nil - })) - // when - searchQuery := `(value)` - spaceID := fxt.Spaces[0].ID.String() - searchResults, _, err := s.sr.SearchFullText(context.Background(), searchQuery, &start, &limit, &spaceID) - // then - require.Nil(t, err) - verify(t, searchQuery, searchResults, 1) - - }) - - s.T().Run("Search accross title with braces and brackets", func(t *testing.T) { - // given - fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(10, func(fxt *tf.TestFixture, idx int) error { - if idx == 0 { - fxt.WorkItems[idx].Fields[workitem.SystemTitle] = "trial for braces (pranav) {shoubhik} [aslak]" - } - return nil - })) - // when - searchQuery := `(pranav) {shoubhik} [aslak]` - spaceID := fxt.Spaces[0].ID.String() - searchResults, _, err := s.sr.SearchFullText(context.Background(), searchQuery, &start, &limit, &spaceID) - // then - require.Nil(t, err) - verify(t, searchQuery, searchResults, 1) + t.Run("match title and descrition", func(t *testing.T) { + // given + fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(10, func(fxt *tf.TestFixture, idx int) error { + if idx == 0 { + fxt.WorkItems[idx].Fields[workitem.SystemTitle] = "test sbose title '12345678asdfgh'" + fxt.WorkItems[idx].Fields[workitem.SystemDescription] = rendering.NewMarkupContentFromLegacy(`"description" for search test`) + } + return nil + })) + // when + searchQuery := `Sbose "deScription" '12345678asdfgh'` + spaceID := fxt.Spaces[0].ID.String() + searchResults, _, err := s.sr.SearchFullText(context.Background(), searchQuery, &start, &limit, &spaceID) + // then + require.Nil(t, err) + verify(t, searchQuery, searchResults, 1) + }) + + t.Run("match title with description undefined", func(t *testing.T) { + // given + fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(10, func(fxt *tf.TestFixture, idx int) error { + if idx == 0 { + fxt.WorkItems[idx].Fields[workitem.SystemTitle] = "test nofield sbose title '12345678asdfgh'" + } + return nil + })) + // when + searchQuery := `sbose nofield` + spaceID := fxt.Spaces[0].ID.String() + searchResults, _, err := s.sr.SearchFullText(context.Background(), searchQuery, &start, &limit, &spaceID) + // then + require.Nil(t, err) + verify(t, searchQuery, searchResults, 1) + }) + + t.Run("match title with slash", func(t *testing.T) { + // given + fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(10, func(fxt *tf.TestFixture, idx int) error { + if idx == 0 { + fxt.WorkItems[idx].Fields[workitem.SystemTitle] = "add new error types in models/errors.go'" + fxt.WorkItems[idx].Fields[workitem.SystemDescription] = rendering.NewMarkupContentFromLegacy(`Make sure remoteworkitem can access..`) + } + return nil + })) + // when + searchQuery := `models/errors.go remoteworkitem` + spaceID := fxt.Spaces[0].ID.String() + searchResults, _, err := s.sr.SearchFullText(context.Background(), searchQuery, &start, &limit, &spaceID) + // then + require.Nil(t, err) + verify(t, searchQuery, searchResults, 1) + }) + + t.Run("match title with braces", func(t *testing.T) { + // given + fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(10, func(fxt *tf.TestFixture, idx int) error { + if idx == 0 { + fxt.WorkItems[idx].Fields[workitem.SystemTitle] = "Bug reported by administrator for input = (value)" + } + return nil + })) + // when + searchQuery := `(value)` + spaceID := fxt.Spaces[0].ID.String() + searchResults, _, err := s.sr.SearchFullText(context.Background(), searchQuery, &start, &limit, &spaceID) + // then + require.Nil(t, err) + verify(t, searchQuery, searchResults, 1) + + }) + + t.Run("match title with braces and brackets", func(t *testing.T) { + // given + fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(10, func(fxt *tf.TestFixture, idx int) error { + if idx == 0 { + fxt.WorkItems[idx].Fields[workitem.SystemTitle] = "trial for braces (pranav) {shoubhik} [aslak]" + } + return nil + })) + // when + searchQuery := `(pranav) {shoubhik} [aslak]` + spaceID := fxt.Spaces[0].ID.String() + searchResults, _, err := s.sr.SearchFullText(context.Background(), searchQuery, &start, &limit, &spaceID) + // then + require.Nil(t, err) + verify(t, searchQuery, searchResults, 1) + + }) + + t.Run("no match", func(t *testing.T) { + // given + fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(10, func(fxt *tf.TestFixture, idx int) error { + if idx == 0 { + fxt.WorkItems[idx].Fields[workitem.SystemTitle] = "test should return 0 results'" + } + return nil + })) + // when + searchQuery := `negative case` + spaceID := fxt.Spaces[0].ID.String() + searchResults, _, err := s.sr.SearchFullText(context.Background(), searchQuery, &start, &limit, &spaceID) + // then + require.Nil(t, err) + verify(t, searchQuery, searchResults, 0) + }) }) - s.T().Run("Search accross title and description undefined and no match", func(t *testing.T) { - // given - fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(10, func(fxt *tf.TestFixture, idx int) error { - if idx == 0 { - fxt.WorkItems[idx].Fields[workitem.SystemTitle] = "test should return 0 results'" + s.T().Run("Search by number", func(t *testing.T) { + t.Run("single match", func(t *testing.T) { + + // given + fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(10)) + queryNumber := fxt.WorkItems[2].Number + // when looking for `number:3` + searchQuery := fmt.Sprintf("number:%d", queryNumber) + spaceID := fxt.Spaces[0].ID.String() + searchResults, _, err := s.sr.SearchFullText(context.Background(), searchQuery, &start, &limit, &spaceID) + // then there should be a single match + require.Nil(t, err) + require.Len(t, searchResults, 1) + assert.Equal(t, queryNumber, searchResults[0].Number) + }) + + t.Run("multiple matches", func(t *testing.T) { + // given + fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(10)) + queryNumber := fxt.WorkItems[0].Number + // when looking for `number:1` + searchQuery := fmt.Sprintf("number:%d", queryNumber) + spaceID := fxt.Spaces[0].ID.String() + searchResults, _, err := s.sr.SearchFullText(context.Background(), searchQuery, &start, &limit, &spaceID) + // then there should be 2 matches: `1` and `10` + require.Nil(t, err) + require.Len(t, searchResults, 2) + for _, searchResult := range searchResults { + // verifies that the number in the search result contains the query number + assert.Contains(t, strconv.Itoa(searchResult.Number), strconv.Itoa(queryNumber)) } - return nil - })) - // when - searchQuery := `negative case` - spaceID := fxt.Spaces[0].ID.String() - searchResults, _, err := s.sr.SearchFullText(context.Background(), searchQuery, &start, &limit, &spaceID) - // then - require.Nil(t, err) - verify(t, searchQuery, searchResults, 0) - }) - - s.T().Run("Search by number - single match", func(t *testing.T) { - // given - fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(10)) - queryNumber := fxt.WorkItems[2].Number - // when looking for `number:3` - searchQuery := fmt.Sprintf("number:%d", queryNumber) - spaceID := fxt.Spaces[0].ID.String() - searchResults, _, err := s.sr.SearchFullText(context.Background(), searchQuery, &start, &limit, &spaceID) - // then there should be a single match - require.Nil(t, err) - require.Len(t, searchResults, 1) - assert.Equal(t, queryNumber, searchResults[0].Number) - }) - - s.T().Run("Search by number - multiple matches", func(t *testing.T) { - // given - fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(10)) - queryNumber := fxt.WorkItems[0].Number - // when looking for `number:1` - searchQuery := fmt.Sprintf("number:%d", queryNumber) - spaceID := fxt.Spaces[0].ID.String() - searchResults, _, err := s.sr.SearchFullText(context.Background(), searchQuery, &start, &limit, &spaceID) - // then there should be 2 matches: `1` and `10` - require.Nil(t, err) - require.Len(t, searchResults, 2) - for _, searchResult := range searchResults { - // verifies that the number in the search result contains the query number - assert.Contains(t, strconv.Itoa(searchResult.Number), strconv.Itoa(queryNumber)) - } + }) }) s.T().Run("Search by URL - single match", func(t *testing.T) { - // given - fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(10, func(fxt *tf.TestFixture, idx int) error { - if idx == 0 { - fxt.WorkItems[idx].Fields[workitem.SystemTitle] = "test nofield sbose title '12345678asdfgh'" - } - return nil - })) - // when looking for `http://.../3` there should be a single match - queryNumber := fxt.WorkItems[2].Number - searchQuery := fmt.Sprintf("%s%d", "http://demo.openshift.io/work-item/list/detail/", queryNumber) - spaceID := fxt.Spaces[0].ID.String() - searchResults, _, err := s.sr.SearchFullText(context.Background(), searchQuery, &start, &limit, &spaceID) - // then - require.Nil(t, err) - require.Len(t, searchResults, 1) - assert.Equal(t, queryNumber, searchResults[0].Number) - }) - - s.T().Run("Search by URL - multiple matches", func(t *testing.T) { - // given - fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(10, func(fxt *tf.TestFixture, idx int) error { - if idx == 0 { - fxt.WorkItems[idx].Fields[workitem.SystemTitle] = "test nofield sbose title '12345678asdfgh'" + t.Run("single match", func(t *testing.T) { + // given + fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(10, func(fxt *tf.TestFixture, idx int) error { + if idx == 0 { + fxt.WorkItems[idx].Fields[workitem.SystemTitle] = "test nofield sbose title '12345678asdfgh'" + } + return nil + })) + // when looking for `http://.../3` there should be a single match + queryNumber := fxt.WorkItems[2].Number + searchQuery := fmt.Sprintf("%s%d", "http://demo.openshift.io/work-item/list/detail/", queryNumber) + spaceID := fxt.Spaces[0].ID.String() + searchResults, _, err := s.sr.SearchFullText(context.Background(), searchQuery, &start, &limit, &spaceID) + // then + require.Nil(t, err) + require.Len(t, searchResults, 1) + assert.Equal(t, queryNumber, searchResults[0].Number) + }) + + t.Run("multiple matches", func(t *testing.T) { + // given + fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(10, func(fxt *tf.TestFixture, idx int) error { + if idx == 0 { + fxt.WorkItems[idx].Fields[workitem.SystemTitle] = "test nofield sbose title '12345678asdfgh'" + } + return nil + })) + // when looking for `http://.../1` there should be a 2 matchs: `http://.../1` and `http://.../10`` + queryNumber := fxt.WorkItems[0].Number + searchQuery := fmt.Sprintf("%s%d", "http://demo.openshift.io/work-item/list/detail/", queryNumber) + spaceID := fxt.Spaces[0].ID.String() + searchResults, _, err := s.sr.SearchFullText(context.Background(), searchQuery, &start, &limit, &spaceID) + // then + require.Nil(t, err) + require.Len(t, searchResults, 2) + for _, searchResult := range searchResults { + // verifies that the number in the search result contains the query number + assert.Contains(t, strconv.Itoa(searchResult.Number), strconv.Itoa(queryNumber)) } - return nil - })) - // when looking for `http://.../1` there should be a 2 matchs: `http://.../1` and `http://.../10`` - queryNumber := fxt.WorkItems[0].Number - searchQuery := fmt.Sprintf("%s%d", "http://demo.openshift.io/work-item/list/detail/", queryNumber) - spaceID := fxt.Spaces[0].ID.String() - searchResults, _, err := s.sr.SearchFullText(context.Background(), searchQuery, &start, &limit, &spaceID) - // then - require.Nil(t, err) - require.Len(t, searchResults, 2) - for _, searchResult := range searchResults { - // verifies that the number in the search result contains the query number - assert.Contains(t, strconv.Itoa(searchResult.Number), strconv.Itoa(queryNumber)) - } + }) }) - } // verify verifies that the search results match with the expected count and that the title or description contain all diff --git a/workitem/number_sequence/work_item_number_sequence_repository_test.go b/workitem/number_sequence/work_item_number_sequence_repository_test.go index 20bf792dfa..ba1c6427c9 100644 --- a/workitem/number_sequence/work_item_number_sequence_repository_test.go +++ b/workitem/number_sequence/work_item_number_sequence_repository_test.go @@ -6,6 +6,8 @@ import ( "sync" "testing" + "github.com/fabric8-services/fabric8-wit/application" + "github.com/fabric8-services/fabric8-wit/gormapplication" "github.com/fabric8-services/fabric8-wit/gormtestsupport" "github.com/fabric8-services/fabric8-wit/resource" tf "github.com/fabric8-services/fabric8-wit/test/testfixture" @@ -43,13 +45,16 @@ func (s *workItemNumberSequenceTest) TestConcurrentNextVal() { // when running concurrent go routines simultaneously var wg sync.WaitGroup for i := 0; i < routines; i++ { + wg.Add(1) // in each go rountine, run 10 creations go func(routineID int) { - wg.Add(1) defer wg.Done() report := Report{id: routineID} for j := 0; j < itemsPerRoutine; j++ { - if _, err := s.repo.NextVal(context.Background(), fxt.Spaces[0].ID); err != nil { + if err := application.Transactional(gormapplication.NewGormDB(s.DB), func(app application.Application) error { + _, err := s.repo.NextVal(context.Background(), fxt.Spaces[0].ID) + return err + }); err != nil { s.T().Logf("Creation failed: %s", err.Error()) report.failures++ } diff --git a/workitem/workitem_repository_blackbox_test.go b/workitem/workitem_repository_blackbox_test.go index 2503c92cf0..f8eb8d82b1 100644 --- a/workitem/workitem_repository_blackbox_test.go +++ b/workitem/workitem_repository_blackbox_test.go @@ -2,7 +2,6 @@ package workitem_test import ( "context" - "fmt" "sync" "testing" "time" @@ -320,9 +319,9 @@ func (s *workItemRepoBlackBoxTest) TestConcurrentWorkItemCreations() { // when running concurrent go routines simultaneously var wg sync.WaitGroup for i := 0; i < routines; i++ { + wg.Add(1) // in each go rountine, run 10 creations go func(routineID int) { - wg.Add(1) defer wg.Done() report := Report{id: routineID} for j := 0; j < itemsPerRoutine; j++ { @@ -343,7 +342,7 @@ func (s *workItemRepoBlackBoxTest) TestConcurrentWorkItemCreations() { // then // wait for all items to be created for _, report := range reports { - fmt.Printf("Routine #%d done: %d creations, including %d failure(s)\n", report.id, report.total, report.failures) + s.T().Logf("Routine #%d done: %d creations, including %d failure(s)\n", report.id, report.total, report.failures) assert.Equal(s.T(), itemsPerRoutine, report.total) assert.Equal(s.T(), 0, report.failures) }