Skip to content

Commit 0301d43

Browse files
danwinshipdanwinship
committed
Delete old /test help messages after a successful invocation
1 parent b4f6613 commit 0301d43

File tree

5 files changed

+265
-9
lines changed

5 files changed

+265
-9
lines changed

‎pkg/pjutil/help.go‎

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,10 +30,15 @@ var (
3030
RetestWithTargetRe = regexp.MustCompile(`(?m)^/retest[ \t]+\S+`)
3131
TestWithAnyTargetRe = regexp.MustCompile(`(?m)^/test[ \t]+\S+`)
3232

33+
anyTestRe = regexp.MustCompile(`(?m)^/(?:re)?test\b`)
34+
3335
TestWithoutTargetNote = "The `/test` command needs one or more targets.\n"
3436
RetestWithTargetNote = "The `/retest` command does not accept any targets.\n"
3537
TargetNotFoundNote = "The specified target(s) for `/test` were not found.\n"
3638
ThereAreNoTestAllJobsNote = "No jobs can be run with `/test all`.\n"
39+
40+
availableRequiredTestsNote = "The following commands are available to trigger required jobs:"
41+
availableOptionalTestsNote = "The following commands are available to trigger optional jobs:"
3742
)
3843

3944
func MayNeedHelpComment(body string) bool {
@@ -60,6 +65,20 @@ func ShouldRespondWithHelp(body string, toRunOrSkip int) (bool, string) {
6065
}
6166
}
6267

68+
func ShouldRespondByPruningHelp(body string) bool {
69+
return anyTestRe.MatchString(body)
70+
}
71+
72+
func IsHelpComment(body string) bool {
73+
return strings.Contains(body, TestWithoutTargetNote) ||
74+
strings.Contains(body, RetestWithTargetNote) ||
75+
strings.Contains(body, TargetNotFoundNote) ||
76+
strings.Contains(body, ThereAreNoTestAllJobsNote) ||
77+
// The response to "/test ?" has no header, so we have to search for these as well.
78+
strings.Contains(body, availableRequiredTestsNote) ||
79+
strings.Contains(body, availableOptionalTestsNote)
80+
}
81+
6382
// HelpMessage returns a user friendly help message with the
6483
//
6584
// available /test commands that can be triggered
@@ -79,10 +98,10 @@ func HelpMessage(org, repo, branch, note string, testAllNames, optionalTestComma
7998

8099
resp = note
81100
if requiredTestCommands.Len() > 0 {
82-
resp += fmt.Sprintf("The following commands are available to trigger required jobs:%s\n\n", listBuilder(requiredTestCommands))
101+
resp += availableRequiredTestsNote + listBuilder(requiredTestCommands) + "\n\n"
83102
}
84103
if optionalTestCommands.Len() > 0 {
85-
resp += fmt.Sprintf("The following commands are available to trigger optional jobs:%s\n\n", listBuilder(optionalTestCommands))
104+
resp += availableOptionalTestsNote + listBuilder(optionalTestCommands) + "\n\n"
86105
}
87106

88107
var testAllNote string

‎pkg/pjutil/help_test.go‎

Lines changed: 175 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,175 @@
1+
/*
2+
Copyright 2025 The Kubernetes Authors.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
17+
package pjutil
18+
19+
import (
20+
"testing"
21+
22+
"k8s.io/apimachinery/pkg/util/sets"
23+
)
24+
25+
func TestHelp(t *testing.T) {
26+
tests := []struct {
27+
name string
28+
userComment string
29+
matchingTests int
30+
mayNeedHelp bool
31+
shouldRespond bool
32+
note string
33+
shouldPrune bool
34+
}{
35+
{
36+
name: "empty comment is neither a failed nor a successful test trigger",
37+
userComment: "",
38+
mayNeedHelp: false,
39+
shouldPrune: false,
40+
},
41+
{
42+
name: "random comment is neither a failed nor a successful test trigger",
43+
userComment: "This is a comment about testing.",
44+
mayNeedHelp: false,
45+
shouldPrune: false,
46+
},
47+
{
48+
name: "request for existing test is a successful test trigger",
49+
userComment: "/test e2e",
50+
matchingTests: 1,
51+
mayNeedHelp: true,
52+
shouldRespond: false,
53+
shouldPrune: true,
54+
},
55+
{
56+
name: "request for non-existent test is an unsuccessful test trigger",
57+
userComment: "/test f2f",
58+
matchingTests: 0,
59+
mayNeedHelp: true,
60+
shouldRespond: true,
61+
note: TargetNotFoundNote,
62+
shouldPrune: false,
63+
},
64+
{
65+
name: "test all when tests exist is a successful test trigger",
66+
userComment: "/test all",
67+
matchingTests: 1,
68+
mayNeedHelp: true,
69+
shouldRespond: false,
70+
shouldPrune: true,
71+
},
72+
{
73+
name: "test all when no tests exist is an unsuccessful test trigger",
74+
userComment: "/test all",
75+
matchingTests: 0,
76+
mayNeedHelp: true,
77+
shouldRespond: true,
78+
note: ThereAreNoTestAllJobsNote,
79+
shouldPrune: false,
80+
},
81+
{
82+
name: "retest is a successful test trigger",
83+
userComment: "/retest",
84+
matchingTests: 1,
85+
mayNeedHelp: false,
86+
shouldPrune: true,
87+
},
88+
{
89+
name: "retest is a successful test trigger, even when no tests exist",
90+
userComment: "/retest",
91+
matchingTests: 0,
92+
mayNeedHelp: false,
93+
shouldPrune: true,
94+
},
95+
{
96+
name: "empty /test is invalid",
97+
userComment: "/test",
98+
mayNeedHelp: true,
99+
shouldRespond: true,
100+
note: TestWithoutTargetNote,
101+
shouldPrune: false,
102+
},
103+
{
104+
name: "retest with target is invalid",
105+
userComment: "/retest e2e",
106+
mayNeedHelp: true,
107+
shouldRespond: true,
108+
note: RetestWithTargetNote,
109+
shouldPrune: false,
110+
},
111+
{
112+
name: "/test ? is a request for help, not a trigger",
113+
userComment: "/test ?",
114+
mayNeedHelp: true,
115+
shouldRespond: true,
116+
note: "",
117+
shouldPrune: false,
118+
},
119+
}
120+
121+
required := sets.New("/test e2e", "/test e2e-serial", "/test unit")
122+
optional := sets.New("/test lint", "/test e2e-conformance-commodore64")
123+
all := required.Union(optional)
124+
helpBody := HelpMessage("", "", "", "", all, optional, required)
125+
126+
for _, tc := range tests {
127+
t.Run(tc.name, func(t *testing.T) {
128+
// None of the user comments should look like our help comment.
129+
if IsHelpComment(tc.userComment) {
130+
t.Errorf("Expected IsHelpComment(%q) to return false, got true", tc.userComment)
131+
}
132+
133+
// MayNeedHelpComment is true if the comment uses `/test` or
134+
// `/retest` at all (whether valid or invalid).
135+
mayNeedHelp := MayNeedHelpComment(tc.userComment)
136+
if mayNeedHelp != tc.mayNeedHelp {
137+
t.Errorf("Expected MayNeedHelpComment(%q) to return %v, got %v", tc.userComment, tc.mayNeedHelp, mayNeedHelp)
138+
}
139+
140+
// ShouldRespondWithHelp will check if userComment contains a
141+
// `/test` or `/retest` invocation that is invalid given the
142+
// number of matching tests.
143+
shouldRespond, note := ShouldRespondWithHelp(tc.userComment, tc.matchingTests)
144+
if shouldRespond != tc.shouldRespond {
145+
t.Errorf("Expected ShouldRespondWithHelp(%q, %d) to return %v, got %v", tc.userComment, tc.matchingTests, tc.shouldRespond, shouldRespond)
146+
}
147+
if note != tc.note {
148+
t.Errorf("Expected ShouldRespondWithHelp(%q) to return note %q, got %q", tc.userComment, tc.note, note)
149+
}
150+
151+
// If we should respond with a help comment, then HelpMessage
152+
// should return the expected message, and IsHelpComment should
153+
// recognize it.
154+
if shouldRespond {
155+
expectHelpMessage := tc.note + helpBody
156+
helpMessage := HelpMessage("", "", "", note, all, optional, required)
157+
if helpMessage != expectHelpMessage {
158+
t.Errorf("Expected HelpMessage() to return %q, got %q", expectHelpMessage, helpMessage)
159+
}
160+
if !IsHelpComment(helpMessage) {
161+
t.Errorf("Expected IsHelpComment(%q) to return true, got false", helpMessage)
162+
}
163+
}
164+
165+
// If we shouldn't respond with a help comment, then we possibly
166+
// should respond by deleted old help comments.
167+
if !shouldRespond {
168+
shouldPrune := ShouldRespondByPruningHelp(tc.userComment)
169+
if shouldPrune != tc.shouldPrune {
170+
t.Errorf("Expected ShouldRespondByPruningHelp(%q) to return %v, got %v", tc.userComment, tc.shouldPrune, shouldPrune)
171+
}
172+
}
173+
})
174+
}
175+
}

‎pkg/plugins/trigger/generic-comment.go‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,11 @@ import (
3131
"sigs.k8s.io/prow/pkg/plugins"
3232
)
3333

34-
func handleGenericComment(c Client, trigger plugins.Trigger, gc github.GenericCommentEvent) error {
34+
type commentPruner interface {
35+
PruneComments(shouldPrune func(github.IssueComment) bool)
36+
}
37+
38+
func handleGenericComment(c Client, cp commentPruner, trigger plugins.Trigger, gc github.GenericCommentEvent) error {
3539
org := gc.Repo.Owner.Login
3640
repo := gc.Repo.Name
3741
number := gc.Number
@@ -174,6 +178,15 @@ func handleGenericComment(c Client, trigger plugins.Trigger, gc github.GenericCo
174178
}
175179
}
176180
}
181+
182+
if pjutil.ShouldRespondByPruningHelp(textToCheck) {
183+
// The run was triggered by a "/test" or "/retest" (and not by
184+
// "/ok-to-test" for example), so prune any previous help message.
185+
cp.PruneComments(func(comment github.IssueComment) bool {
186+
return pjutil.IsHelpComment(comment.Body)
187+
})
188+
}
189+
177190
return RunRequestedWithLabels(c, pr, baseSHA, toTest, gc.GUID, additionalLabels)
178191
}
179192

0 commit comments

Comments
 (0)