-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Fix /api/repo/search integration tests #2550
Fix /api/repo/search integration tests #2550
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2550 +/- ##
=======================================
Coverage 26.99% 26.99%
=======================================
Files 85 85
Lines 17097 17097
=======================================
Hits 4615 4615
Misses 11807 11807
Partials 675 675 Continue to review full report at Codecov.
|
3ee61ec
to
5a3a6de
Compare
ecce5cc
to
c3beba9
Compare
DecodeJSON(t, resp, &body) | ||
for _, repo := range body.data { | ||
assert.True(t, strings.Contains(repo.Name, keyword)) | ||
assert.NotEmpty(t, body.Data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
require.NotEmpty
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bkcsoft I don't want to stop tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Morlinest Why? One failing test is enough to let whole integration tests fail. It will speed up testing a lot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@daviian If you are thinking about that like this, we should use require.*
everywhere :). I want to see more than 1 step ahead. In my opinion, it's like parsing a form and showing just first error to user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Morlinest in this particular test every following assertion will evaluate to false so there is no reason to check them anyway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@daviian Following assertions are inside range loop.
LGTM |
LGTM |
Make LG-TM work |
Make L-G-T-M work again |
Make L-GTM work :) |
Extracted fix from #2371. Fixes (and improves) #2446.