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.

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

Conversation

zeripath
Copy link
Contributor

Fixes #6977 by force waiting till the repository has finished updating the index.

@zeripath zeripath added this to the 1.9.0 milestone May 20, 2019
@codecov-io
Copy link

codecov-io commented May 20, 2019

Codecov Report

Merging #7004 into master will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #7004      +/-   ##
=========================================
+ Coverage   41.49%   41.5%   +<.01%     
=========================================
  Files         440     440              
  Lines       59453   59453              
=========================================
+ Hits        24668   24673       +5     
+ Misses      31566   31560       -6     
- Partials     3219    3220       +1
Impacted Files Coverage Δ
modules/log/event.go 65.98% <0%> (+1.52%) ⬆️
models/unit.go 67.56% <0%> (+5.4%) ⬆️

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 1f84970...35ba40b. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 20, 2019
@lunny
Copy link
Member

lunny commented May 21, 2019

LGTM

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels May 21, 2019
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.

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)

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels May 21, 2019
@zeripath zeripath merged commit 84bfd00 into go-gitea:master May 21, 2019
@zeripath zeripath deleted the fix-#6977-Force-update-index-and-wait branch May 21, 2019 19:11
jeffliu27 pushed a commit to jeffliu27/gitea that referenced this pull request Jul 18, 2019
* Fix TestSearchRepo by waiting till indexing is done

* Update integrations/repo_search_test.go

* changes as per @mrsdizzie
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TestSearchRepos is broken
5 participants