From 6298ee4d3a7d50aeb3bdeaf653359978ee9fc2e6 Mon Sep 17 00:00:00 2001 From: BtbN Date: Fri, 31 Oct 2025 16:17:23 +0100 Subject: [PATCH] fix: pull request review comment position (#9914) ## Checklist This PR contains both #9889 and #9912, since it depends on the one, and the other provides a test for it. The exact reasoning behind its logic is described here: https://codeberg.org/forgejo/forgejo/issues/9473#issuecomment-7976186 This PR should return the behaviour back to how it was before a PR to Gitea changed it. Only the resulting Database-Entry will reference the line blamed commit, now also with the correct adjusted line. While the context diff view is pulled from the commit the commenter actually commented on. ### Tests - I added test coverage for Go changes... - [ ] in their respective `*_test.go` for unit tests. - [ ] in the `tests/integration` directory if it involves interactions with a live Forgejo server. - I added test coverage for JavaScript changes... - [ ] in `web_src/js/*.test.js` if it can be unit tested. - [x] in `tests/e2e/*.test.e2e.js` if it requires interactions with a live Forgejo server (see also the [developer guide for JavaScript testing](https://codeberg.org/forgejo/forgejo/src/branch/forgejo/tests/e2e/README.md#end-to-end-tests)). ### 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 - [ ] I do not want this change to show in the release notes. - [x] I want the title to show in the release notes with a link to this pull request. - [ ] I want the content of the `release-notes/.md` to be be used for the release notes instead of the title. Co-authored-by: Gusted Co-authored-by: Mathieu Fenniak Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/9914 Reviewed-by: Mathieu Fenniak Reviewed-by: Earl Warren Co-authored-by: BtbN Co-committed-by: BtbN --- modules/git/repo_blame.go | 34 +- modules/git/repo_blame_test.go | 70 +- services/pull/review.go | 19 +- .../long-diff-test/0-README.md | 1113 ++++++++++++++++ .../long-diff-test/1-README.md | 1115 +++++++++++++++++ .../long-diff-test/2-README.md | 1115 +++++++++++++++++ .../long-diff-test/3-README.md | 1112 ++++++++++++++++ tests/e2e/declare_repos_test.go | 49 + tests/e2e/pr-review.test.e2e.ts | 52 + 9 files changed, 4659 insertions(+), 20 deletions(-) create mode 100644 tests/e2e/declarative-repo/long-diff-test/0-README.md create mode 100644 tests/e2e/declarative-repo/long-diff-test/1-README.md create mode 100644 tests/e2e/declarative-repo/long-diff-test/2-README.md create mode 100644 tests/e2e/declarative-repo/long-diff-test/3-README.md diff --git a/modules/git/repo_blame.go b/modules/git/repo_blame.go index d812354af5..50f4c572d5 100644 --- a/modules/git/repo_blame.go +++ b/modules/git/repo_blame.go @@ -7,6 +7,8 @@ import ( "errors" "fmt" "regexp" + "strconv" + "strings" ) var ( @@ -17,7 +19,7 @@ var ( ) // LineBlame returns the latest commit at the given line -func (repo *Repository) LineBlame(revision, file string, line uint64) (*Commit, error) { +func (repo *Repository) LineBlame(revision, file string, line uint64) (*Commit, uint64, error) { res, _, gitErr := NewCommand(repo.Ctx, "blame"). AddOptionFormat("-L %d,%d", line, line). AddOptionValues("-p", revision). @@ -26,24 +28,40 @@ func (repo *Repository) LineBlame(revision, file string, line uint64) (*Commit, stdErr := gitErr.Stderr() if stdErr == fmt.Sprintf("fatal: no such path %s in %s\n", file, revision) { - return nil, ErrBlameFileDoesNotExist + return nil, 0, ErrBlameFileDoesNotExist } if notEnoughLinesRe.MatchString(stdErr) { - return nil, ErrBlameFileNotEnoughLines + return nil, 0, ErrBlameFileNotEnoughLines } - return nil, gitErr + return nil, 0, gitErr } objectFormat, err := repo.GetObjectFormat() if err != nil { - return nil, err + return nil, 0, err } objectIDLen := objectFormat.FullLength() - if len(res) < objectIDLen { - return nil, fmt.Errorf("output of blame is invalid, cannot contain commit ID: %s", res) + + if len(res) < objectIDLen+1 { + return nil, 0, fmt.Errorf("output of blame is invalid, cannot contain commit ID: %s", res) } - return repo.GetCommit(res[:objectIDLen]) + commit, err := repo.GetCommit(res[:objectIDLen]) + if err != nil { + return nil, 0, fmt.Errorf("GetCommit: %w", err) + } + + endIdxOriginalLineNo := strings.IndexRune(res[objectIDLen+1:], ' ') + if endIdxOriginalLineNo == -1 { + return nil, 0, fmt.Errorf("output of blame is invalid, cannot contain original line number: %s", res) + } + + originalLineNo, err := strconv.ParseUint(res[objectIDLen+1:objectIDLen+1+endIdxOriginalLineNo], 10, 64) + if err != nil { + return nil, 0, fmt.Errorf("strconv.ParseUint: %w", err) + } + + return commit, originalLineNo, nil } diff --git a/modules/git/repo_blame_test.go b/modules/git/repo_blame_test.go index 126b95386d..4ddd5d9c3f 100644 --- a/modules/git/repo_blame_test.go +++ b/modules/git/repo_blame_test.go @@ -4,6 +4,9 @@ package git import ( + "bytes" + "os" + "path" "path/filepath" "testing" @@ -17,17 +20,20 @@ func TestLineBlame(t *testing.T) { require.NoError(t, err) defer repo.Close() - commit, err := repo.LineBlame("HEAD", "foo/link_short", 1) + commit, lineno, err := repo.LineBlame("HEAD", "foo/link_short", 1) require.NoError(t, err) assert.Equal(t, "37991dec2c8e592043f47155ce4808d4580f9123", commit.ID.String()) + assert.EqualValues(t, 1, lineno) - commit, err = repo.LineBlame("HEAD", "foo/link_short", 512) + commit, lineno, err = repo.LineBlame("HEAD", "foo/link_short", 512) require.ErrorIs(t, err, ErrBlameFileNotEnoughLines) assert.Nil(t, commit) + assert.Zero(t, lineno) - commit, err = repo.LineBlame("HEAD", "non-existent/path", 512) + commit, lineno, err = repo.LineBlame("HEAD", "non-existent/path", 512) require.ErrorIs(t, err, ErrBlameFileDoesNotExist) assert.Nil(t, commit) + assert.Zero(t, lineno) }) t.Run("SHA256", func(t *testing.T) { @@ -37,16 +43,68 @@ func TestLineBlame(t *testing.T) { require.NoError(t, err) defer repo.Close() - commit, err := repo.LineBlame("HEAD", "foo/link_short", 1) + commit, lineno, err := repo.LineBlame("HEAD", "foo/link_short", 1) require.NoError(t, err) assert.Equal(t, "6aae864a3d1d0d6a5be0cc64028c1e7021e2632b031fd8eb82afc5a283d1c3d1", commit.ID.String()) + assert.EqualValues(t, 1, lineno) - commit, err = repo.LineBlame("HEAD", "foo/link_short", 512) + commit, lineno, err = repo.LineBlame("HEAD", "foo/link_short", 512) require.ErrorIs(t, err, ErrBlameFileNotEnoughLines) assert.Nil(t, commit) + assert.Zero(t, lineno) - commit, err = repo.LineBlame("HEAD", "non-existent/path", 512) + commit, lineno, err = repo.LineBlame("HEAD", "non-existent/path", 512) require.ErrorIs(t, err, ErrBlameFileDoesNotExist) assert.Nil(t, commit) + assert.Zero(t, lineno) + }) + + t.Run("Moved line", func(t *testing.T) { + test := func(t *testing.T, objectFormatName string) { + t.Helper() + tmpDir := t.TempDir() + + require.NoError(t, InitRepository(t.Context(), tmpDir, false, objectFormatName)) + + gitRepo, err := OpenRepository(t.Context(), tmpDir) + require.NoError(t, err) + defer gitRepo.Close() + + require.NoError(t, os.WriteFile(path.Join(tmpDir, "ANSWER"), []byte("abba\n"), 0o666)) + require.NoError(t, AddChanges(tmpDir, true)) + require.NoError(t, CommitChanges(tmpDir, CommitChangesOptions{Message: "Favourite singer of everyone who follows a automata course"})) + + firstCommit, err := gitRepo.GetRefCommitID("HEAD") + require.NoError(t, err) + + require.NoError(t, os.WriteFile(path.Join(tmpDir, "ANSWER"), append(bytes.Repeat([]byte("baba\n"), 9), []byte("abba\n")...), 0o666)) + require.NoError(t, AddChanges(tmpDir, true)) + require.NoError(t, CommitChanges(tmpDir, CommitChangesOptions{Message: "Now there's several of them"})) + + secondCommit, err := gitRepo.GetRefCommitID("HEAD") + require.NoError(t, err) + + commit, lineno, err := gitRepo.LineBlame("HEAD", "ANSWER", 10) + require.NoError(t, err) + assert.Equal(t, firstCommit, commit.ID.String()) + assert.EqualValues(t, 1, lineno) + + 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) + } + } + + t.Run("SHA1", func(t *testing.T) { + test(t, Sha1ObjectFormat.Name()) + }) + + t.Run("SHA256", func(t *testing.T) { + skipIfSHA256NotSupported(t) + + test(t, Sha256ObjectFormat.Name()) + }) }) } diff --git a/services/pull/review.go b/services/pull/review.go index 8efe7d6bcb..760235325d 100644 --- a/services/pull/review.go +++ b/services/pull/review.go @@ -44,7 +44,7 @@ func (err ErrDismissRequestOnClosedPR) Unwrap() error { // If the line got changed the comment is going to be invalidated. func checkInvalidation(ctx context.Context, c *issues_model.Comment, repo *git.Repository, branch string) error { // FIXME differentiate between previous and proposed line - commit, err := repo.LineBlame(branch, c.TreePath, c.UnsignedLine()) + commit, _, err := repo.LineBlame(branch, c.TreePath, c.UnsignedLine()) if err != nil && (errors.Is(err, git.ErrBlameFileDoesNotExist) || errors.Is(err, git.ErrBlameFileNotEnoughLines)) { c.Invalidated = true return issues_model.UpdateCommentInvalidate(ctx, c) @@ -178,7 +178,8 @@ func CreateCodeComment(ctx context.Context, doer *user_model.User, gitRepo *git. // CreateCodeCommentKnownReviewID creates a plain code comment at the specified line / path func CreateCodeCommentKnownReviewID(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, issue *issues_model.Issue, content, treePath string, line, reviewID int64, attachments []string) (*issues_model.Comment, error) { - var commitID, patch string + var commitID, blamedCommitID, patch string + blamedLine := line if err := issue.LoadPullRequest(ctx); err != nil { return nil, fmt.Errorf("LoadPullRequest: %w", err) } @@ -226,12 +227,15 @@ func CreateCodeCommentKnownReviewID(ctx context.Context, doer *user_model.User, // FIXME validate treePath // Get latest commit referencing the commented line // No need for get commit for base branch changes - commit, err := gitRepo.LineBlame(head, treePath, uint64(line)) + commit, lineres, err := gitRepo.LineBlame(head, treePath, uint64(line)) if err == nil { - commitID = commit.ID.String() + blamedCommitID = commit.ID.String() + blamedLine = int64(lineres) } else if !errors.Is(err, git.ErrBlameFileDoesNotExist) && !errors.Is(err, git.ErrBlameFileNotEnoughLines) { return nil, fmt.Errorf("LineBlame[%s, %s, %s, %d]: %w", pr.GetGitRefName(), gitRepo.Path, treePath, line, err) } + } else { + blamedCommitID = commitID } } @@ -243,6 +247,9 @@ func CreateCodeCommentKnownReviewID(ctx context.Context, doer *user_model.User, return nil, fmt.Errorf("GetRefCommitID[%s]: %w", head, err) } } + if len(blamedCommitID) == 0 { + blamedCommitID = commitID + } reader, writer := io.Pipe() defer func() { _ = reader.Close() @@ -268,9 +275,9 @@ func CreateCodeCommentKnownReviewID(ctx context.Context, doer *user_model.User, Repo: repo, Issue: issue, Content: content, - LineNum: line, + LineNum: blamedLine, TreePath: treePath, - CommitSHA: commitID, + CommitSHA: blamedCommitID, ReviewID: reviewID, Patch: patch, Invalidated: invalidated, diff --git a/tests/e2e/declarative-repo/long-diff-test/0-README.md b/tests/e2e/declarative-repo/long-diff-test/0-README.md new file mode 100644 index 0000000000..c4ce5aaed9 --- /dev/null +++ b/tests/e2e/declarative-repo/long-diff-test/0-README.md @@ -0,0 +1,1113 @@ +# Introduction + +Hello! + +This is the introduction to this repository. + + +# Purpose and Goals + +This is the purpose and goal of this repository. + + +# Known Issues + +TODO: Fill out known issues. + + +# Contributing + +TODO: Fill out contributing. + +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More diff --git a/tests/e2e/declarative-repo/long-diff-test/1-README.md b/tests/e2e/declarative-repo/long-diff-test/1-README.md new file mode 100644 index 0000000000..d3094546f3 --- /dev/null +++ b/tests/e2e/declarative-repo/long-diff-test/1-README.md @@ -0,0 +1,1115 @@ +# Introduction + +Hello! + +This is the introduction to this repository. + +This line was introduced in commit 1. + + +# Purpose and Goals + +This is the purpose and goal of this repository. + + +# Known Issues + +TODO: Fill out known issues. + + +# Contributing + +TODO: Fill out contributing. + +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More diff --git a/tests/e2e/declarative-repo/long-diff-test/2-README.md b/tests/e2e/declarative-repo/long-diff-test/2-README.md new file mode 100644 index 0000000000..d97c8532a1 --- /dev/null +++ b/tests/e2e/declarative-repo/long-diff-test/2-README.md @@ -0,0 +1,1115 @@ +# Introduction + +Hello! + +This is the introduction to this repository. + +This line was introduced in commit 1. + + +# Purpose and Goals + +This is the purpose and goal of this repository. + + +# Known Issues + +TODO: Fill out known issues. + + +# Contributing + +TODO: Fill out contributing. + +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More This line was changed in commit 2 +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More diff --git a/tests/e2e/declarative-repo/long-diff-test/3-README.md b/tests/e2e/declarative-repo/long-diff-test/3-README.md new file mode 100644 index 0000000000..9e6dec4b1a --- /dev/null +++ b/tests/e2e/declarative-repo/long-diff-test/3-README.md @@ -0,0 +1,1112 @@ +# Introduction + +Hello! + +This is the introduction to this repository. + +This line was introduced in commit 1. + + +# Purpose and Goals + +This is the purpose and goal of this repository. + + +# Known Issues + +TODO: Fill out known issues. + + +# Contributing + +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More This line was changed in commit 2 +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More Soon a line will be deleted in commit 3 +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More +More Soon a line will be deleted in commit 3 +More +More +More diff --git a/tests/e2e/declare_repos_test.go b/tests/e2e/declare_repos_test.go index 8afad81df3..23bc023b70 100644 --- a/tests/e2e/declare_repos_test.go +++ b/tests/e2e/declare_repos_test.go @@ -5,6 +5,7 @@ package e2e import ( "fmt" + "os" "strings" "testing" "time" @@ -130,6 +131,19 @@ body: postIssue(repo, user, 450, "right issue", "an issue containing word right") postIssue(repo, user, 150, "left issue", "an issue containing word left") }), + newRepo(t, 2, "long-diff-test", nil, []FileChanges{{ + Filename: "test-README.md", + Versions: []string{ + readStringFile(t, "tests/e2e/declarative-repo/long-diff-test/0-README.md"), + }, + }}, func(user *user_model.User, repo *repo_model.Repository) { + commit1Sha := addCommitToBranch(t, user, repo, "main", "test-branch", "test-README.md", "", + readStringFile(t, "tests/e2e/declarative-repo/long-diff-test/1-README.md")) + commit2Sha := addCommitToBranch(t, user, repo, "test-branch", "test-branch", "test-README.md", commit1Sha, + readStringFile(t, "tests/e2e/declarative-repo/long-diff-test/2-README.md")) + addCommitToBranch(t, user, repo, "test-branch", "test-branch", "test-README.md", commit2Sha, + readStringFile(t, "tests/e2e/declarative-repo/long-diff-test/3-README.md")) + }), // add your repo declarations here } @@ -140,6 +154,12 @@ body: } } +func readStringFile(t *testing.T, fn string) string { + c, err := os.ReadFile(fn) + require.NoError(t, err) + return string(c) +} + func newRepo(t *testing.T, userID int64, repoName string, initOpts *tests.DeclarativeRepoOptions, fileChanges []FileChanges, setup SetupRepo) func() { user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: userID}) @@ -206,3 +226,32 @@ func newRepo(t *testing.T, userID int64, repoName string, initOpts *tests.Declar return cleanupFunc } + +func addCommitToBranch(t *testing.T, user *user_model.User, repo *repo_model.Repository, oldBranch, newBranch, filename, lastSha, content string) string { + resp, err := files_service.ChangeRepoFiles(git.DefaultContext, repo, user, &files_service.ChangeRepoFilesOptions{ + Files: []*files_service.ChangeRepoFile{{ + Operation: "update", + TreePath: filename, + ContentReader: strings.NewReader(content), + }}, + Message: "add commit to branch", + OldBranch: oldBranch, + NewBranch: newBranch, + Author: &files_service.IdentityOptions{ + Name: user.Name, + Email: user.Email, + }, + Committer: &files_service.IdentityOptions{ + Name: user.Name, + Email: user.Email, + }, + Dates: &files_service.CommitDateOptions{ + Author: time.Now(), + Committer: time.Now(), + }, + LastCommitID: lastSha, + }) + require.NoError(t, err) + assert.NotEmpty(t, resp) + return resp.Commit.SHA +} diff --git a/tests/e2e/pr-review.test.e2e.ts b/tests/e2e/pr-review.test.e2e.ts index 22f3971202..24b636eb79 100644 --- a/tests/e2e/pr-review.test.e2e.ts +++ b/tests/e2e/pr-review.test.e2e.ts @@ -133,3 +133,55 @@ test('PR: Test mentions values', async ({page}) => { await page.locator("ul.suggestions li[data-value='@user1']").click(); await expect(page.locator('.review-box-panel textarea#_combo_markdown_editor_0')).toHaveValue('@user1 '); }); + +test('multi-commit commenting', async ({page, request}) => { + const response = await page.goto('/user2/long-diff-test'); + expect(response?.status()).toBe(200); + + try { + await page.getByText('2 branches').click(); // navigate to branch list + await page.getByText('New pull request').click(); // load compare view for the branch + await page.locator('.show-form-container').getByText('New pull request').click(); // actually open the PR form + await page.locator('.primary.button').getByText('Create pull request').click(); // submit PR creation + + // Test situation: adding a comment on a line that was created in the *second* commit, doing it from the "Files changed" view. + await page.getByText('Files changed').click(); + await page.getByText('More This line was changed in commit 2') + .locator('..') + .locator('button.add-code-comment') + .click(); + await page.getByPlaceholder('Leave a comment').fill('Comment on line changed in commit 2'); + await page.getByText('Add single comment').click(); + + // Test assertion: when viewing the comment from the 'Conversation' page, it's diff should look correct: + await page.getByText('Conversation').click(); + await expect(page.locator('.pull.menu .item.active')).toContainText('Conversation'); // ensure we navigated back to Conversation page + await expect(page.locator('.text.comment-content .render-content.markup')).toHaveText('Comment on line changed in commit 2'); + await expect(page.locator('.diff-file-box .code-diff')).toContainText('More This line was changed in commit 2'); + + // Test assertion: when viewing the comment from the second commit, it should be placed correctly in the UI: + await page.getByRole('link', {name: 'Commits'}).click(); + await page.getByText('add commit to branch').nth(1).click(); + // FIXME: The intent of this test is to make sure that the comment box appears in the "right spot", which is *below* + // the line of code that it was commented on. This check uses the elements bounding boxes which... is pretty ugly + // and could be done better. Probably would be better to find the line of code that the box is rendered on, + // instead. + const codeLine = await page.getByText('More This line was changed in commit 2').boundingBox(); + const commentBox = await page.locator('.render-content.markup').getByText('Comment on line changed in commit 2').boundingBox(); + expect(commentBox.y).toBeGreaterThan(codeLine.y); + } finally { + // Delete any PRs on the test repo so that this test can be rerun. + const issuesResp = await request.get(`/api/v1/repos/user2/long-diff-test/issues`); + expect(issuesResp.ok()).toBeTruthy(); + const issues = await issuesResp.json(); + for (const issue of issues) { + const delResp = await request.delete(`/api/v1/repos/user2/long-diff-test/issues/${issue.number}`, { + headers: { + 'Content-Type': 'application/json', + 'Authorization': `Basic ${btoa(`user1:password`)}`, + }, + }); + expect(delResp.ok()).toBeTruthy(); + } + } +});