Skip to content

Conversation

@sapk
Copy link
Member

@sapk sapk commented Dec 11, 2017

Add some bench for testing so I propose to add them.

@lafriks lafriks added this to the 1.4.0 milestone Dec 11, 2017
@codecov-io
Copy link

codecov-io commented Dec 11, 2017

Codecov Report

Merging #3161 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #3161   +/-   ##
======================================
  Coverage    34.9%   34.9%           
======================================
  Files         277     277           
  Lines       40108   40108           
======================================
  Hits        14000   14000           
  Misses      24059   24059           
  Partials     2049    2049
Impacted Files Coverage Δ
models/unit_tests.go 78.02% <100%> (ø) ⬆️
modules/indexer/indexer.go 70% <0%> (-7.5%) ⬇️
models/repo.go 43.38% <0%> (ø) ⬆️
modules/process/manager.go 81.15% <0%> (+4.34%) ⬆️

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 a995ad9...be7deb6. Read the comment docs.

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 11, 2017
@lafriks
Copy link
Member

lafriks commented Dec 12, 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 12, 2017
Copy link
Member

Choose a reason for hiding this comment

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

Should these prints be removed (also line 113)?

Copy link
Member

Choose a reason for hiding this comment

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

I think this can be simplified. Just make samples a list of repo IDs, and load the owner and repo in the main loop. We can get rid of the anonymous struct and this first loop.

for _, repoID := range repoIDs {
	repo := models.AssertExistsAndLoadBean(b, &models.Repository{ID: repoID}).(*models.Repository)
	owner := models.AssertExistsAndLoadBean(b, &models.User{ID: repo.OwnerID}).(*models.User)
	... // run test
}

Copy link
Member

Choose a reason for hiding this comment

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

branchName could begin with a slash, in which case it would not be a valid branch name

Copy link
Member

Choose a reason for hiding this comment

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

Would "WebViewCommit" be a better name? The :owner/:repo/commit/:sha endpoint doesn't necessarily have anything to do with branches.

@ethantkoenig
Copy link
Member

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 24, 2017
@lafriks lafriks merged commit cc7b8e3 into go-gitea:master Dec 24, 2017
@sapk sapk deleted the improve-bench branch January 15, 2018 23:21
@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/testing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants