Skip to content

WIP: Add a concurrent test for issue creation #7950

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

WIP: Add a concurrent test for issue creation #7950

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions integrations/issue_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,13 @@
package integrations

import (
"context"
"fmt"
"net/http"
"path"
"strconv"
"strings"
"sync"
"testing"

"code.gitea.io/gitea/models"
Expand Down Expand Up @@ -184,3 +187,34 @@ func TestIssueCommentClose(t *testing.T) {
val := htmlDoc.doc.Find(".comment-list .comments .comment .render-content p").First().Text()
assert.Equal(t, "Description", val)
}

func TestNewIssueConcurrent(t *testing.T) {

prepareTestEnv(t)
session := loginUser(t, "user2")

f := func(ctx context.Context, cancel context.CancelFunc, wg *sync.WaitGroup) {
defer wg.Done()

select {
case <-ctx.Done():
return
default:
}
testNewIssue(t, session, "user2", "repo1", fmt.Sprintf("Title"), "Description")
Copy link
Member

Choose a reason for hiding this comment

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

You are stressing the database here (what you need) but also stressing other layers, like the http client & server, the computer resources, etc. I think it would be preferrable if you'd call the models/issue.go methods instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. I'll try to figure out how to do that.

if t.Failed() {
Copy link
Member

Choose a reason for hiding this comment

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

I think a great deal of times this function will fail for reasons unrelated to a duplicate key (which is what this test is for) and give false negatives. It's important to check the error and verify it is actually a duplicate key; other errors should be ignored. Gitea will be built in all kinds of environments, e.g. small Raspberry Pi boards.
Also, this might extend the test times beyond the alloted values in the drone (something to be dealt with).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's true. But I am afraid that there are some other undiscovered bugs, so an integration test might be helpful to an extent if it can reveal them.

cancel()
return
}
}

wg := &sync.WaitGroup{}
ctx, cancel := context.WithCancel(context.Background())
defer cancel()

for i := 0; i < 1000; i++ {
Copy link
Member

Choose a reason for hiding this comment

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

Note: launching 1,000 threads will not result in 1,000 simultaneous calls to the database, as the database can have a reduced connection pool. I believe that there are defaults in golang for the number of simultaneous threads too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense. Noted.
I thought that it could minimize the probability of false negative.
I'll revisit my presumptions considering there is a limited number of connections.

wg.Add(1)
go f(ctx, cancel, wg)
}
wg.Wait()
}