Skip to content

Do not show full lfs file on error in git_test.go:rawTest() #14980

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
merged 5 commits into from
Mar 14, 2021

Conversation

zeripath
Copy link
Contributor

If there is a problem uploading to LFS it is possible for the raw
endpoint to return a very large file when a pointer file is expected
This will then cause the drone logs to fill up unnecessarily with
the contents of the very large file.

If the file returned from raw is of the incorrect size we should
therefore not test it see if it contains the pointer file
and just declare that it is incorrect.

Signed-off-by: Andrew Thornton [email protected]

If there is a problem uploading to LFS it is possible for the raw
endpoint to return a very large file when a pointer file is expected
This will then cause the drone logs to fill up unnecessarily with
the contents of the very large file.

If the file returned from raw is of the incorrect size we should
therefore not test it see if it contains the pointer file
and just declare that it is incorrect.

Signed-off-by: Andrew Thornton <[email protected]>
@zeripath zeripath added type/testing skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. labels Mar 12, 2021
@zeripath zeripath added this to the 1.14.0 milestone Mar 12, 2021
Signed-off-by: Andrew Thornton <[email protected]>
@@ -216,7 +216,10 @@ func rawTest(t *testing.T, ctx *APITestContext, little, big, littleLFS, bigLFS s
req = NewRequest(t, "GET", path.Join("/", username, reponame, "/raw/branch/master/", littleLFS))
resp = session.MakeRequest(t, req, http.StatusOK)
assert.NotEqual(t, littleSize, resp.Body.Len())
assert.Contains(t, resp.Body.String(), models.LFSMetaFileIdentifier)
assert.LessOrEqual(t, resp.Body.Len(), 1024)
if resp.Body.Len() != littleSize && resp.Body.Len() <= 1024 {
Copy link
Member

Choose a reason for hiding this comment

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

This condition is already asserted above, right?
Can't we check for the exact expected length?
Are there tests for small and large files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason for the changes being the way they are is because of the contains below this line.

If the resp.Body does not contain the expected value it will be emitted whole into the test response.

This is bad and completely unreadable - we therefore need to skip doing that test in that case.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 13, 2021
@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 Mar 13, 2021
@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 Mar 13, 2021
@zeripath
Copy link
Contributor Author

make lgtm work

@zeripath zeripath merged commit 6463483 into go-gitea:master Mar 14, 2021
@zeripath zeripath deleted the no-diff-on-error-during-lfs-test branch March 14, 2021 15:54
@go-gitea go-gitea locked and limited conversation to collaborators May 13, 2021
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. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. type/testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants