ossf/scorecard
1
1
Fork 0
mirror of https://github.com/ossf/scorecard.git synced 2024-10-05 13:17:08 +03:00
Code Issues Actions 7 Packages Projects Releases Wiki Activity

🐛 Code-Review: change phabricator regex to allow URLs (#4086)

Browse Source 
The old regex used \w which only allowed [0-9A-Za-z_], however most
projects use full URLs with phabricator (e.g.
https://reviews.foo.org/D###). This led to errors parsing the revisions,
where "https" was seen as the revision, leading to an underreporting of
code review practices.

The new regex focuses on the D#### part and uses it as the revision.

Signed-off-by: Spencer Schrock <sschrock@google.com>
This commit is contained in:
Spencer Schrock 2024-05-07 09:51:39 -07:00 committed by GitHub
parent 81d239f19c
commit 250690511d
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 66 additions and 30 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
checks/code_review_test.go
View File

@ -206,7 +206,7 @@ func TestCodereview(t *testing.T) {
Committer: clients.User{
Login: "bob",
},
Message: "Title\nReviewed By: alice\nDifferential Revision: PHAB234",
Message: "Title\nReviewed By: alice\nDifferential Revision: D234",
},
},
expected: scut.TestReturn{
@ -236,7 +236,7 @@ func TestCodereview(t *testing.T) {
Committer: clients.User{
Login: "bob",
},
Message: "Title\nDifferential Revision: PHAB234",
Message: "Title\nDifferential Revision: D234",
},
},
expected: scut.TestReturn{

2
checks/raw/code_review.go
View File

@ -25,7 +25,7 @@ import (
)
var (
rePhabricatorRevID = regexp.MustCompile(`Differential Revision:\s*(\w+)`)
rePhabricatorRevID = regexp.MustCompile(`Differential Revision:[^\r\n]*(D\d+)`)
rePiperRevID = regexp.MustCompile(`PiperOrigin-RevId:\s*(\d{3,})`)
)

90
checks/raw/code_review_test.go
View File

@ -67,29 +67,41 @@ func Test_getChangesets(t *testing.T) {
}
phabricatorCommitA = clients.Commit{
Message: "\nDifferential Revision: 123\nReviewed By: user-123",
Message: "\nDifferential Revision: D123\nReviewed By: user-123",
SHA: "abc",
}
phabricatorCommitAUnsquashed = clients.Commit{
Message: "\nDifferential Revision: 123\nReviewed By: user-123",
Message: "\nDifferential Revision: D123\nReviewed By: user-123",
SHA: "adef",
}
phabricatorCommitAUnsquashed2 = clients.Commit{
Message: "\nDifferential Revision: 123\nReviewed By: user-456",
Message: "\nDifferential Revision: D123\nReviewed By: user-456",
SHA: "afab",
}
phabricatorCommitB = clients.Commit{
Message: "\nDifferential Revision: 158\nReviewed By: user-123",
Message: "\nDifferential Revision: D158\nReviewed By: user-123",
SHA: "def",
}
phabricatorCommitC = clients.Commit{
Message: "\nDifferential Revision: 2000\nReviewed By: user-456",
Message: "\nDifferential Revision: D2000\nReviewed By: user-456",
SHA: "fab",
}
phabricatorCommitD = clients.Commit{
Message: "\nDifferential Revision: 2\nReviewed By: user-456",
Message: "\nDifferential Revision: D2\nReviewed By: user-456",
SHA: "d",
}
phabricatorCommitE = clients.Commit{
Message: "\nDifferential Revision: https://reviews.foo.org/D123 \nReviewed By: user-123",
SHA: "e",
}
phabricatorCommitF = clients.Commit{
Message: "\nDifferential Revision: https://foo.bar.example.com/D456 \nReviewed By: user-123",
SHA: "f",
}
phabricatorCommitG = clients.Commit{
Message: "\nDifferential Revision: https://reviews.bar.org/D78910 \nReviewed By: user-123",
SHA: "g",
}
gerritCommitB = clients.Commit{
Message: "first change\nReviewed-on: server.url \nReviewed-by:user-123",
@ -256,17 +268,17 @@ func Test_getChangesets(t *testing.T) {
commits: []clients.Commit{phabricatorCommitA, phabricatorCommitB, phabricatorCommitC},
expected: []checker.Changeset{
{
RevisionID: "123",
RevisionID: "D123",
ReviewPlatform: checker.ReviewPlatformPhabricator,
Commits: []clients.Commit{phabricatorCommitA},
},
{
RevisionID: "158",
RevisionID: "D158",
ReviewPlatform: checker.ReviewPlatformPhabricator,
Commits: []clients.Commit{phabricatorCommitB},
},
{
RevisionID: "2000",
RevisionID: "D2000",
ReviewPlatform: checker.ReviewPlatformPhabricator,
Commits: []clients.Commit{phabricatorCommitC},
},
@ -277,7 +289,7 @@ func Test_getChangesets(t *testing.T) {
commits: []clients.Commit{phabricatorCommitA, phabricatorCommitAUnsquashed, phabricatorCommitAUnsquashed2},
expected: []checker.Changeset{
{
RevisionID: "123",
RevisionID: "D123",
ReviewPlatform: checker.ReviewPlatformPhabricator,
Commits: []clients.Commit{phabricatorCommitA, phabricatorCommitAUnsquashed, phabricatorCommitAUnsquashed2},
},
@ -305,12 +317,12 @@ func Test_getChangesets(t *testing.T) {
expected: []checker.Changeset{
{
ReviewPlatform: checker.ReviewPlatformPhabricator,
RevisionID: "123",
RevisionID: "D123",
Commits: []clients.Commit{phabricatorCommitA},
},
{
ReviewPlatform: checker.ReviewPlatformPhabricator,
RevisionID: "2",
RevisionID: "D2",
Commits: []clients.Commit{phabricatorCommitD},
},
{
@ -351,23 +363,47 @@ func Test_getChangesets(t *testing.T) {
},
},
},
{
name: "phabricator with URL for differential revision",
commits: []clients.Commit{phabricatorCommitE, phabricatorCommitF, phabricatorCommitG},
expected: []checker.Changeset{
{
ReviewPlatform: checker.ReviewPlatformPhabricator,
RevisionID: "D123",
Commits: []clients.Commit{phabricatorCommitE},
},
{
ReviewPlatform: checker.ReviewPlatformPhabricator,
RevisionID: "D456",
Commits: []clients.Commit{phabricatorCommitF},
},
{
ReviewPlatform: checker.ReviewPlatformPhabricator,
RevisionID: "D78910",
Commits: []clients.Commit{phabricatorCommitG},
},
},
},
}
for _, tt := range tests {
t.Logf("test: %s", tt.name)
changesets := getChangesets(tt.commits)
if !cmp.Equal(tt.expected, changesets,
cmpopts.SortSlices(func(x, y checker.Changeset) bool {
if x.RevisionID == y.RevisionID {
return x.ReviewPlatform < y.ReviewPlatform
}
return x.RevisionID < y.RevisionID
}),
cmpopts.SortSlices(func(x, y clients.Commit) bool {
return x.SHA < y.SHA
})) {
t.Log(cmp.Diff(tt.expected, changesets))
t.Fail()
}
tt := tt
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
changesets := getChangesets(tt.commits)
if !cmp.Equal(tt.expected, changesets,
cmpopts.SortSlices(func(x, y checker.Changeset) bool {
if x.RevisionID == y.RevisionID {
return x.ReviewPlatform < y.ReviewPlatform
}
return x.RevisionID < y.RevisionID
}),
cmpopts.SortSlices(func(x, y clients.Commit) bool {
return x.SHA < y.SHA
})) {
t.Log(cmp.Diff(tt.expected, changesets))
t.Fail()
}
})
}
}