Issue content should not be updated when closing with comment #2833
Conversation
lafriks
force-pushed
the
fix/issue_comment_close
branch
2 times, most recently
from
5b71fe4 to
5508747
Compare
| @@ -107,7 +107,7 @@ func TestNoLoginViewIssue(t *testing.T) { | |||
| MakeRequest(t, req, http.StatusOK) | |||
| } | |||
|
|
|||
| func testNewIssue(t *testing.T, session *TestSession, user, repo, title string) { | |||
| func testNewIssue(t *testing.T, session *TestSession, user, repo, title, content string) string { | |||
There was a problem hiding this comment.
nit: from the signature, it's not obvious what the function returns. Could we add a brief comment specifying that the created issue's URL is returned?
| return issueURL | ||
| } | ||
|
|
||
| func testIssueAddComment(t *testing.T, session *TestSession, issueURL, content, status string) { |
There was a problem hiding this comment.
nit: it's not obvious what values are valid for status (without looking through other parts of the codebase), and if I see something like testIssueAddComment(t, sess, url, "hello", "close") in a test, it's not obvious that this will close the issue, in addition to making a comment.
Could we instead do:
func testIssueAddComment(t, session, issueURL, content) {
testIssueAndCommentWithStatus(t,session,issueURL,content,"")
}
func testIssueAddClosingComment(t, session, issueURL, content) {
testIssueAndCommentWithStatus(t,session,issueURL,content,"close")
}
// helper function
func testIssueAddCommentWithStatus(t, session, issueURL, content, status) {
...
}
I think this will make it easier for future tests to reuse these helpers, and will make tests more readable.
|
LGTM |
tboerger
added
the
lgtm/need 1
|
But CI failed. |
Codecov Report
@@ Coverage Diff @@
## master #2833 +/- ##
=======================================
Coverage 26.85% 26.85%
=======================================
Files 89 89
Lines 17607 17607
=======================================
Hits 4728 4728
Misses 12193 12193
Partials 686 686
Continue to review full report at Codecov.
|
|
|
|
LGTM |
tboerger
added
lgtm/done
Fixes #2551