Skip to content

Fix TestSearchRepo by waiting till indexing is done #7004

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

Merged
merged 5 commits into from
May 21, 2019
Merged

Fix TestSearchRepo by waiting till indexing is done #7004

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
00ed170
Fix TestSearchRepo by waiting till indexing is done
zeripath May 20, 2019
5601344
Update integrations/repo_search_test.go
zeripath May 21, 2019
73ffb93
Merge branch 'master' into fix-#6977-Force-update-index-and-wait
zeripath May 21, 2019
bb51da2
changes as per @mrsdizzie
zeripath May 21, 2019
35ba40b
Merge branch 'fix-#6977-Force-update-index-and-wait' of github.com:ze…
zeripath May 21, 2019
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
21 changes: 21 additions & 0 deletions integrations/repo_search_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,12 @@
package integrations

import (
"log"
"net/http"
"testing"
"time"

"code.gitea.io/gitea/models"

"github.com/PuerkitoBio/goquery"
"github.com/stretchr/testify/assert"
Expand All @@ -27,6 +31,23 @@ func resultFilenames(t testing.TB, doc *HTMLDoc) []string {
func TestSearchRepo(t *testing.T) {
prepareTestEnv(t)

repo, err := models.GetRepositoryByOwnerAndName("user2", "repo1")
assert.NoError(t, err)

models.UpdateRepoIndexer(repo)

log.Printf("Waiting for indexing\n")

i := 0
for {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
for {
for i < 60 {
if (repo.IndexerStatus != nil && len(repo.IndexerStatus.CommitSha) != 0) {

Might be a bit more readable, but thats just a small nit.

if (repo.IndexerStatus != nil && len(repo.IndexerStatus.CommitSha) != 0) || i > 60 {
break
}
time.Sleep(1 * time.Second)
i++
}
log.Printf("Indexing took: %d s\n", i)
Copy link
Member

Choose a reason for hiding this comment

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

What happens if the indexing didn't finish after 60 seconds? Should this check to see if it finished and fail with a message if not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure that's much we can really infer much about indexing - the API is very minimal - if anything inferring that the indexing has finished by checking IndexerStatus,CommitSha is non-zero is probably assuming too much. Yes it appears to work and looking at the source code that appears the only way to check but I don't think we can say much more than that.

If after 60s the IndexerStatus.CommitSha hasn't been set then maybe the "API" of indexer has changed? At present this test would just check and hopefully pass - probably taking too long to do so. If however, you enforce that the IndexerStatus has to be set then we're now forcing an API - one which I would not design this way. I guess it depends on how much you want to enforce an API on UpdateRepoIndexer.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, that all makes sense and I wouldn't want to make any big changes or block this fix -- just to avoid confusion by saying it has finished if really we just hit the time limit. Maybe just:

if i < 60 {
log.Printf("Indexing took: %d s\n", i)
}  else {
log.Printf("Waited the limit: %d s for indexing, continuing\n", i)
}

Which keeps the current assumptions from the loop but specifies if we think it finished vs it reached the time limit (since reaching the time limit is probably the only thing we would know for sure here)


req := NewRequestf(t, "GET", "/user2/repo1/search?q=Description&page=1")
resp := MakeRequest(t, req, http.StatusOK)

Expand Down