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 LFS tests + fix lfs url refs + keep path upper/lowercase in db. #3092

Merged
merged 14 commits into from
Dec 8, 2017

Conversation

sapk
Copy link
Member

@sapk sapk commented Dec 4, 2017

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
@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

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.

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.

})

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

@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

@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

@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LFS: unable to push objects tracked by git LFS
7 participants