Skip to content
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

improve testing for pgsql empty repository #22223

Merged
merged 1 commit into from
Dec 23, 2022

Conversation

lunny
Copy link
Member

@lunny lunny commented Dec 23, 2022

sometimes ci failure https://drone.gitea.io/go-gitea/gitea/64840/3/8
This PR is to know what happened.

@wxiaoguang
Copy link
Contributor

I do not think this PR makes sense. Actually, it should already be clear about what happened:

[E] [63a47c93] Repository 34:user21/golang has a broken repository on the file system: 
/tmp/tmp.zA74IwWOdz/tests/integration/gitea-integration-pgsql/gitea-repositories/user21/golang.git 
Error: no such file or directory

My guess is that the test code of TestEmptyRepo is not stable.

	emptyRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{}, unittest.Cond("is_empty = ?", true))
	owner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: emptyRepo.OwnerID})

The query WHERE is_emtpy=true is not stable, it may return different results (it depends on database's behavior)

If the returned repo & user have valid git repo on filesystem, then the test passes (most time).

But, if something like user21's repo is returned, since it doesn't have any valid git repo on filesystem, the test fails.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 23, 2022
@lunny
Copy link
Member Author

lunny commented Dec 23, 2022

I do not think this PR makes sense. Actually, it should already be clear about what happened:

[E] [63a47c93] Repository 34:user21/golang has a broken repository on the file system: 
/tmp/tmp.zA74IwWOdz/tests/integration/gitea-integration-pgsql/gitea-repositories/user21/golang.git 
Error: no such file or directory

My guess is that the test code of TestEmptyRepo is not stable.

	emptyRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{}, unittest.Cond("is_empty = ?", true))
	owner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: emptyRepo.OwnerID})

The query WHERE is_emtpy=true is not stable, it may return different results (it depends on database's behavior)

If the returned repo & user have valid git repo on filesystem, then the test passes (most time).

But, if something like user21's repo is returned, since it doesn't have any valid git repo on filesystem, the test fails.

I do not think this PR makes sense. Actually, it should already be clear about what happened:

[E] [63a47c93] Repository 34:user21/golang has a broken repository on the file system: 
/tmp/tmp.zA74IwWOdz/tests/integration/gitea-integration-pgsql/gitea-repositories/user21/golang.git 
Error: no such file or directory

My guess is that the test code of TestEmptyRepo is not stable.

	emptyRepo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{}, unittest.Cond("is_empty = ?", true))
	owner := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: emptyRepo.OwnerID})

The query WHERE is_emtpy=true is not stable, it may return different results (it depends on database's behavior)

If the returned repo & user have valid git repo on filesystem, then the test passes (most time).

But, if something like user21's repo is returned, since it doesn't have any valid git repo on filesystem, the test fails.

Thanks! Done.

Copy link
Contributor

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

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

LGTM, while it may be easier to just use WHERE id=xxx to query an empty repo explicitly.

@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 Dec 23, 2022
@KN4CK3R
Copy link
Member

KN4CK3R commented Dec 23, 2022

While looking at the code I thought the same as @wxiaoguang , why not use a specific repo which meets the conditions?

@lunny
Copy link
Member Author

lunny commented Dec 23, 2022

@KN4CK3R @wxiaoguang done.

@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 Dec 23, 2022
@jolheiser jolheiser merged commit 2b0b563 into go-gitea:main Dec 23, 2022
@lunny lunny deleted the lunny/improve_pg_test branch December 24, 2022 01:23
zjjhot added a commit to zjjhot/gitea that referenced this pull request Dec 26, 2022
* upstream/main:
  Add Mermaid copy button, avoid unnecessary tooltip hide (go-gitea#22225)
  [skip ci] Updated licenses and gitignores
  Improve testing for pgsql empty repository (go-gitea#22223)
  JS refactors (go-gitea#22227)
  Check primary keys for all tables and drop ForeignReference (go-gitea#21721)
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
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.

None yet

6 participants