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.

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

Fix "index out of range" runtime error in repo_list tests #2376

Merged
merged 1 commit into from
Aug 28, 2017
Merged

Fix "index out of range" runtime error in repo_list tests #2376

merged 1 commit into from
Aug 28, 2017

Conversation

Morlinest
Copy link
Member

Fix runtime errors in specific conditions.

bkcsoft
bkcsoft suggested changes Aug 24, 2017
View reviewed changes
models/repo_list_test.go Outdated
@@ -23,7 +23,9 @@ func TestSearchRepositoryByName(t *testing.T) {

assert.NotNil(t, repos)
assert.NoError(t, err)
assert.Equal(t, "test_repo_12", repos[0].Name)
if len(repos) > 0 {
Copy link
Member

Choose a reason for hiding this comment

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

if assert.NotEmpty(repos) {
  assert.Equal(t, "test_repo_12", repos[0].Name)
}

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right, my first tests :D

models/repo_list_test.go Outdated
@@ -48,7 +50,9 @@ func TestSearchRepositoryByName(t *testing.T) {

assert.NotNil(t, repos)
assert.NoError(t, err)
assert.Equal(t, "test_repo_13", repos[0].Name)
if len(repos) > 0 {
Copy link
Member

Choose a reason for hiding this comment

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

Same as above

@strk
Copy link
Member

strk commented Aug 24, 2017

it's a test, shouldn't you know what to expect (repo being or not being empty) ?
I'd think if a repo is unexpectedly empty there is some other problem (the test will never run?)

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 24, 2017
@Morlinest
Copy link
Member Author

@strk Sure, you should know what to expect. But that doesn't mean, you can write tests that can ends by runtime error. I think event creator of tests will have problems to know what that mean without looking to code. If someone else wants to run tests, he don't know what that runtime error means at all without looking to code. I don't see any reason to write any code that can contains bugs, "it is only test" is not excuse.

I think this:
image

is better than:
image

@Morlinest
Copy link
Member Author

Also, panic: runtime error means end of any other tests = you will be missing tests after that line.

@strk
Copy link
Member

strk commented Aug 24, 2017

Yep, an error about unexpected empty is great. I didn't notice the assert there.
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 Aug 24, 2017
@lafriks lafriks added this to the 1.3.0 milestone Aug 24, 2017
@lafriks
Copy link
Member

lafriks commented Aug 24, 2017

LGTM
@bkcsoft need your approval

@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 Aug 24, 2017
lunny
lunny requested changes Aug 24, 2017
View reviewed changes
models/repo_list_test.go Outdated
@@ -23,7 +23,9 @@ func TestSearchRepositoryByName(t *testing.T) {

assert.NotNil(t, repos)
assert.NoError(t, err)
assert.Equal(t, "test_repo_12", repos[0].Name)
if assert.NotEmpty(t, repos) {
Copy link
Member

@lunny lunny Aug 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

No. You should assert how many repos are there.

Copy link
Member

Choose a reason for hiding this comment

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

We only need to know that there's more than 0 repos. (since we're accessing repos[0])

@Morlinest Morlinest mentioned this pull request Aug 24, 2017
Merged
@lunny
Copy link
Member

lunny commented Aug 24, 2017

But it seems it's no need to do that?

@lunny lunny merged commit 0f2c98a into go-gitea:master Aug 28, 2017
@Morlinest Morlinest deleted the fix/repo-list-test-panic branch August 30, 2017 08:35
@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.
Reviewers

@bkcsoft bkcsoft bkcsoft approved these changes

@lunny lunny lunny approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/testing
Projects
None yet
Milestone
1.3.0
Development

Successfully merging this pull request may close these issues.
6 participants
@Morlinest @strk @lafriks @lunny @bkcsoft @tboerger