mirror of
https://codeberg.org/forgejo/forgejo.git
synced 2026-05-12 22:10:25 +00:00
fix: relocate PR review comments using git blame --reverse, improving comment placement (#12015)
When a review comment is placed on a PR in Forgejo, Forgejo performs a `git blame` to identify which commit originated the line, and records that commit and line number in the comment's database record. Later when the review is viewed, Forgejo currently makes no effort to place that comment in the correct *current* location, which may vary -- for example, if a PR had two commits and the comment was made on a line in the first commit, but the second commit changes line numbers in that file, the comment will appear in the incorrect location. This PR adds the usage of `git blame --reverse` to calculate the correct location to display the comment in the current view (whether reviewing the PR commit-by-commit, or "Files changed"). It certainly does not fix all problems with comment placement (see comments). Another major addition in this PR is a test harness for making relatively complex PRs and reviewing the diffs on the per-commit view and PR-diff views. ## Checklist The [contributor guide](https://forgejo.org/docs/next/contributor/) contains information that will be helpful to first time contributors. All work and communication must conform to Forgejo's [AI Agreement](https://codeberg.org/forgejo/governance/src/branch/main/AIAgreement.md). There also are a few [conditions for merging Pull Requests in Forgejo repositories](https://codeberg.org/forgejo/governance/src/branch/main/PullRequestsAgreement.md). You are also welcome to join the [Forgejo development chatroom](https://matrix.to/#/#forgejo-development:matrix.org). ### Tests for Go changes - I added test coverage for Go changes... - [x] in their respective `*_test.go` for unit tests. - [x] in the `tests/integration` directory if it involves interactions with a live Forgejo server. - I ran... - [ ] `make pr-go` before pushing ### Documentation - [ ] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change. - [x] I did not document these changes and I do not expect someone else to do it. ### Release notes - [x] This change will be noticed by a Forgejo user or admin (feature, bug fix, performance, etc.). I suggest to include a release note for this change. - [ ] This change is not visible to a Forgejo user or admin (refactor, dependency upgrade, etc.). I think there is no need to add a release note for this change. <!--start release-notes-assistant--> ## Release notes <!--URL:https://codeberg.org/forgejo/forgejo--> - Bug fixes - [PR](https://codeberg.org/forgejo/forgejo/pulls/12015): <!--number 12015 --><!--line 0 --><!--description cmVsb2NhdGUgUFIgcmV2aWV3IGNvbW1lbnRzIHVzaW5nIGBnaXQgYmxhbWUgLS1yZXZlcnNlYCwgaW1wcm92aW5nIGNvbW1lbnQgcGxhY2VtZW50-->relocate PR review comments using `git blame --reverse`, improving comment placement<!--description--> <!--end release-notes-assistant--> Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/12015 Reviewed-by: Andreas Ahlenstorf <aahlenst@noreply.codeberg.org> Co-authored-by: Mathieu Fenniak <mathieu@fenniak.net> Co-committed-by: Mathieu Fenniak <mathieu@fenniak.net>
This commit is contained in:
Mathieu Fenniak
Mathieu Fenniak
parent
92b95414e8
commit
9fe0cbee02
9 changed files with 936 additions and 19 deletions
|
|
@ -19,7 +19,9 @@ import (
|
|||
project_model "forgejo.org/models/project"
|
||||
repo_model "forgejo.org/models/repo"
|
||||
user_model "forgejo.org/models/user"
|
||||
"forgejo.org/modules/cache"
|
||||
"forgejo.org/modules/container"
|
||||
"forgejo.org/modules/git"
|
||||
"forgejo.org/modules/gitrepo"
|
||||
"forgejo.org/modules/json"
|
||||
"forgejo.org/modules/log"
|
||||
|
|
@ -321,6 +323,8 @@ type Comment struct {
|
|||
CommitsNum int64 `xorm:"-"`
|
||||
IsForcePush bool `xorm:"-"`
|
||||
|
||||
reverseLineBlame *git.ReverseLineBlame `xorm:"-"`
|
||||
|
||||
// If you add new fields that might be used to store abusive content (mainly string fields),
|
||||
// please also add them in the CommentData struct and the corresponding constructor.
|
||||
}
|
||||
|
|
@ -744,6 +748,55 @@ func (c *Comment) UnsignedLine() uint64 {
|
|||
return uint64(c.Line)
|
||||
}
|
||||
|
||||
func (c *Comment) ResolveCurrentLine(ctx context.Context, repo *repo_model.Repository, currentHead string) (*git.ReverseLineBlame, error) {
|
||||
if c.reverseLineBlame != nil {
|
||||
return c.reverseLineBlame, nil
|
||||
}
|
||||
|
||||
// When a PR is viewed, the requirement to perform `git blame --reverse...` on every comment is a bit of a
|
||||
// performance risk. To minimize this risk, cache the results relative to the requested head, so it only needs to be
|
||||
// recalculated when head changes (or on cache eviction).
|
||||
//
|
||||
// Some performance testing was done which showed that a hot cache is much faster than the blame reverse
|
||||
// operation -- 500-1000x runtime difference:
|
||||
//
|
||||
// - cache miss (Forgejo repo) took 7,690,574 ns
|
||||
// - cache miss (~1000 commit repo) took 1,671,223 ns
|
||||
// - cache hit (in-memory adapter) took 3,710 ns
|
||||
// - cache hit (redis adapter) took 77,311 ns
|
||||
resolveJSON, err := cache.GetString(fmt.Sprintf("comment.Resolve;ID=%d;HEAD=%s", c.ID, currentHead), func() (string, error) {
|
||||
gitRepo, closer, err := gitrepo.RepositoryFromContextOrOpen(ctx, repo)
|
||||
if err != nil {
|
||||
return "", fmt.Errorf("failed to open repo: %w", err)
|
||||
}
|
||||
defer closer.Close()
|
||||
|
||||
reverseBlame, err := gitRepo.ReverseLineBlame(c.CommitSHA, c.TreePath, c.UnsignedLine(), currentHead)
|
||||
if err != nil {
|
||||
return "", fmt.Errorf("failed to perform `git blame --reverse` to resolve current line for comment (id=%d): %w", c.ID, err)
|
||||
}
|
||||
|
||||
data, err := json.Marshal(reverseBlame)
|
||||
if err != nil {
|
||||
return "", err
|
||||
}
|
||||
|
||||
return string(data), nil
|
||||
})
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
var reverseBlame *git.ReverseLineBlame
|
||||
err = json.Unmarshal([]byte(resolveJSON), &reverseBlame)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
c.reverseLineBlame = reverseBlame
|
||||
return c.reverseLineBlame, nil
|
||||
}
|
||||
|
||||
// CodeCommentLink returns the url to a comment in code
|
||||
func (c *Comment) CodeCommentLink(ctx context.Context) string {
|
||||
err := c.LoadIssue(ctx)
|
||||
|
|
|
|||
|
|
@ -7,7 +7,10 @@ import (
|
|||
"context"
|
||||
|
||||
"forgejo.org/models/db"
|
||||
repo_model "forgejo.org/models/repo"
|
||||
user_model "forgejo.org/models/user"
|
||||
"forgejo.org/modules/git"
|
||||
"forgejo.org/modules/log"
|
||||
"forgejo.org/modules/markup"
|
||||
"forgejo.org/modules/markup/markdown"
|
||||
|
||||
|
|
@ -23,33 +26,62 @@ type CodeConversationsAtLine map[int64][]CodeConversation
|
|||
// CodeConversationsAtLineAndTreePath contains the conversations for a given TreePath and line
|
||||
type CodeConversationsAtLineAndTreePath map[string]CodeConversationsAtLine
|
||||
|
||||
func newCodeConversationsAtLineAndTreePath(comments []*Comment) CodeConversationsAtLineAndTreePath {
|
||||
func newCodeConversationsAtLineAndTreePath(ctx context.Context, comments []*Comment, repo *repo_model.Repository, headCommitID string) (CodeConversationsAtLineAndTreePath, error) {
|
||||
tree := make(CodeConversationsAtLineAndTreePath)
|
||||
for _, comment := range comments {
|
||||
tree.insertComment(comment)
|
||||
blame, err := comment.ResolveCurrentLine(ctx, repo, headCommitID)
|
||||
if err != nil {
|
||||
// ResolveCurrentLine can fail in at least one known situation -- where a comment is left on a line in a
|
||||
// file that is being deleted. The blame would be for the commit that deleted the file, and a reverse git
|
||||
// blame won't work because the file is missing in the target sha.
|
||||
log.Warn("ResolveCurrentLine failed: %s", err.Error())
|
||||
// handle gracefully -- insertComment will use the original values which may be usable
|
||||
blame = nil
|
||||
} else if blame.CommitID != headCommitID {
|
||||
// Commit was made on a line that can't be reverse-blamed to the currently viewing head. This can happen
|
||||
// because:
|
||||
// - line of code was removed between the commit it was tagged on, and the head commit
|
||||
// - force push on the repo caused there to be no git relationship between blame.CommitID->headCommitID
|
||||
// We won't insert this comment into the comment tree because we don't know where to place it; it may appear
|
||||
// when the user views a different commit in the PR, and it will always appear on the "Conversations" tab.
|
||||
continue
|
||||
}
|
||||
return tree
|
||||
tree.insertComment(comment, blame)
|
||||
}
|
||||
return tree, nil
|
||||
}
|
||||
|
||||
func (tree CodeConversationsAtLineAndTreePath) insertComment(comment *Comment) {
|
||||
func (tree CodeConversationsAtLineAndTreePath) insertComment(comment *Comment, blame *git.ReverseLineBlame) {
|
||||
treePath := comment.TreePath
|
||||
line := comment.Line
|
||||
if blame != nil {
|
||||
treePath = blame.FilePath
|
||||
line = int64(blame.LineNumber)
|
||||
if comment.Line < 0 {
|
||||
line *= -1
|
||||
}
|
||||
}
|
||||
|
||||
// attempt to append comment to existing conversations (i.e. list of comments belonging to the same review)
|
||||
for i, conversation := range tree[comment.TreePath][comment.Line] {
|
||||
for i, conversation := range tree[treePath][line] {
|
||||
if conversation[0].ReviewID == comment.ReviewID {
|
||||
tree[comment.TreePath][comment.Line][i] = append(conversation, comment)
|
||||
tree[treePath][line][i] = append(conversation, comment)
|
||||
return
|
||||
}
|
||||
}
|
||||
|
||||
// no previous conversation was found at this line, create it
|
||||
if tree[comment.TreePath] == nil {
|
||||
tree[comment.TreePath] = make(map[int64][]CodeConversation)
|
||||
if tree[treePath] == nil {
|
||||
tree[treePath] = make(map[int64][]CodeConversation)
|
||||
}
|
||||
|
||||
tree[comment.TreePath][comment.Line] = append(tree[comment.TreePath][comment.Line], CodeConversation{comment})
|
||||
tree[treePath][line] = append(tree[treePath][line], CodeConversation{comment})
|
||||
}
|
||||
|
||||
// FetchCodeConversations will return a 2d-map: ["Path"]["Line"] = List of CodeConversation (one per review) for this line
|
||||
func FetchCodeConversations(ctx context.Context, issue *Issue, doer *user_model.User, showOutdatedComments bool) (CodeConversationsAtLineAndTreePath, error) {
|
||||
// FetchCodeConversations will return a 2d-map: ["Path"]["Line"] = List of CodeConversation (one per review) for this
|
||||
// line. headCommitID will be used to reverse-blame the comment into the correct path & line for the current context
|
||||
// that is being viewed.
|
||||
func FetchCodeConversations(ctx context.Context, issue *Issue, doer *user_model.User, showOutdatedComments bool, headCommitID string) (CodeConversationsAtLineAndTreePath, error) {
|
||||
opts := FindCommentsOptions{
|
||||
Type: CommentTypeCode,
|
||||
IssueID: issue.ID,
|
||||
|
|
@ -59,7 +91,7 @@ func FetchCodeConversations(ctx context.Context, issue *Issue, doer *user_model.
|
|||
return nil, err
|
||||
}
|
||||
|
||||
return newCodeConversationsAtLineAndTreePath(comments), nil
|
||||
return newCodeConversationsAtLineAndTreePath(ctx, comments, issue.Repo, headCommitID)
|
||||
}
|
||||
|
||||
// CodeComments represents comments on code by using this structure: FILENAME -> LINE (+ == proposed; - == previous) -> COMMENTS
|
||||
|
|
|
|||
|
|
@ -63,7 +63,7 @@ func TestFetchCodeConversations(t *testing.T) {
|
|||
unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ID: 4}),
|
||||
user, true))
|
||||
|
||||
res, err := issues_model.FetchCodeConversations(db.DefaultContext, issue, user, false)
|
||||
res, err := issues_model.FetchCodeConversations(db.DefaultContext, issue, user, false, "")
|
||||
require.NoError(t, err)
|
||||
require.Contains(t, res, "README.md")
|
||||
require.Contains(t, res["README.md"], int64(4))
|
||||
|
|
@ -77,7 +77,7 @@ func TestFetchCodeConversations(t *testing.T) {
|
|||
assert.NotNil(t, r.User)
|
||||
|
||||
user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
|
||||
res, err = issues_model.FetchCodeConversations(db.DefaultContext, issue, user2, false)
|
||||
res, err = issues_model.FetchCodeConversations(db.DefaultContext, issue, user2, false, "")
|
||||
require.NoError(t, err)
|
||||
assert.Len(t, res, 1)
|
||||
}
|
||||
|
|
|
|||
|
|
@ -65,3 +65,97 @@ func (repo *Repository) LineBlame(revision, file string, line uint64) (*Commit,
|
|||
|
||||
return commit, originalLineNo, nil
|
||||
}
|
||||
|
||||
type ReverseLineBlame struct {
|
||||
CommitID string
|
||||
LineNumber uint64
|
||||
FilePath string
|
||||
}
|
||||
|
||||
// Reverses the effect of [LineBlame]. If a file was modified at originalLine number in originalRevision,
|
||||
// ReverseLineBlame will identify the last commit up-to-and-including currentRevision where that line exists, including
|
||||
// its new path and line number. If the returned commit is not the same as currentRevision, then it indicates that
|
||||
// content can no longer be located in currentRevision, and the returned commit is the last commit that had it.
|
||||
func (repo *Repository) ReverseLineBlame(originalRevision, file string, originalLine uint64, currentRevision string) (*ReverseLineBlame, error) {
|
||||
if originalRevision == currentRevision {
|
||||
// Would cause an error to run the reverse, "fatal: More than one commit to dig up from, (N) and (N)"
|
||||
return &ReverseLineBlame{
|
||||
CommitID: originalRevision,
|
||||
LineNumber: originalLine,
|
||||
FilePath: file,
|
||||
}, nil
|
||||
}
|
||||
|
||||
res, _, gitErr := NewCommand(repo.Ctx, "blame").
|
||||
AddOptionValues("--reverse").
|
||||
AddDynamicArguments(fmt.Sprintf("%s..%s", originalRevision, currentRevision)).
|
||||
AddOptionFormat("-L %d,%d", originalLine, originalLine).
|
||||
AddOptionValues("-p").
|
||||
AddDashesAndList(file).RunStdString(&RunOpts{Dir: repo.Path})
|
||||
if gitErr != nil {
|
||||
return nil, gitErr
|
||||
}
|
||||
|
||||
// Example output:
|
||||
//
|
||||
// 74be0e8aa338d1374ab7ca0a25a4f594955a69c2 16 9 1
|
||||
// author FirstName LastName
|
||||
// author-mail <author@example.org>
|
||||
// author-time 1775492007
|
||||
// author-tz -0600
|
||||
// committer FirstName LastName
|
||||
// committer-mail <author@example.org>
|
||||
// committer-time 1775492007
|
||||
// committer-tz -0600
|
||||
// summary restore file-in-base to orig, now not present in diff
|
||||
// filename README.md
|
||||
//
|
||||
// Header (https://git-scm.com/docs/git-blame#_the_porcelain_format):
|
||||
// - 40-byte SHA-1 of the commit the line is attributed to;
|
||||
// - the line number of the line in the original file; [note: opposite in reverse]
|
||||
// - the line number of the line in the final file; [note: opposite in reverse]
|
||||
// - on a line that starts a group of lines from a different commit than the previous one, the number of lines in
|
||||
// this group. On subsequent lines this field is absent.
|
||||
|
||||
lines := strings.Split(res, "\n")
|
||||
|
||||
header := lines[0]
|
||||
headerValues := strings.Split(header, " ")
|
||||
if len(headerValues) < 2 {
|
||||
return nil, fmt.Errorf("failed to parse blame --reverse header: %q", header)
|
||||
}
|
||||
|
||||
objectFormat, err := repo.GetObjectFormat()
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
objectIDLen := objectFormat.FullLength()
|
||||
objectID := headerValues[0]
|
||||
if len(objectID) != objectIDLen {
|
||||
return nil, fmt.Errorf("output of blame is invalid, cannot contain commit ID: %s", objectID)
|
||||
}
|
||||
commit, err := repo.GetCommit(objectID)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("GetCommit: %w", err)
|
||||
}
|
||||
|
||||
currentLineStr := headerValues[1]
|
||||
currentLineNo, err := strconv.ParseUint(currentLineStr, 10, 64)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("strconv.ParseUint: %w", err)
|
||||
}
|
||||
|
||||
var filename string
|
||||
for _, otherLine := range lines {
|
||||
if strings.HasPrefix(otherLine, "filename ") {
|
||||
filename = otherLine[len("filename "):]
|
||||
break
|
||||
}
|
||||
}
|
||||
|
||||
return &ReverseLineBlame{
|
||||
CommitID: commit.ID.String(),
|
||||
LineNumber: currentLineNo,
|
||||
FilePath: filename,
|
||||
}, nil
|
||||
}
|
||||
|
|
|
|||
|
|
@ -89,11 +89,23 @@ func TestLineBlame(t *testing.T) {
|
|||
assert.Equal(t, firstCommit, commit.ID.String())
|
||||
assert.EqualValues(t, 1, lineno)
|
||||
|
||||
rev, err := gitRepo.ReverseLineBlame(commit.ID.String(), "ANSWER", lineno, secondCommit)
|
||||
require.NoError(t, err)
|
||||
assert.Equal(t, secondCommit, rev.CommitID)
|
||||
assert.Equal(t, "ANSWER", rev.FilePath)
|
||||
assert.EqualValues(t, 10, rev.LineNumber)
|
||||
|
||||
for i := range uint64(9) {
|
||||
commit, lineno, err = gitRepo.LineBlame("HEAD", "ANSWER", i+1)
|
||||
require.NoError(t, err)
|
||||
assert.Equal(t, secondCommit, commit.ID.String())
|
||||
assert.Equal(t, i+1, lineno)
|
||||
|
||||
rev, err := gitRepo.ReverseLineBlame(commit.ID.String(), "ANSWER", lineno, secondCommit)
|
||||
require.NoError(t, err)
|
||||
assert.Equal(t, secondCommit, rev.CommitID)
|
||||
assert.Equal(t, "ANSWER", rev.FilePath)
|
||||
assert.Equal(t, i+1, rev.LineNumber)
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -108,3 +120,66 @@ func TestLineBlame(t *testing.T) {
|
|||
})
|
||||
})
|
||||
}
|
||||
|
||||
func TestReverseLineBlame(t *testing.T) {
|
||||
t.Run("single commit", func(t *testing.T) {
|
||||
tmpDir := t.TempDir()
|
||||
require.NoError(t, InitRepository(t.Context(), tmpDir, false, Sha1ObjectFormat.Name()))
|
||||
|
||||
gitRepo, err := OpenRepository(t.Context(), tmpDir)
|
||||
require.NoError(t, err)
|
||||
defer gitRepo.Close()
|
||||
|
||||
require.NoError(t, os.WriteFile(path.Join(tmpDir, "file1.md"), []byte("abba\n"), 0o666))
|
||||
require.NoError(t, AddChanges(tmpDir, true))
|
||||
require.NoError(t, CommitChanges(tmpDir, CommitChangesOptions{Message: "abba spelt backwards"}))
|
||||
|
||||
commit, err := gitRepo.GetRefCommitID("HEAD")
|
||||
require.NoError(t, err)
|
||||
|
||||
blameCommit, lineno, err := gitRepo.LineBlame("HEAD", "file1.md", 1)
|
||||
require.NoError(t, err)
|
||||
assert.Equal(t, commit, blameCommit.ID.String())
|
||||
assert.EqualValues(t, 1, lineno)
|
||||
|
||||
rev, err := gitRepo.ReverseLineBlame(commit, "file1.md", lineno, commit)
|
||||
require.NoError(t, err)
|
||||
assert.Equal(t, commit, rev.CommitID)
|
||||
assert.Equal(t, "file1.md", rev.FilePath)
|
||||
assert.EqualValues(t, 1, rev.LineNumber)
|
||||
})
|
||||
|
||||
t.Run("move file", func(t *testing.T) {
|
||||
tmpDir := t.TempDir()
|
||||
require.NoError(t, InitRepository(t.Context(), tmpDir, false, Sha1ObjectFormat.Name()))
|
||||
|
||||
gitRepo, err := OpenRepository(t.Context(), tmpDir)
|
||||
require.NoError(t, err)
|
||||
defer gitRepo.Close()
|
||||
|
||||
require.NoError(t, os.WriteFile(path.Join(tmpDir, "file1.md"), []byte("abba\n"), 0o666))
|
||||
require.NoError(t, AddChanges(tmpDir, true))
|
||||
require.NoError(t, CommitChanges(tmpDir, CommitChangesOptions{Message: "abba spelt backwards"}))
|
||||
|
||||
firstCommit, err := gitRepo.GetRefCommitID("HEAD")
|
||||
require.NoError(t, err)
|
||||
|
||||
require.NoError(t, os.Rename(path.Join(tmpDir, "file1.md"), path.Join(tmpDir, "file2.md")))
|
||||
require.NoError(t, AddChanges(tmpDir, true))
|
||||
require.NoError(t, CommitChanges(tmpDir, CommitChangesOptions{Message: "move file"}))
|
||||
|
||||
secondCommit, err := gitRepo.GetRefCommitID("HEAD")
|
||||
require.NoError(t, err)
|
||||
|
||||
blameCommit, lineno, err := gitRepo.LineBlame("HEAD", "file2.md", 1)
|
||||
require.NoError(t, err)
|
||||
assert.Equal(t, firstCommit, blameCommit.ID.String())
|
||||
assert.EqualValues(t, 1, lineno)
|
||||
|
||||
rev, err := gitRepo.ReverseLineBlame(firstCommit, "file1.md", lineno, secondCommit)
|
||||
require.NoError(t, err)
|
||||
assert.Equal(t, secondCommit, rev.CommitID)
|
||||
assert.Equal(t, "file2.md", rev.FilePath)
|
||||
assert.EqualValues(t, 1, rev.LineNumber)
|
||||
})
|
||||
}
|
||||
|
|
|
|||
|
|
@ -1099,7 +1099,7 @@ func viewPullFiles(ctx *context.Context, specifiedStartCommit, specifiedEndCommi
|
|||
"numberOfViewedFiles": diff.NumViewedFiles,
|
||||
}
|
||||
|
||||
if err = diff.LoadComments(ctx, issue, ctx.Doer, ctx.Data["ShowOutdatedComments"].(bool)); err != nil {
|
||||
if err = diff.LoadComments(ctx, issue, ctx.Doer, ctx.Data["ShowOutdatedComments"].(bool), endCommitID); err != nil {
|
||||
ctx.ServerError("LoadComments", err)
|
||||
return
|
||||
}
|
||||
|
|
|
|||
|
|
@ -487,8 +487,8 @@ type Diff struct {
|
|||
}
|
||||
|
||||
// LoadComments loads comments into each line
|
||||
func (diff *Diff) LoadComments(ctx context.Context, issue *issues_model.Issue, currentUser *user_model.User, showOutdatedComments bool) error {
|
||||
allConversations, err := issues_model.FetchCodeConversations(ctx, issue, currentUser, showOutdatedComments)
|
||||
func (diff *Diff) LoadComments(ctx context.Context, issue *issues_model.Issue, currentUser *user_model.User, showOutdatedComments bool, headCommitID string) error {
|
||||
allConversations, err := issues_model.FetchCodeConversations(ctx, issue, currentUser, showOutdatedComments, headCommitID)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
|
|
|
|||
|
|
@ -600,7 +600,7 @@ func TestDiff_LoadCommentsNoOutdated(t *testing.T) {
|
|||
issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 2})
|
||||
user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
|
||||
diff := setupDefaultDiff()
|
||||
require.NoError(t, diff.LoadComments(db.DefaultContext, issue, user, false))
|
||||
require.NoError(t, diff.LoadComments(db.DefaultContext, issue, user, false, ""))
|
||||
assert.Len(t, diff.Files[0].Sections[0].Lines[0].Conversations, 2)
|
||||
}
|
||||
|
||||
|
|
@ -610,7 +610,7 @@ func TestDiff_LoadCommentsWithOutdated(t *testing.T) {
|
|||
issue := unittest.AssertExistsAndLoadBean(t, &issues_model.Issue{ID: 2})
|
||||
user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
|
||||
diff := setupDefaultDiff()
|
||||
require.NoError(t, diff.LoadComments(db.DefaultContext, issue, user, true))
|
||||
require.NoError(t, diff.LoadComments(db.DefaultContext, issue, user, true, ""))
|
||||
assert.Len(t, diff.Files[0].Sections[0].Lines[0].Conversations, 2)
|
||||
assert.Len(t, diff.Files[0].Sections[0].Lines[0].Conversations[0], 2)
|
||||
assert.Len(t, diff.Files[0].Sections[0].Lines[0].Conversations[1], 1)
|
||||
|
|
|
|||
|
|
@ -6,6 +6,7 @@ package integration
|
|||
|
||||
import (
|
||||
"bytes"
|
||||
"encoding/base64"
|
||||
"fmt"
|
||||
"io"
|
||||
"net/http"
|
||||
|
|
@ -13,11 +14,13 @@ import (
|
|||
"net/url"
|
||||
"os"
|
||||
"path"
|
||||
"slices"
|
||||
"strconv"
|
||||
"strings"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
auth_model "forgejo.org/models/auth"
|
||||
"forgejo.org/models/db"
|
||||
issues_model "forgejo.org/models/issues"
|
||||
repo_model "forgejo.org/models/repo"
|
||||
|
|
@ -26,7 +29,9 @@ import (
|
|||
user_model "forgejo.org/models/user"
|
||||
"forgejo.org/modules/git"
|
||||
"forgejo.org/modules/gitrepo"
|
||||
"forgejo.org/modules/optional"
|
||||
repo_module "forgejo.org/modules/repository"
|
||||
api "forgejo.org/modules/structs"
|
||||
"forgejo.org/modules/test"
|
||||
issue_service "forgejo.org/services/issue"
|
||||
"forgejo.org/services/mailer"
|
||||
|
|
@ -35,8 +40,10 @@ import (
|
|||
"forgejo.org/tests"
|
||||
|
||||
"github.com/PuerkitoBio/goquery"
|
||||
"github.com/google/uuid"
|
||||
"github.com/stretchr/testify/assert"
|
||||
"github.com/stretchr/testify/require"
|
||||
"golang.org/x/net/html"
|
||||
)
|
||||
|
||||
func TestPullView_ReviewerMissed(t *testing.T) {
|
||||
|
|
@ -1048,3 +1055,659 @@ func TestPullRequestStaleReview(t *testing.T) {
|
|||
})
|
||||
})
|
||||
}
|
||||
|
||||
func TestPullRequestCommentPlacement(t *testing.T) {
|
||||
onApplicationRun(t, func(t *testing.T, u *url.URL) {
|
||||
t.Run("comment directly on change in PR", func(t *testing.T) {
|
||||
defer tests.PrintCurrentTest(t)()
|
||||
tester := newPullRequestCommentPlacementTester(t)
|
||||
|
||||
commitSHA := tester.changeFile("file1.md",
|
||||
strings.Replace(tester.fileContent, "Line 50\n", "Line 50--modified\n", 1))
|
||||
tester.createPR()
|
||||
|
||||
comment := tester.commentFromFilesChanged("file1.md", 50)
|
||||
assert.Equal(t, `diff --git a/file1.md b/file1.md
|
||||
--- a/file1.md
|
||||
+++ b/file1.md
|
||||
@@ -48,3 +48,3 @@
|
||||
Line 48
|
||||
Line 49
|
||||
-Line 50
|
||||
+Line 50--modified`, comment.PatchQuoted)
|
||||
assert.Equal(t, "proposed", comment.DiffSide())
|
||||
assert.EqualValues(t, 50, comment.Line)
|
||||
assert.Equal(t, commitSHA, comment.CommitSHA)
|
||||
|
||||
diff := []diffTableRow{
|
||||
{rowType: RowHasCode, code: "Line 49"},
|
||||
{rowType: RowDelCode, code: "Line 50"},
|
||||
{rowType: RowAddCode, code: "Line 50--modified"},
|
||||
{rowType: RowComment, commentID: comment.ID},
|
||||
{rowType: RowHasCode, code: "Line 51"},
|
||||
}
|
||||
tester.assertFilesChangedDiff(diff)
|
||||
tester.assertCommitDiff(commitSHA, diff)
|
||||
})
|
||||
|
||||
t.Run("comment lands on blame from commit within PR", func(t *testing.T) {
|
||||
defer tests.PrintCurrentTest(t)()
|
||||
tester := newPullRequestCommentPlacementTester(t)
|
||||
|
||||
// Modify an earlier part of the file in one commit, and a later part of the file in a second commit.
|
||||
content := tester.fileContent
|
||||
content = strings.Replace(content, "Line 25\n", "Line 25--modified\n", 1)
|
||||
commit1 := tester.changeFile("file1.md", content)
|
||||
content = strings.Replace(content, "Line 75\n", "Line 75--modified\n", 1)
|
||||
commit2 := tester.changeFile("file1.md", content)
|
||||
tester.createPR()
|
||||
|
||||
// Comment on the earlier change, from the "Files changed" view; this should "git blame" and be asociated
|
||||
// with the first commit where that change was made, therefore appearing on the commit-specific diff later:
|
||||
comment := tester.commentFromFilesChanged("file1.md", 25)
|
||||
assert.Equal(t, `diff --git a/file1.md b/file1.md
|
||||
--- a/file1.md
|
||||
+++ b/file1.md
|
||||
@@ -23,3 +23,3 @@
|
||||
Line 23
|
||||
Line 24
|
||||
-Line 25
|
||||
+Line 25--modified`, comment.PatchQuoted)
|
||||
assert.Equal(t, "proposed", comment.DiffSide())
|
||||
assert.EqualValues(t, 25, comment.Line)
|
||||
assert.Equal(t, commit1, comment.CommitSHA)
|
||||
|
||||
diff25 := []diffTableRow{
|
||||
{rowType: RowHasCode, code: "Line 24"},
|
||||
{rowType: RowDelCode, code: "Line 25"},
|
||||
{rowType: RowAddCode, code: "Line 25--modified"},
|
||||
{rowType: RowComment, commentID: comment.ID},
|
||||
{rowType: RowHasCode, code: "Line 26"},
|
||||
}
|
||||
tester.assertFilesChangedDiff(diff25)
|
||||
tester.assertCommitDiff(commit1, diff25)
|
||||
|
||||
diff75 := []diffTableRow{
|
||||
{rowType: RowHasCode, code: "Line 74"},
|
||||
{rowType: RowDelCode, code: "Line 75"},
|
||||
{rowType: RowAddCode, code: "Line 75--modified"},
|
||||
{rowType: RowHasCode, code: "Line 76"},
|
||||
}
|
||||
tester.assertFilesChangedDiff(diff75)
|
||||
tester.assertCommitDiff(commit2, diff75)
|
||||
})
|
||||
|
||||
t.Run("comment lands on blame commit from before PR", func(t *testing.T) {
|
||||
defer tests.PrintCurrentTest(t)()
|
||||
tester := newPullRequestCommentPlacementTester(t)
|
||||
|
||||
// Modify line 50...
|
||||
commitSHA := tester.changeFile("file1.md",
|
||||
strings.Replace(tester.fileContent, "Line 50\n", "Line 50--modified\n", 1))
|
||||
tester.createPR()
|
||||
|
||||
// But while viewing line 50's diff, place a comment on line 49. This will "git blame" to a commit outside
|
||||
// of this PR, but that's fine...
|
||||
comment := tester.commentFromFilesChanged("file1.md", 49)
|
||||
assert.Equal(t, `diff --git a/file1.md b/file1.md
|
||||
--- a/file1.md
|
||||
+++ b/file1.md
|
||||
@@ -47,7 +47,7 @@ Line 46
|
||||
Line 47
|
||||
Line 48
|
||||
Line 49`, comment.PatchQuoted)
|
||||
assert.Equal(t, "proposed", comment.DiffSide())
|
||||
assert.EqualValues(t, 49, comment.Line)
|
||||
assert.NotEqual(t, commitSHA, comment.CommitSHA)
|
||||
|
||||
diff := []diffTableRow{
|
||||
{rowType: RowHasCode, code: "Line 48"},
|
||||
{rowType: RowHasCode, code: "Line 49"},
|
||||
{rowType: RowComment, commentID: comment.ID},
|
||||
{rowType: RowDelCode, code: "Line 50"},
|
||||
{rowType: RowAddCode, code: "Line 50--modified"},
|
||||
{rowType: RowHasCode, code: "Line 51"},
|
||||
}
|
||||
tester.assertFilesChangedDiff(diff)
|
||||
tester.assertCommitDiff(commitSHA, diff)
|
||||
})
|
||||
|
||||
t.Run("comment on line moves due to a following commit", func(t *testing.T) {
|
||||
defer tests.PrintCurrentTest(t)()
|
||||
tester := newPullRequestCommentPlacementTester(t)
|
||||
|
||||
// Modify line 50
|
||||
content := tester.fileContent
|
||||
content = strings.Replace(content, "Line 50\n", "Line 50--modified\n", 1)
|
||||
commit1 := tester.changeFile("file1.md", content)
|
||||
tester.createPR()
|
||||
|
||||
// Place a comment on "Line 50--modified"
|
||||
comment := tester.commentFromFilesChanged("file1.md", 50)
|
||||
assert.Equal(t, `diff --git a/file1.md b/file1.md
|
||||
--- a/file1.md
|
||||
+++ b/file1.md
|
||||
@@ -48,3 +48,3 @@
|
||||
Line 48
|
||||
Line 49
|
||||
-Line 50
|
||||
+Line 50--modified`, comment.PatchQuoted)
|
||||
assert.Equal(t, "proposed", comment.DiffSide())
|
||||
assert.EqualValues(t, 50, comment.Line)
|
||||
assert.Equal(t, commit1, comment.CommitSHA)
|
||||
|
||||
// Add a second commit to the PR which removes "Line 1" - "Line 10".
|
||||
content = strings.Replace(content, "Line 1\nLine 2\nLine 3\nLine 4\nLine 5\nLine 6\nLine 7\nLine 8\nLine 9\nLine 10\n", "", 1)
|
||||
commit2 := tester.changeFile("file1.md", content)
|
||||
|
||||
diff2 := []diffTableRow{
|
||||
{rowType: RowDelCode, code: "Line 9"},
|
||||
{rowType: RowDelCode, code: "Line 10"},
|
||||
{rowType: RowHasCode, code: "Line 11"},
|
||||
}
|
||||
tester.assertFilesChangedDiff(diff2, "checking commit2 contents in full PR diff")
|
||||
tester.assertCommitDiff(commit2, diff2, "checking commit2 contents in single-commit diff")
|
||||
|
||||
diff1 := []diffTableRow{
|
||||
{rowType: RowHasCode, code: "Line 49"},
|
||||
{rowType: RowDelCode, code: "Line 50"},
|
||||
{rowType: RowAddCode, code: "Line 50--modified"},
|
||||
{rowType: RowComment, commentID: comment.ID},
|
||||
{rowType: RowHasCode, code: "Line 51"},
|
||||
}
|
||||
tester.assertFilesChangedDiff(diff1, "checking commit1 contents in full PR diff")
|
||||
tester.assertCommitDiff(commit1, diff1, "checking commit1 contents in single-commit diff")
|
||||
})
|
||||
|
||||
t.Run("comment on line moves due to a following commit, following commit is rewritten and force-push'd", func(t *testing.T) {
|
||||
defer tests.PrintCurrentTest(t)()
|
||||
tester := newPullRequestCommentPlacementTester(t)
|
||||
|
||||
// Modify line 50
|
||||
content := tester.fileContent
|
||||
content = strings.Replace(content, "Line 50\n", "Line 50--modified\n", 1)
|
||||
commit1 := tester.changeFile("file1.md", content)
|
||||
tester.createPR()
|
||||
|
||||
// Place a comment on "Line 50--modified"
|
||||
comment := tester.commentFromFilesChanged("file1.md", 50)
|
||||
assert.Equal(t, `diff --git a/file1.md b/file1.md
|
||||
--- a/file1.md
|
||||
+++ b/file1.md
|
||||
@@ -48,3 +48,3 @@
|
||||
Line 48
|
||||
Line 49
|
||||
-Line 50
|
||||
+Line 50--modified`, comment.PatchQuoted)
|
||||
assert.Equal(t, "proposed", comment.DiffSide())
|
||||
assert.EqualValues(t, 50, comment.Line)
|
||||
assert.Equal(t, commit1, comment.CommitSHA)
|
||||
|
||||
// Add a second commit to the PR which removes "Line 1" - "Line 10".
|
||||
content = strings.Replace(content, "Line 1\nLine 2\nLine 3\nLine 4\nLine 5\nLine 6\nLine 7\nLine 8\nLine 9\nLine 10\n", "", 1)
|
||||
tester.changeFile("file1.md", content)
|
||||
|
||||
// Now amend commit2v1 with an additional change, causing a force push of the branch
|
||||
tester.withBranchCheckout(func(repoPath string) {
|
||||
content = strings.Replace(content, "Line 11\n", "", 1) // Remove Line 11 as well
|
||||
require.NoError(t, os.WriteFile(path.Join(repoPath, "file1.md"), []byte(content), 0o644))
|
||||
require.NoError(t, git.NewCommand(t.Context(), "commit", "-a", "--amend", "--no-edit").Run(&git.RunOpts{Dir: repoPath}))
|
||||
require.NoError(t, git.NewCommand(t.Context(), "push", "--force").Run(&git.RunOpts{Dir: repoPath}))
|
||||
})
|
||||
|
||||
diff2 := []diffTableRow{
|
||||
{rowType: RowDelCode, code: "Line 10"},
|
||||
{rowType: RowDelCode, code: "Line 11"},
|
||||
{rowType: RowHasCode, code: "Line 12"},
|
||||
}
|
||||
tester.assertFilesChangedDiff(diff2, "checking commit2 (force push) contents in full PR diff")
|
||||
|
||||
diff1 := []diffTableRow{
|
||||
{rowType: RowHasCode, code: "Line 49"},
|
||||
{rowType: RowDelCode, code: "Line 50"},
|
||||
{rowType: RowAddCode, code: "Line 50--modified"},
|
||||
{rowType: RowComment, commentID: comment.ID},
|
||||
{rowType: RowHasCode, code: "Line 51"},
|
||||
}
|
||||
tester.assertFilesChangedDiff(diff1, "checking commit1 contents in full PR diff")
|
||||
tester.assertCommitDiff(commit1, diff1, "checking commit1 contents in single-commit diff")
|
||||
})
|
||||
|
||||
t.Run("comment on line commit is rewritten and force-push'd", func(t *testing.T) {
|
||||
defer tests.PrintCurrentTest(t)()
|
||||
tester := newPullRequestCommentPlacementTester(t)
|
||||
|
||||
// Modify line 50
|
||||
content := tester.fileContent
|
||||
content = strings.Replace(content, "Line 50\n", "Line 50--modified\n", 1)
|
||||
commit1 := tester.changeFile("file1.md", content)
|
||||
tester.createPR()
|
||||
|
||||
// Place a comment on "Line 50--modified"
|
||||
comment := tester.commentFromFilesChanged("file1.md", 50)
|
||||
assert.Equal(t, `diff --git a/file1.md b/file1.md
|
||||
--- a/file1.md
|
||||
+++ b/file1.md
|
||||
@@ -48,3 +48,3 @@
|
||||
Line 48
|
||||
Line 49
|
||||
-Line 50
|
||||
+Line 50--modified`, comment.PatchQuoted)
|
||||
assert.Equal(t, "proposed", comment.DiffSide())
|
||||
assert.EqualValues(t, 50, comment.Line)
|
||||
assert.Equal(t, commit1, comment.CommitSHA)
|
||||
|
||||
// Add a second commit to the PR which removes "Line 1" - "Line 10".
|
||||
content = strings.Replace(content, "Line 1\nLine 2\nLine 3\nLine 4\nLine 5\nLine 6\nLine 7\nLine 8\nLine 9\nLine 10\n", "", 1)
|
||||
commit2 := tester.changeFile("file1.md", content)
|
||||
|
||||
// Now, reorganize these commits, so that it's main->commit2->commit1 on the branch, rather than
|
||||
// main->commit1->commit2. Then force push the branch.
|
||||
tester.withBranchCheckout(func(repoPath string) {
|
||||
// move commit2 onto main, off commit1
|
||||
require.NoError(t,
|
||||
git.NewCommand(t.Context(), "rebase").
|
||||
AddArguments("--onto").AddDynamicArguments(tester.initialSHA).
|
||||
AddDynamicArguments(commit1).
|
||||
AddDynamicArguments(commit2).
|
||||
Run(&git.RunOpts{Dir: repoPath}))
|
||||
// move commit1 onto HEAD, off main
|
||||
require.NoError(t,
|
||||
git.NewCommand(t.Context(), "rebase").
|
||||
AddArguments("--onto").AddDynamicArguments("HEAD").
|
||||
AddDynamicArguments(tester.initialSHA).
|
||||
AddDynamicArguments(commit1).
|
||||
Run(&git.RunOpts{Dir: repoPath}))
|
||||
|
||||
// delete branch for the PR
|
||||
has, branch := tester.branch.Get()
|
||||
require.True(t, has)
|
||||
require.NoError(t,
|
||||
git.NewCommand(t.Context(), "branch").
|
||||
AddArguments("-D").AddDynamicArguments(branch).
|
||||
Run(&git.RunOpts{Dir: repoPath}))
|
||||
// call HEAD as the branch
|
||||
require.NoError(t,
|
||||
git.NewCommand(t.Context(), "branch").
|
||||
AddDynamicArguments(branch).
|
||||
Run(&git.RunOpts{Dir: repoPath}))
|
||||
|
||||
// force push the rebuilt branch
|
||||
require.NoError(t, git.NewCommand(t.Context(), "push", "--force", "origin").AddDynamicArguments(branch).Run(&git.RunOpts{Dir: repoPath}))
|
||||
})
|
||||
|
||||
diff2 := []diffTableRow{
|
||||
{rowType: RowDelCode, code: "Line 10"},
|
||||
{rowType: RowHasCode, code: "Line 11"},
|
||||
}
|
||||
tester.assertFilesChangedDiff(diff2, "checking commit2 (force push) contents in full PR diff")
|
||||
|
||||
diff1 := []diffTableRow{
|
||||
{rowType: RowHasCode, code: "Line 49"},
|
||||
{rowType: RowDelCode, code: "Line 50"},
|
||||
{rowType: RowAddCode, code: "Line 50--modified"},
|
||||
// no comment visible anymore; force push has lost its place at this time
|
||||
{rowType: RowHasCode, code: "Line 51"},
|
||||
}
|
||||
tester.assertFilesChangedDiff(diff1, "checking commit1 contents in full PR diff")
|
||||
})
|
||||
|
||||
t.Run("comment lands on blame with original line number varying from current", func(t *testing.T) {
|
||||
defer tests.PrintCurrentTest(t)()
|
||||
tester := newPullRequestCommentPlacementTester(t)
|
||||
|
||||
// Remove "Line 1" - "Line 10", on the base branch. If you "git blame" Line 50 at that point, it will have
|
||||
// an original line number 50, but actually be appearing at line number index 40, causing wrong outputs.
|
||||
content := tester.fileContent
|
||||
content = strings.Replace(content, "Line 1\nLine 2\nLine 3\nLine 4\nLine 5\nLine 6\nLine 7\nLine 8\nLine 9\nLine 10\n", "", 1)
|
||||
tester.changeFileOnBase("file1.md", content)
|
||||
|
||||
// Now modify "Line 51" in a PR:
|
||||
commitSHA := tester.changeFile("file1.md", strings.Replace(content, "Line 51\n", "Line 51--modified\n", 1))
|
||||
tester.createPR()
|
||||
|
||||
// Place a comment on "Line 50", which would "git blame" to the original commit and line number 50, even
|
||||
// though it's now actually at line number 40.
|
||||
comment := tester.commentFromFilesChanged("file1.md", lineNumber(content, "Line 50"))
|
||||
assert.Equal(t, `diff --git a/file1.md b/file1.md
|
||||
--- a/file1.md
|
||||
+++ b/file1.md
|
||||
@@ -38,7 +38,7 @@ Line 47
|
||||
Line 48
|
||||
Line 49
|
||||
Line 50`, comment.PatchQuoted)
|
||||
assert.Equal(t, "proposed", comment.DiffSide())
|
||||
assert.EqualValues(t, 50, comment.Line)
|
||||
assert.Equal(t, tester.initialSHA, comment.CommitSHA)
|
||||
|
||||
diff := []diffTableRow{
|
||||
{rowType: RowHasCode, code: "Line 49"},
|
||||
{rowType: RowHasCode, code: "Line 50"},
|
||||
{rowType: RowComment, commentID: comment.ID},
|
||||
{rowType: RowDelCode, code: "Line 51"},
|
||||
{rowType: RowAddCode, code: "Line 51--modified"},
|
||||
{rowType: RowHasCode, code: "Line 52"},
|
||||
}
|
||||
tester.assertFilesChangedDiff(diff)
|
||||
tester.assertCommitDiff(commitSHA, diff)
|
||||
})
|
||||
})
|
||||
}
|
||||
|
||||
type PullRequestCommentPlacementTester struct {
|
||||
t *testing.T
|
||||
user *user_model.User
|
||||
session *TestSession
|
||||
apiToken string
|
||||
fileContent string
|
||||
repo *repo_model.Repository
|
||||
initialSHA string
|
||||
branch optional.Option[string]
|
||||
pr *api.PullRequest
|
||||
}
|
||||
|
||||
func newPullRequestCommentPlacementTester(t *testing.T) *PullRequestCommentPlacementTester {
|
||||
user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
|
||||
token := getUserToken(t, "user2", auth_model.AccessTokenScopeWriteRepository, auth_model.AccessTokenScopeWriteIssue)
|
||||
session := loginUser(t, user2.Name)
|
||||
|
||||
var content strings.Builder
|
||||
for i := range 100 {
|
||||
content.WriteString(fmt.Sprintf("Line %d\n", i+1)) // +1 -> make "Line N" appear on the Nth line and avoid off-by-one confusions
|
||||
}
|
||||
|
||||
repo, initialSHA, reset := tests.CreateDeclarativeRepoWithOptions(t, user2, tests.DeclarativeRepoOptions{
|
||||
Files: optional.Some([]*files_service.ChangeRepoFile{
|
||||
{
|
||||
Operation: "create",
|
||||
TreePath: "file1.md",
|
||||
ContentReader: strings.NewReader(content.String()),
|
||||
},
|
||||
{
|
||||
Operation: "create",
|
||||
TreePath: "file2.md",
|
||||
ContentReader: strings.NewReader(content.String()),
|
||||
},
|
||||
}),
|
||||
})
|
||||
t.Cleanup(reset)
|
||||
|
||||
return &PullRequestCommentPlacementTester{
|
||||
t: t,
|
||||
user: user2,
|
||||
session: session,
|
||||
apiToken: token,
|
||||
fileContent: content.String(),
|
||||
repo: repo,
|
||||
initialSHA: initialSHA,
|
||||
}
|
||||
}
|
||||
|
||||
func (tester *PullRequestCommentPlacementTester) changeFileOnBranch(sourceBranch, targetBranch string, targetBranchIsNew bool, filename, newContent string) string {
|
||||
req := NewRequest(tester.t,
|
||||
"GET",
|
||||
fmt.Sprintf("/api/v1/repos/%s/%s/contents/%s?ref=%s", tester.repo.OwnerName, tester.repo.Name, filename, sourceBranch)).
|
||||
AddTokenAuth(tester.apiToken)
|
||||
resp := MakeRequest(tester.t, req, http.StatusOK)
|
||||
var existingFile api.ContentsResponse
|
||||
DecodeJSON(tester.t, resp, &existingFile)
|
||||
|
||||
opts := api.UpdateFileOptions{
|
||||
DeleteFileOptions: api.DeleteFileOptions{
|
||||
SHA: existingFile.SHA,
|
||||
},
|
||||
ContentBase64: base64.StdEncoding.EncodeToString([]byte(newContent)),
|
||||
}
|
||||
if targetBranchIsNew {
|
||||
opts.DeleteFileOptions.FileOptions.NewBranchName = targetBranch
|
||||
} else {
|
||||
opts.DeleteFileOptions.FileOptions.BranchName = targetBranch
|
||||
}
|
||||
req = NewRequestWithJSON(tester.t,
|
||||
"PUT",
|
||||
fmt.Sprintf("/api/v1/repos/%s/%s/contents/%s", tester.repo.OwnerName, tester.repo.Name, filename),
|
||||
opts).AddTokenAuth(tester.apiToken)
|
||||
resp = MakeRequest(tester.t, req, http.StatusOK)
|
||||
var updateFileResponse api.FileResponse
|
||||
DecodeJSON(tester.t, resp, &updateFileResponse)
|
||||
|
||||
return updateFileResponse.Commit.SHA
|
||||
}
|
||||
|
||||
func (tester *PullRequestCommentPlacementTester) changeFileOnBase(filename, newContent string) string {
|
||||
return tester.changeFileOnBranch(tester.repo.DefaultBranch, tester.repo.DefaultBranch, false, filename, newContent)
|
||||
}
|
||||
|
||||
func (tester *PullRequestCommentPlacementTester) changeFile(filename, newContent string) string {
|
||||
var sourceBranch string // where to get the file's last SHA from
|
||||
branchExists, branch := tester.branch.Get()
|
||||
if !branchExists {
|
||||
branch = fmt.Sprintf("branch-%s", uuid.New().String())
|
||||
tester.branch = optional.Some(branch)
|
||||
sourceBranch = tester.repo.DefaultBranch
|
||||
} else {
|
||||
sourceBranch = branch
|
||||
}
|
||||
return tester.changeFileOnBranch(sourceBranch, branch, !branchExists, filename, newContent)
|
||||
}
|
||||
|
||||
func (tester *PullRequestCommentPlacementTester) createPR() {
|
||||
branchExists, branch := tester.branch.Get()
|
||||
require.True(tester.t, branchExists)
|
||||
req := NewRequestWithJSON(tester.t, "POST",
|
||||
fmt.Sprintf("/api/v1/repos/%s/%s/pulls", tester.repo.OwnerName, tester.repo.Name),
|
||||
&api.CreatePullRequestOption{
|
||||
Head: branch,
|
||||
Base: tester.repo.DefaultBranch,
|
||||
Title: fmt.Sprintf("PR from branch %s", tester.branch),
|
||||
}).AddTokenAuth(tester.apiToken)
|
||||
resp := MakeRequest(tester.t, req, http.StatusCreated)
|
||||
var pr api.PullRequest
|
||||
DecodeJSON(tester.t, resp, &pr)
|
||||
tester.pr = &pr
|
||||
}
|
||||
|
||||
func (tester *PullRequestCommentPlacementTester) commentFromFilesChanged(filename string, line int) *issues_model.Comment {
|
||||
commentContent := uuid.New().String()
|
||||
|
||||
req := NewRequest(tester.t, "GET",
|
||||
fmt.Sprintf("/%s/%s/pulls/%d/files/reviews/new_comment", tester.repo.OwnerName, tester.repo.Name, tester.pr.Index))
|
||||
resp := tester.session.MakeRequest(tester.t, req, http.StatusOK)
|
||||
|
||||
doc := NewHTMLParser(tester.t, resp.Body)
|
||||
req = NewRequestWithValues(tester.t, "POST",
|
||||
fmt.Sprintf("/%s/%s/pulls/%d/files/reviews/comments", tester.repo.OwnerName, tester.repo.Name, tester.pr.Index),
|
||||
map[string]string{
|
||||
"origin": doc.GetInputValueByName("origin"),
|
||||
"latest_commit_id": doc.GetInputValueByName("latest_commit_id"),
|
||||
"side": "proposed", // "proposed" (RHS) or "previous" (LHS)
|
||||
"line": strconv.Itoa(line),
|
||||
"path": filename,
|
||||
"diff_start_cid": doc.GetInputValueByName("diff_start_cid"),
|
||||
"diff_end_cid": doc.GetInputValueByName("diff_end_cid"),
|
||||
"diff_base_cid": doc.GetInputValueByName("diff_base_cid"),
|
||||
"content": commentContent,
|
||||
"single_review": "true",
|
||||
})
|
||||
tester.session.MakeRequest(tester.t, req, http.StatusOK)
|
||||
|
||||
comment := unittest.AssertExistsAndLoadBean(tester.t, &issues_model.Comment{Content: commentContent})
|
||||
return comment
|
||||
}
|
||||
|
||||
func (tester *PullRequestCommentPlacementTester) withBranchCheckout(action func(string)) {
|
||||
dstPath := tester.t.TempDir()
|
||||
cloneURL, _ := url.Parse(tester.repo.CloneLink().HTTPS)
|
||||
cloneURL.User = url.UserPassword(tester.user.LoginName, userPassword)
|
||||
require.NoError(tester.t, git.CloneWithArgs(tester.t.Context(), nil, cloneURL.String(), dstPath, git.CloneRepoOptions{}))
|
||||
doGitSetRemoteURL(dstPath, "origin", cloneURL)(tester.t)
|
||||
|
||||
branchExists, branch := tester.branch.Get()
|
||||
require.True(tester.t, branchExists)
|
||||
require.NoError(tester.t, git.NewCommand(tester.t.Context(), "checkout").AddDynamicArguments(branch).Run(&git.RunOpts{Dir: dstPath}))
|
||||
|
||||
action(dstPath)
|
||||
}
|
||||
|
||||
func (tester *PullRequestCommentPlacementTester) assertFilesChangedDiff(rowAssertions []diffTableRow, note ...string) {
|
||||
req := NewRequest(tester.t, "GET",
|
||||
fmt.Sprintf("/%s/%s/pulls/%d/files?show-outdated=true", tester.repo.OwnerName, tester.repo.Name, tester.pr.Index))
|
||||
resp := tester.session.MakeRequest(tester.t, req, http.StatusOK)
|
||||
doc := NewHTMLParser(tester.t, resp.Body)
|
||||
var testNote string
|
||||
if len(note) == 0 {
|
||||
testNote = "contents in single-commit diff"
|
||||
} else {
|
||||
testNote = note[0]
|
||||
}
|
||||
assertDiffTable(tester.t, doc, rowAssertions, testNote)
|
||||
}
|
||||
|
||||
func (tester *PullRequestCommentPlacementTester) assertCommitDiff(commitSHA string, rowAssertions []diffTableRow, note ...string) {
|
||||
req := NewRequest(tester.t, "GET",
|
||||
fmt.Sprintf("/%s/%s/pulls/%d/commits/%s?show-outdated=true", tester.repo.OwnerName, tester.repo.Name, tester.pr.Index, commitSHA))
|
||||
resp := tester.session.MakeRequest(tester.t, req, http.StatusOK)
|
||||
doc := NewHTMLParser(tester.t, resp.Body)
|
||||
var testNote string
|
||||
if len(note) == 0 {
|
||||
testNote = "contents in full PR diff"
|
||||
} else {
|
||||
testNote = note[0]
|
||||
}
|
||||
assertDiffTable(tester.t, doc, rowAssertions, testNote)
|
||||
}
|
||||
|
||||
func lineNumber(content, line string) int {
|
||||
return slices.Index(strings.Split(content, "\n"), line) + 1
|
||||
}
|
||||
|
||||
type diffTableRowType int
|
||||
|
||||
const (
|
||||
RowHasCode diffTableRowType = iota
|
||||
RowAddCode
|
||||
RowDelCode
|
||||
RowComment
|
||||
)
|
||||
|
||||
type diffTableRow struct {
|
||||
rowType diffTableRowType
|
||||
// RowHasCode, RowAddCode, RowDelCode
|
||||
code string
|
||||
// RowComment
|
||||
commentID int64
|
||||
}
|
||||
|
||||
func nodeText(node *html.Node) string {
|
||||
if node.Type == html.TextNode {
|
||||
return node.Data
|
||||
}
|
||||
var builder strings.Builder
|
||||
for child := range node.ChildNodes() {
|
||||
childText := strings.TrimSpace(nodeText(child))
|
||||
builder.WriteString(childText)
|
||||
}
|
||||
return builder.String()
|
||||
}
|
||||
|
||||
func nodeAttr(node *html.Node, key string) string {
|
||||
for _, attr := range node.Attr {
|
||||
if attr.Key == key {
|
||||
return attr.Val
|
||||
}
|
||||
}
|
||||
return ""
|
||||
}
|
||||
|
||||
func checkDiffTableRow(t *testing.T, tableRow *html.Node, rowAssertion diffTableRow) string {
|
||||
switch rowAssertion.rowType {
|
||||
case RowHasCode:
|
||||
text := nodeText(tableRow)
|
||||
if text != rowAssertion.code {
|
||||
return fmt.Sprintf("wanted diff %q, but found diff %q", rowAssertion.code, text)
|
||||
}
|
||||
case RowDelCode:
|
||||
dataLineType := nodeAttr(tableRow, "data-line-type")
|
||||
if dataLineType != "del" {
|
||||
return fmt.Sprintf("wanted delete code in diff, but found data-line-type=%q", dataLineType)
|
||||
}
|
||||
text := nodeText(tableRow)
|
||||
if text != rowAssertion.code {
|
||||
return fmt.Sprintf("wanted delete code with line %q, but found diff %q", rowAssertion.code, text)
|
||||
}
|
||||
case RowAddCode:
|
||||
dataLineType := nodeAttr(tableRow, "data-line-type")
|
||||
if dataLineType != "add" {
|
||||
return fmt.Sprintf("wanted add code in diff, but found data-line-type=%q", dataLineType)
|
||||
}
|
||||
text := nodeText(tableRow)
|
||||
if text != rowAssertion.code {
|
||||
return fmt.Sprintf("wanted add code with line %q, but found diff %q", rowAssertion.code, text)
|
||||
}
|
||||
case RowComment:
|
||||
class := nodeAttr(tableRow, "class")
|
||||
if class != "add-comment" {
|
||||
return fmt.Sprintf("wanted comment in diff, but found class=%q", class)
|
||||
}
|
||||
found := false
|
||||
for desc := range tableRow.Descendants() {
|
||||
descID := nodeAttr(desc, "id")
|
||||
if descID == fmt.Sprintf("code-comments-%d", rowAssertion.commentID) {
|
||||
found = true
|
||||
break
|
||||
}
|
||||
}
|
||||
if !found {
|
||||
return fmt.Sprintf("wanted comment with ID %d, but could not be identified", rowAssertion.commentID)
|
||||
}
|
||||
}
|
||||
return ""
|
||||
}
|
||||
|
||||
func assertDiffTable(t *testing.T, doc *HTMLDoc, rowAssertions []diffTableRow, note string) {
|
||||
require.NotEmpty(t, rowAssertions)
|
||||
|
||||
diffTable := doc.Find("table.chroma")
|
||||
require.Equal(t, 1, diffTable.Length())
|
||||
|
||||
rows := diffTable.Find("tbody > tr[data-line-type]") // [data-line-type] is used to avoid matching tables within comment boxes
|
||||
|
||||
// Find the first row to match rowAssertions[0], and then we'll iterate from there matching each row exactly.
|
||||
tableFirstRowIndex := 0
|
||||
foundFirst := false
|
||||
firstRowMismatches := []string{}
|
||||
for ; tableFirstRowIndex < rows.Length(); tableFirstRowIndex++ {
|
||||
mismatch := checkDiffTableRow(t, rows.Get(tableFirstRowIndex), rowAssertions[0])
|
||||
if mismatch == "" {
|
||||
foundFirst = true
|
||||
break
|
||||
}
|
||||
firstRowMismatches = append(firstRowMismatches, mismatch)
|
||||
}
|
||||
if !foundFirst {
|
||||
// We're going to fail because we couldn't find the first row in rowAssertions -- this can be tricky to debug so
|
||||
// help out by outputting all the rows we looked at that didn't match:
|
||||
t.Log("first row mismatches:")
|
||||
for _, mm := range firstRowMismatches {
|
||||
t.Logf("\t%s", mm)
|
||||
}
|
||||
require.Failf(t, "unable to find first row", "test %s: failed to find first row assertion", note)
|
||||
}
|
||||
|
||||
for idx, assertion := range rowAssertions {
|
||||
if idx == 0 { // skip first row assertion, already checked to find tableFirstRowIndex
|
||||
continue
|
||||
}
|
||||
|
||||
tableIdx := tableFirstRowIndex + idx
|
||||
if tableIdx >= rows.Length() {
|
||||
require.Failf(t, "ran out of table rows", "test %s: row assertion at index %d couldn't be satisfied", note, idx)
|
||||
}
|
||||
|
||||
tableRow := rows.Get(tableIdx)
|
||||
check := checkDiffTableRow(t, tableRow, assertion)
|
||||
if check != "" {
|
||||
assert.Failf(t, check, "test %s: row assertion at index %d couldn't be satisfied", note, idx)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue