Skip to content

Improve LFS tests + fix lfs url refs + keep path upper/lowercase in db. #3092

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

Jump to bottom
Merged
merged 14 commits into from
Dec 8, 2017
Merged

Improve LFS tests + fix lfs url refs + keep path upper/lowercase in db. #3092

merged 14 commits into from
Dec 8, 2017

Conversation

sapk
Copy link
Member

@sapk sapk commented Dec 4, 2017 •
edited
Loading

All in the title.

Fix #3101
@sapk sapk changed the title [WIP] Improve LFS tests + fix lfs url refs. [WIP] Improve LFS tests + fix lfs url refs + keep path upper/lowercase in db. Dec 5, 2017
@sapk sapk changed the title [WIP] Improve LFS tests + fix lfs url refs + keep path upper/lowercase in db. Improve LFS tests + fix lfs url refs + keep path upper/lowercase in db. Dec 5, 2017
@sapk
Copy link
Member Author

sapk commented Dec 5, 2017

Need dockhippie/golang#2
@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 5, 2017
@flozz flozz mentioned this pull request Dec 6, 2017
Closed
7 tasks
@sapk
Copy link
Member Author

sapk commented Dec 6, 2017

I need to setup a separate repo to not influence other repos tests since clone after will not work because the server is not really started and can't get file for lfs.
@codecov-io
Copy link

codecov-io commented Dec 6, 2017 •
edited
Loading

Codecov Report

Merging #3092 into master will increase coverage by 0.76%.
The diff coverage is 83.33%.

Impacted file tree graph

@@ Coverage Diff @@ ## master #3092 +/- ## ========================================== + Coverage 34.06% 34.82% +0.76%  ========================================== Files 274 274 Lines 40026 40026 ========================================== + Hits 13636 13941 +305  + Misses 24455 24086 -369  - Partials 1935 1999 +64
Impacted Files Coverage Δ
routers/repo/view.go 25.41% <0%> (ø) ⬆️
modules/lfs/server.go 35.01% <100%> (+31.29%) ⬆️
models/lfs_lock.go 69.76% <100%> (ø) ⬆️
models/repo_list.go 65.62% <0%> (-1.57%) ⬇️
models/repo_indexer.go 48.54% <0%> (+0.48%) ⬆️
modules/auth/auth.go 52.51% <0%> (+2.79%) ⬆️
models/repo.go 41.28% <0%> (+2.94%) ⬆️
modules/process/manager.go 81.15% <0%> (+4.34%) ⬆️
modules/lfs/locks.go 52.08% <0%> (+4.68%) ⬆️
routers/api/v1/repo/repo.go 40.1% <0%> (+7.1%) ⬆️
... and 6 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 aecfc56...3ec055e. Read the comment docs.
@strk
Copy link
Member

strk commented Dec 6, 2017

LGTM
@tboerger tboerger 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 6, 2017
@lunny lunny added this to the 1.4.0 milestone Dec 7, 2017
@lunny lunny added the type/bug label Dec 7, 2017
@lunny
Copy link
Member

lunny commented Dec 7, 2017

LGTM
@tboerger tboerger 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 7, 2017
@lunny
Copy link
Member

lunny commented Dec 7, 2017

CI failed.
ethantkoenig
ethantkoenig reviewed Dec 7, 2017
View reviewed changes
Copy link
Member

@ethantkoenig ethantkoenig left a comment

Choose a reason for hiding this comment

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

One general comment is that there is a lot of repeated boilerplate (e.g. making commits, pushing). While it would be nice to consolidate, I'm guessing there are other tests with similar boilerplate, so it's probably best to do wide-scale refactoring in a separate PR.
integrations/git_test.go Outdated
})

u.Path = "user2/repo-tmp-17.git"
u.User = url.UserPassword("user2", "password")
Copy link
Member

Choose a reason for hiding this comment

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

nit: use the userPassword constant (also in 162).
@sapk
Copy link
Member Author

sapk commented Dec 7, 2017 •
edited
Loading

@ethantkoenig Replaced with userPassword and yes some integration tests could be refactor (in another PR) and maybe we could use a lib in this case to generate random file to commit.
@lunny
Copy link
Member

lunny commented Dec 7, 2017

@sapk maybe you could force push once since the Drone just recovered.
@lafriks
Copy link
Member

lafriks commented Dec 7, 2017

@sapk test failed
@sapk sapk force-pushed the test-more-lfs branch 6 times, most recently from 98f40cd to 0880e19 Compare December 7, 2017 21:57
@lunny
Copy link
Member

lunny commented Dec 8, 2017

@sapk CI still failed because of fmt-check
@sapk
Copy link
Member Author

sapk commented Dec 8, 2017 •
edited
Loading

@lunny I know but I fix it but drone keep to stay on that commit. I retry to start multiple time, squash the commit fixing fmt ... with no luck
@sapk
Copy link
Member Author

sapk commented Dec 8, 2017

@lafriks thx, but drone seems broken and block on an unrelated test since 1hour ...
@lunny lunny merged commit ef78309 into go-gitea:master Dec 8, 2017
@lafriks
Copy link
Member

lafriks commented Dec 8, 2017 •
edited
Loading

@sapk please submit backport to 1.3 branch this bug was just in master and not in 1.3.0, right?
@sapk
Copy link
Member Author

sapk commented Dec 8, 2017

@lafriks It should not be in 1.3
@sapk
Copy link
Member Author

sapk commented Dec 8, 2017

thx all for the help.
@sapk sapk deleted the test-more-lfs branch December 9, 2017 21:46
@go-gitea go-gitea locked and limited conversation to collaborators Nov 23, 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/bug

7 participants

@sapk @codecov-io @strk @lunny @lafriks @ethantkoenig @tboerger