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

xcoulon · 2017-10-02T16:30:10Z

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

Refactored the search space tests

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

Fixes #1676

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

…services#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 <xcoulon@redhat.com>

…vices#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 fabric8-services#1676 Signed-off-by: Xavier Coulon <xcoulon@redhat.com>

kwk · 2017-10-02T16:35:49Z

controller/search_spaces_blackbox_test.go

@@ -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.


Why not use a uuid.NewV4().String() as a prefix?

hum, I need to check. I found some issues just before about some invalid name length, so I wanted to use a shorter value, but a uuid should be fine. Let me fix that.

fabric8cd · 2017-10-02T16:37:54Z

@xcoulon snapshot fabric8-wit image is available for testing. docker pull docker.io/fabric8/fabric8-wit:SNAPSHOT-PR-1677-2

kwk

Not reviewed everything yet, but I found some problem.

kwk · 2017-10-02T16:41:42Z

search/search_repository_whitebox_test.go

+		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)


Sorry but I have to reject this change because you're mixing up a newly defined test fixture space with a WIT that belongs to the system space. That only works because of a hack. Can you instead maybe just also ask for a WIT in your test fixture above (tf.WorkItemTypes(1))? Then you can use that WIT's ID instead of workitem.SystemBug. Right now the two WITs (the new one and workitem.SystemBug) are compatible.

ok, let me check that

fixed: I'm now using a WorkItemType created by the TestFixture.

kwk · 2017-10-02T16:42:51Z

search/search_repository_whitebox_test.go

-		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)


May I ask why you don't use a test fixture here?

I can do that, indeed

kwk · 2017-10-02T16:43:34Z

workitem/number_sequence/workitem_number_sequence.go

 	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"`


This is one of the most important changes of this PR, isn't it?

yes, indeed

fabric8cd · 2017-10-02T17:02:49Z

@xcoulon snapshot fabric8-wit image is available for testing. docker pull docker.io/fabric8/fabric8-wit:SNAPSHOT-PR-1677-3

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

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 <xcoulon@redhat.com>

codecov-io · 2017-10-03T16:08:21Z

Codecov Report

Merging #1677 into master will increase coverage by 0.01%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master    #1677      +/-   ##
==========================================
+ Coverage   56.65%   56.66%   +0.01%     
==========================================
  Files         125      126       +1     
  Lines       14533    14549      +16     
==========================================
+ Hits         8233     8244      +11     
- Misses       5662     5666       +4     
- Partials      638      639       +1

Impacted Files	Coverage Δ
...rkitem/number_sequence/workitem_number_sequence.go	`50% <0%> (ø)`
space/space.go	`62.59% <100%> (+0.44%)`	⬆️
workitem/link/link_revision_repository.go	`80.64% <100%> (ø)`	⬆️
gormsupport/cleaner/db_clean.go	`100% <100%> (ø)`	⬆️
search/search_repository.go	`75.24% <100%> (-0.1%)`	⬇️
workitem/workitem_revision_repository.go	`82.35% <100%> (ø)`	⬆️
account/identity.go	`51.57% <100%> (ø)`	⬆️
workitem/workitemtype_repository.go	`74.68% <100%> (ø)`	⬆️
workitem/workitem_repository.go	`68.94% <75%> (-0.09%)`	⬇️
...er_sequence/workitem_number_sequence_repository.go	`80% <80%> (ø)`
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e28b5cd...239517b. Read the comment docs.

xcoulon · 2017-10-03T16:11:09Z

@sbose can you please take a look at the refactoring of the search_repository_whitebox_test.go file in commit 461eeca to confirm if the removal of the URL in the search keywords makes sense ?

fabric8cd · 2017-10-03T16:16:18Z

@xcoulon snapshot fabric8-wit image is available for testing. docker pull docker.io/fabric8/fabric8-wit:SNAPSHOT-PR-1677-4

pranavgore09 · 2017-10-05T10:04:43Z

search/search_repository_whitebox_test.go

 				}
+				assert.True(s.T(),
+					strings.Contains(workItemTitle, keyWord) || strings.Contains(workItemDescription, keyWord),


okay! a question
what would happen when user types in just 10 to search. Will it return a WI with id=10? Can we have a test for that too?
Currently, we are using this search when a user wants to link WIs.

@pranavgore09 I added a test for searching by WI number, but not by id . Also, I used the number: prefix, but maybe I should also add a test to search without the prefix ? My question is: ID is supposed to be an internal UUID, so do we expect users to use it to look-up a work item ?

oh! yeah, I was concerned with UUID in the search bar, but yes we may not need that case.
Generally, number will be used. Not sure if UI need to make this change or they are already using number only. I will confirm with the team and respond here.
thanks @xcoulon for an explanation.
And test without prefix would be helpful IMHO

Yeah on UI we are using number while search and not uuid!

aslakknutsen · 2017-10-05T10:05:53Z

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)


never mind, I forgot to remove that line ;) (there was a r.db.LogMode(true) on top of this one, which I used to check some requests)

fabric8cd · 2017-10-05T10:06:57Z

@xcoulon snapshot fabric8-wit image is available for testing. docker pull docker.io/fabric8/fabric8-wit:SNAPSHOT-PR-1677-5

aslakknutsen · 2017-10-05T10:09:56Z

workitem/number_sequence/workitem_number_sequence_repository.go

+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)


Evaluate UPDATE table set number = number+1 where... RETURNING number to avoid get and save on each call

good point, I'll try that

actually, I could probably get rid of the SELECT... FOR UPDATE statement using an UPSERT ... RETURNING as you suggested below, in which case there might be no need for the new ID column either. Let me try that

aslakknutsen · 2017-10-05T10:12:25Z

workitem/number_sequence/workitem_number_sequence.go

 	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"`


Why not use SpaceID as primary key?

I thought GORM might be confused if I tried to INSERT a new record while the primary key was provided (it might run an UPDATE query instead)

aslakknutsen · 2017-10-05T10:15:05Z

search/search_repository.go

@@ -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")


yes, so I have colors in my console when working on it :) Obviously, this was not intended to be part of the PR...

aslakknutsen · 2017-10-05T10:16:14Z

workitem/number_sequence/workitem_number_sequence_repository.go

+
+// WorkItemNumberSequenceRepository the interface for the work item number sequence repository
+type WorkItemNumberSequenceRepository interface {
+	Create(ctx context.Context, spaceID uuid.UUID) (*WorkItemNumberSequence, error)


Do we care about create? You have all the info on nextVal to create it

actually, I think it's worth creating the "sequence" upon space creation, so we can effectively lock the row in the table when adding a work item. I suspect that this is what causes the duplicate key issue during the tests.

aslakknutsen · 2017-10-05T10:16:33Z

workitem/number_sequence/workitem_number_sequence_repository.go

+// 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)


Why return WorkItemNumberSequence and not just the Number?

good point, indeed

aslakknutsen · 2017-10-05T10:39:13Z

workitem/number_sequence/workitem_number_sequence_repository.go

+// 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}


CurrVal could be set to 0 as default value on table work_item_number_sequences

right, as-per golang, there's no need to make it explicit here, indeed. But I liked the idea of showing that the sequence is initialized to 0 upon creation, and the first call to NextVal() will return 1

aslakknutsen · 2017-10-05T10:43:42Z

Create and NextVal could be combined into a single UPSERT SQL:

INSERT INTO work_item_number_sequences (space_id) VALUES (?)
ON CONFLICT (space_id) DO UPDATE SET number = work_item_number_sequences.number+1
RETURNING number

(roughly)

baijum · 2017-10-05T10:57:05Z

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;


You can add a test for checking the existence of id column in migration/migration_blackbox_test.go

yes, you're right

xcoulon · 2017-10-05T12:33:55Z

Create and NextVal could be combined into a single UPSERT SQL

@aslak yes, we could actually having this kind of UPSERT statement, but what if we have 2 concurrent inserts of work items and no number sequence for the space yet ? there will be no LOCK on the work_item_number_sequences table and we may have the infamous duplicate key error as both work items would have number 1. Or am I missing something ?

aslakknutsen · 2017-10-05T12:41:10Z

@xcoulon Nothing to lock, but SpaceID is still a primary ID and is unique. Both can't work even concurrently. I would suspect the 'second' would hit on conflict

kwk · 2017-10-09T13:30:14Z

search/search_repository_whitebox_test.go

 			},
 			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 {


Nice try :). Notice that all but the last two WIs, all belong to sbose78 and in your change they all belong to one entity. Please see my longish comment on my first idea how to fix it and my second attempt on why even that is bad: #1677 (review)

why does it cause any issue ? we don't search by creator or assignee anyways

Okay, then. I didn't look at the test itself but only at the test data. And you didn't refactor it 1:1, which was the only cause of my complaint ;)

kwk · 2017-10-09T13:30:54Z

search/search_repository_whitebox_test.go

+				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()},


You should use tf.IdentityByUsername("creator").ID.String() instead.

fabric8cd · 2017-10-09T13:31:16Z

@xcoulon snapshot fabric8-wit image is available for testing. docker pull docker.io/fabric8/fabric8-wit:SNAPSHOT-PR-1677-10

kwk · 2017-10-09T13:31:26Z

search/search_repository_whitebox_test.go

+				workitem.SystemDescription: rendering.NewMarkupContentFromLegacy("Description"),
+				workitem.SystemCreator:     fxt.Identities[0].ID.String(),
+				workitem.SystemAssignees:   []string{fxt.Identities[1].ID.String()},
+				workitem.SystemState:       "closed",


Please use workitem.SystemStateClosed

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

fabric8cd · 2017-10-09T15:03:07Z

@xcoulon snapshot fabric8-wit image is available for testing. docker pull docker.io/fabric8/fabric8-wit:SNAPSHOT-PR-1677-11

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

fabric8cd · 2017-10-09T15:17:58Z

@xcoulon snapshot fabric8-wit image is available for testing. docker pull docker.io/fabric8/fabric8-wit:SNAPSHOT-PR-1677-12

kwk · 2017-10-09T15:54:17Z

search/search_repository_whitebox_test.go

+		tf.Spaces(1, func(fxt *tf.TestFixture, idx int) error {
+			fxt.Spaces[idx].OwnerId = fxt.Identities[0].ID
+			return nil
+		}), tf.WorkItemTypes(1))


The fixture's comment is a little bit off because you're not creating 2 work items at all.

kwk · 2017-10-09T15:57:32Z

search/search_repository_whitebox_test.go

-				Fields: map[string]interface{}{
+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"})),


Use NewTestFixture and give it your test variable, for the fixture to automatically fail if something goes wrong. Then you can get rid of the require.Nil(s.T(), err) below.

kwk · 2017-10-09T16:01:43Z

search/search_repository_whitebox_test.go

+	// 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


No need to write this down explicitly as a space owner is automatically set to the first identity. Look at makeSpaces().

That being said, the whole fixture (6 lines) can be compressed to just:

tf.NewTestFixture(s.DB, s.T(), tf.Identities(2), tf.Spaces(1), tf.WorkItemTypes(1))

Below you're using baseFxt.Identities[0].ID.String(), without specifying an identities username. Therefore you might as well just put my fixture recipe (tf.Identities(2), tf.Spaces(1), tf.WorkItemTypes(1)) from above into every test fixture of every sub-test below. Then you can get rid of these three lines that adjusted the space and the creator:

fxt.WorkItems[idx].SpaceID = baseFxt.Spaces[0].ID fxt.WorkItems[idx].Type = baseFxt.WorkItemTypes[0].ID //... workitem.SystemCreator: baseFxt.Identities[0].ID.String(),

Please note, that it would help me a lot if you do this because when space templates come in and you create two fixtures that use each other, things can become complicated to adjust for me.

If the sate of a workitem is also irrelevant, you can simply ignore it and instead of specifying the whole map of Fields you might as well only set those keys you want. Know, that we pre-fill the state to workitem.SystemStateNew and the creator field to the first identity known to the fixture.

ok, I simplified the test here. Please let mw know if it's good for you.

@xcoulon Now, that is reaaaaaly clean now. Let me have a look at the rest of this PR.

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 <xcoulon@redhat.com>

fabric8cd · 2017-10-10T08:01:18Z

@xcoulon snapshot fabric8-wit image is available for testing. docker pull docker.io/fabric8/fabric8-wit:SNAPSHOT-PR-1677-13

kwk · 2017-10-10T13:57:22Z

test/testfixture/make_functions.go

 		}
 		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])


Makes sense.

kwk · 2017-10-10T13:59:50Z

workitem/number_sequence/workitem_number_sequence.go

@@ -18,3 +20,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)


Please make this SprintID instead of SprintId. See also https://github.com/golang/go/wiki/CodeReviewComments#initialisms

kwk · 2017-10-10T14:02:43Z

workitem/number_sequence/workitem_number_sequence_repository.go

+}
+
+// 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) (*int, error) {


We should add a test for this method maybe. WDYT @xcoulon ?

I added a test with concurrent routines, using a WaitGroup

kwk · 2017-10-10T14:03:42Z

workitem/workitem_repository.go

+		db,
+		numbersequence.NewWorkItemNumberSequenceRepository(db),
+		&GormWorkItemTypeRepository{db},
+		&GormRevisionRepository{db},


Can you maybe add struct member names like member: value?

kwk · 2017-10-10T14:22:22Z

workitem/workitem_repository_blackbox_test.go

+		tf.WorkItemTypes(1, func(fxt *tf.TestFixture, idx int) error {
+			fxt.WorkItemTypes[idx].SpaceID = fxt.Spaces[0].ID
+			return nil
+		}))


Please use fxt := tf.NewTestFixture(s.T(), s.DB, tf.CreateWorkItemEnvironment()) instead of this complicated fixture. What you do in this fixture is exactly what is being done for you already btw.

ok, that's actually simpler ;)

kwk · 2017-10-10T14:43:18Z

workitem/workitem_repository_blackbox_test.go

+		assert.Equal(s.T(), 0, report.failures)
+	}
+
+	// then


This // then looks quite lonely ;)

Should this test also double check that no WI did get the same Number?

no, WI with same numbers will cause a duplicate key error at the DB level.

kwk · 2017-10-10T14:48:44Z

workitem/workitem_repository_blackbox_test.go

+		}(i)
+	}
+	// wait for all items to be created
+	for i := 0; i < routines; i++ {


In order to properly wait for all go routines to finish, you should use a wait group as we do here.

var wg sync.WaitGroup for i := 0; i < 10; i++ { wg.Add(1) go func() { defer wg.Done() // DO HERE WHAT YOU DO IN YOUR GO ROUTINES }() } wg.Wait()

Then you can do this to access each report and get rid of the routines variable for good, since it is only used when spawning the go routines.

for _, report := range done { }

that syntax is nicer and I can use the WaitGroup feature indeed , but AFAICT, my code already waits for routines to complete. Or did I miss something ?

From the top of my head (not writing go routines every day): I think you have a blocking read from a channel that waits until there is nothing more to read. But you don't have a close call of that channel and I just find it much easier with the wait group.

kwk · 2017-10-10T14:59:40Z

search/search_repository_whitebox_test.go


-		sr := NewGormSearchRepository(tx)
+func (s *searchRepositoryWhiteboxTest) searchFor(spaceID uuid.UUID, searchQuery string) ([]workitem.WorkItem, error) {


Can't we just put this above the test cases in func (s *searchRepositoryWhiteboxTest) TestSearch() {? This function searchFor is only used there and I would consider our tests much easier to maintain if we can avoid any helper functions. In the code of TestSearch we would only have to replace

searchResults, err := s.searchFor(fxt.Spaces[0].ID, searchQuery)

with

spaceIDStr := fxt.Spaces[0].ID.String() searchResult, _, err := sr.SearchFullText(ctx, fmt.Sprintf("\"%s\"", searchQuery), &start, &limit, &spaceIDStr)

correct?

kwk · 2017-10-10T15:00:07Z

search/search_repository_whitebox_test.go

-		workItemList, _, err := sr.SearchFullText(ctx, searchString, &start, &limit, nil)
-		if err != nil {
-			s.T().Fatal("Error gettig search result ", err)
+func verify(t *testing.T, searchQuery string, searchResults []workitem.WorkItem, expectedCount int) {


A comment on this method would be nice. What does it verify?

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

fabric8cd · 2017-10-10T16:35:13Z

@xcoulon snapshot fabric8-wit image is available for testing. docker pull docker.io/fabric8/fabric8-wit:SNAPSHOT-PR-1677-14

kwk

LGTM. Except the missing additional WIs.

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 <xcoulon@redhat.com>

fabric8cd · 2017-10-11T10:18:39Z

@xcoulon snapshot fabric8-wit image is available for testing. docker pull docker.io/fabric8/fabric8-wit:SNAPSHOT-PR-1677-15

xcoulon · 2017-10-11T11:40:58Z

[test]

kwk

I'm requesting changes mostly due to the log change that you did. The sub-tests would also be nice.

kwk · 2017-10-11T12:30:30Z

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{}{


I suggest to just reduce the test output by setting F8_LOG_LEVEL=error instead of changing the level code-wise. Most repo create functions use log.Info. Also run tests with go test instead of go test -v

I know I can do that (F8_LOG_LEVEL=error and go test without the verbose flag), but do we really need those logs statements at the INFO level ?

Probably not. Did you cover all "created X" log messages? Then it is fine to change to Debug if you ask me. I mostly feared mixing concerns of this PR.

kwk · 2017-10-11T12:41:05Z

search/search_repository_whitebox_test.go

+		assert.Equal(t, queryNumber, searchResults[0].Number)
+	})
+
+	s.T().Run("Search by number - multiple matches", func(t *testing.T) {


What do you think of nesting those tests to get a better context and not repeat yourself?

s.T().Run("by number", func(t*testing.T)) { t.Run("single match", func(t*testing.T)) { }) t.Run("multiple match", func(t*testing.T)) { }) }) s.T().Run("by URL", func(t*testing.T)) { t.Run("single match", func(t*testing.T)) { }) t.Run("multiple match", func(t*testing.T)) { }) })

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

fabric8cd · 2017-10-11T13:06:57Z

@xcoulon snapshot fabric8-wit image is available for testing. docker pull docker.io/fabric8/fabric8-wit:SNAPSHOT-PR-1677-16

kwk · 2017-10-11T13:14:57Z

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{}{


Probably not. Did you cover all "created X" log messages? Then it is fine to change to Debug if you ask me. I mostly feared mixing concerns of this PR.

fabric8cd · 2017-10-11T13:34:25Z

@xcoulon snapshot fabric8-wit image is available for testing. docker pull docker.io/fabric8/fabric8-wit:SNAPSHOT-PR-1677-17

xcoulon · 2017-10-12T14:16:35Z

thanks for the reviews, @kwk and @aslakknutsen !

xcoulon requested review from kwk, aslakknutsen and sbose78 October 2, 2017 16:30

xcoulon force-pushed the Issue1676_test_failures_duplicate_key branch from 531e75a to 3481ff7 Compare October 2, 2017 16:31

kwk reviewed Oct 2, 2017

View reviewed changes

kwk suggested changes Oct 2, 2017

View reviewed changes

xcoulon added 3 commits October 2, 2017 19:24

Taking comments into account

79ce30a

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

Merge branch 'master' into Issue1676_test_failures_duplicate_key

d175ffd

Merge branch 'master' into Issue1676_test_failures_duplicate_key

b01a50c

xcoulon changed the title ~~Issue1676 test failures duplicate key~~ Random test failures because of work item creation error Oct 5, 2017

xcoulon changed the title ~~Random test failures because of work item creation error~~ Random test failures because of work item creation error (#1676) Oct 5, 2017

pranavgore09 reviewed Oct 5, 2017

View reviewed changes

aslakknutsen reviewed Oct 5, 2017

View reviewed changes

aslakknutsen suggested changes Oct 5, 2017

View reviewed changes

aslakknutsen reviewed Oct 5, 2017

View reviewed changes

baijum reviewed Oct 5, 2017

View reviewed changes

kwk reviewed Oct 9, 2017

View reviewed changes

Refactor TestSearch (again)

c74f9b0

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

Using constants for work item states

15a49f8

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

kwk suggested changes Oct 9, 2017

View reviewed changes

xcoulon added 2 commits October 10, 2017 09:51

Simplify tests

9df5af2

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 <xcoulon@redhat.com>

Merge branch 'master' into Issue1676_test_failures_duplicate_key

65f499b

kwk reviewed Oct 10, 2017

View reviewed changes

xcoulon added 2 commits October 10, 2017 18:29

More review comments

fb7379e

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

Merge branch 'master' into Issue1676_test_failures_duplicate_key

21d67d8

kwk approved these changes Oct 11, 2017

View reviewed changes

Create 10 work items per test

0cfb792

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 <xcoulon@redhat.com>

kwk suggested changes Oct 11, 2017

View reviewed changes

refactor the search tests using subtests, and fix race condition in c…

6cc2e96

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

kwk approved these changes Oct 11, 2017

View reviewed changes

Merge branch 'master' into Issue1676_test_failures_duplicate_key

239517b

aslakknutsen approved these changes Oct 12, 2017

View reviewed changes

xcoulon merged commit f150285 into fabric8-services:master Oct 12, 2017

xcoulon mentioned this pull request Oct 19, 2017

pq: duplicate key value violates unique constraint #1644

Closed


		sr := NewGormSearchRepository(tx)
		func (s *searchRepositoryWhiteboxTest) searchFor(spaceID uuid.UUID, searchQuery string) ([]workitem.WorkItem, error) {

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

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

Conversation

xcoulon commented Oct 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabric8cd commented Oct 2, 2017

kwk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabric8cd commented Oct 2, 2017

codecov-io commented Oct 3, 2017 • edited Loading

Codecov Report

xcoulon commented Oct 3, 2017

fabric8cd commented Oct 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabric8cd commented Oct 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aslakknutsen commented Oct 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xcoulon commented Oct 5, 2017

aslakknutsen commented Oct 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabric8cd commented Oct 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabric8cd commented Oct 9, 2017

fabric8cd commented Oct 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwk Oct 10, 2017 • edited Loading

Choose a reason for hiding this comment

fabric8cd commented Oct 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xcoulon commented Oct 2, 2017 •

edited

Loading

codecov-io commented Oct 3, 2017 •

edited

Loading

aslakknutsen commented Oct 5, 2017 •

edited

Loading

aslakknutsen commented Oct 5, 2017 •

edited

Loading

kwk Oct 10, 2017 •

edited

Loading