diff --git a/pkg/pjutil/help.go b/pkg/pjutil/help.go index 2ec5b4eb3f..af7fb62180 100644 --- a/pkg/pjutil/help.go +++ b/pkg/pjutil/help.go @@ -30,10 +30,15 @@ var ( RetestWithTargetRe = regexp.MustCompile(`(?m)^/retest[ \t]+\S+`) TestWithAnyTargetRe = regexp.MustCompile(`(?m)^/test[ \t]+\S+`) + anyTestRe = regexp.MustCompile(`(?m)^/(?:re)?test\b`) + TestWithoutTargetNote = "The `/test` command needs one or more targets.\n" RetestWithTargetNote = "The `/retest` command does not accept any targets.\n" TargetNotFoundNote = "The specified target(s) for `/test` were not found.\n" ThereAreNoTestAllJobsNote = "No jobs can be run with `/test all`.\n" + + availableRequiredTestsNote = "The following commands are available to trigger required jobs:" + availableOptionalTestsNote = "The following commands are available to trigger optional jobs:" ) func MayNeedHelpComment(body string) bool { @@ -60,6 +65,20 @@ func ShouldRespondWithHelp(body string, toRunOrSkip int) (bool, string) { } } +func ShouldRespondByPruningHelp(body string) bool { + return anyTestRe.MatchString(body) +} + +func IsHelpComment(body string) bool { + return strings.Contains(body, TestWithoutTargetNote) || + strings.Contains(body, RetestWithTargetNote) || + strings.Contains(body, TargetNotFoundNote) || + strings.Contains(body, ThereAreNoTestAllJobsNote) || + // The response to "/test ?" has no header, so we have to search for these as well. + strings.Contains(body, availableRequiredTestsNote) || + strings.Contains(body, availableOptionalTestsNote) +} + // HelpMessage returns a user friendly help message with the // // available /test commands that can be triggered @@ -79,10 +98,10 @@ func HelpMessage(org, repo, branch, note string, testAllNames, optionalTestComma resp = note if requiredTestCommands.Len() > 0 { - resp += fmt.Sprintf("The following commands are available to trigger required jobs:%s\n\n", listBuilder(requiredTestCommands)) + resp += availableRequiredTestsNote + listBuilder(requiredTestCommands) + "\n\n" } if optionalTestCommands.Len() > 0 { - resp += fmt.Sprintf("The following commands are available to trigger optional jobs:%s\n\n", listBuilder(optionalTestCommands)) + resp += availableOptionalTestsNote + listBuilder(optionalTestCommands) + "\n\n" } var testAllNote string diff --git a/pkg/pjutil/help_test.go b/pkg/pjutil/help_test.go new file mode 100644 index 0000000000..b7e00f5bc2 --- /dev/null +++ b/pkg/pjutil/help_test.go @@ -0,0 +1,175 @@ +/* +Copyright 2025 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package pjutil + +import ( + "testing" + + "k8s.io/apimachinery/pkg/util/sets" +) + +func TestHelp(t *testing.T) { + tests := []struct { + name string + userComment string + matchingTests int + mayNeedHelp bool + shouldRespond bool + note string + shouldPrune bool + }{ + { + name: "empty comment is neither a failed nor a successful test trigger", + userComment: "", + mayNeedHelp: false, + shouldPrune: false, + }, + { + name: "random comment is neither a failed nor a successful test trigger", + userComment: "This is a comment about testing.", + mayNeedHelp: false, + shouldPrune: false, + }, + { + name: "request for existing test is a successful test trigger", + userComment: "/test e2e", + matchingTests: 1, + mayNeedHelp: true, + shouldRespond: false, + shouldPrune: true, + }, + { + name: "request for non-existent test is an unsuccessful test trigger", + userComment: "/test f2f", + matchingTests: 0, + mayNeedHelp: true, + shouldRespond: true, + note: TargetNotFoundNote, + shouldPrune: false, + }, + { + name: "test all when tests exist is a successful test trigger", + userComment: "/test all", + matchingTests: 1, + mayNeedHelp: true, + shouldRespond: false, + shouldPrune: true, + }, + { + name: "test all when no tests exist is an unsuccessful test trigger", + userComment: "/test all", + matchingTests: 0, + mayNeedHelp: true, + shouldRespond: true, + note: ThereAreNoTestAllJobsNote, + shouldPrune: false, + }, + { + name: "retest is a successful test trigger", + userComment: "/retest", + matchingTests: 1, + mayNeedHelp: false, + shouldPrune: true, + }, + { + name: "retest is a successful test trigger, even when no tests exist", + userComment: "/retest", + matchingTests: 0, + mayNeedHelp: false, + shouldPrune: true, + }, + { + name: "empty /test is invalid", + userComment: "/test", + mayNeedHelp: true, + shouldRespond: true, + note: TestWithoutTargetNote, + shouldPrune: false, + }, + { + name: "retest with target is invalid", + userComment: "/retest e2e", + mayNeedHelp: true, + shouldRespond: true, + note: RetestWithTargetNote, + shouldPrune: false, + }, + { + name: "/test ? is a request for help, not a trigger", + userComment: "/test ?", + mayNeedHelp: true, + shouldRespond: true, + note: "", + shouldPrune: false, + }, + } + + required := sets.New("/test e2e", "/test e2e-serial", "/test unit") + optional := sets.New("/test lint", "/test e2e-conformance-commodore64") + all := required.Union(optional) + helpBody := HelpMessage("", "", "", "", all, optional, required) + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + // None of the user comments should look like our help comment. + if IsHelpComment(tc.userComment) { + t.Errorf("Expected IsHelpComment(%q) to return false, got true", tc.userComment) + } + + // MayNeedHelpComment is true if the comment uses `/test` or + // `/retest` at all (whether valid or invalid). + mayNeedHelp := MayNeedHelpComment(tc.userComment) + if mayNeedHelp != tc.mayNeedHelp { + t.Errorf("Expected MayNeedHelpComment(%q) to return %v, got %v", tc.userComment, tc.mayNeedHelp, mayNeedHelp) + } + + // ShouldRespondWithHelp will check if userComment contains a + // `/test` or `/retest` invocation that is invalid given the + // number of matching tests. + shouldRespond, note := ShouldRespondWithHelp(tc.userComment, tc.matchingTests) + if shouldRespond != tc.shouldRespond { + t.Errorf("Expected ShouldRespondWithHelp(%q, %d) to return %v, got %v", tc.userComment, tc.matchingTests, tc.shouldRespond, shouldRespond) + } + if note != tc.note { + t.Errorf("Expected ShouldRespondWithHelp(%q) to return note %q, got %q", tc.userComment, tc.note, note) + } + + // If we should respond with a help comment, then HelpMessage + // should return the expected message, and IsHelpComment should + // recognize it. + if shouldRespond { + expectHelpMessage := tc.note + helpBody + helpMessage := HelpMessage("", "", "", note, all, optional, required) + if helpMessage != expectHelpMessage { + t.Errorf("Expected HelpMessage() to return %q, got %q", expectHelpMessage, helpMessage) + } + if !IsHelpComment(helpMessage) { + t.Errorf("Expected IsHelpComment(%q) to return true, got false", helpMessage) + } + } + + // If we shouldn't respond with a help comment, then we possibly + // should respond by deleted old help comments. + if !shouldRespond { + shouldPrune := ShouldRespondByPruningHelp(tc.userComment) + if shouldPrune != tc.shouldPrune { + t.Errorf("Expected ShouldRespondByPruningHelp(%q) to return %v, got %v", tc.userComment, tc.shouldPrune, shouldPrune) + } + } + }) + } +} diff --git a/pkg/plugins/trigger/generic-comment.go b/pkg/plugins/trigger/generic-comment.go index 1831d94d80..19bdaf77f7 100644 --- a/pkg/plugins/trigger/generic-comment.go +++ b/pkg/plugins/trigger/generic-comment.go @@ -31,7 +31,11 @@ import ( "sigs.k8s.io/prow/pkg/plugins" ) -func handleGenericComment(c Client, trigger plugins.Trigger, gc github.GenericCommentEvent) error { +type commentPruner interface { + PruneComments(shouldPrune func(github.IssueComment) bool) +} + +func handleGenericComment(c Client, cp commentPruner, trigger plugins.Trigger, gc github.GenericCommentEvent) error { org := gc.Repo.Owner.Login repo := gc.Repo.Name number := gc.Number @@ -174,6 +178,15 @@ func handleGenericComment(c Client, trigger plugins.Trigger, gc github.GenericCo } } } + + if pjutil.ShouldRespondByPruningHelp(textToCheck) { + // The run was triggered by a "/test" or "/retest" (and not by + // "/ok-to-test" for example), so prune any previous help message. + cp.PruneComments(func(comment github.IssueComment) bool { + return pjutil.IsHelpComment(comment.Body) + }) + } + return RunRequestedWithLabels(c, pr, baseSHA, toTest, gc.GUID, additionalLabels) } diff --git a/pkg/plugins/trigger/generic-comment_test.go b/pkg/plugins/trigger/generic-comment_test.go index 7888f1d9c7..c714572bb4 100644 --- a/pkg/plugins/trigger/generic-comment_test.go +++ b/pkg/plugins/trigger/generic-comment_test.go @@ -47,6 +47,14 @@ func issueLabels(labels ...string) []string { const shouldNotAddComment = "" +type fakeCommentPruner struct { + called bool +} + +func (cp *fakeCommentPruner) PruneComments(shouldPrune func(github.IssueComment) bool) { + cp.called = true +} + type testcase struct { name string @@ -64,6 +72,7 @@ type testcase struct { IssueLabels []string IgnoreOkToTest bool AddedComment string + PruneHelp bool } func TestHandleGenericComment(t *testing.T) { @@ -123,6 +132,7 @@ func TestHandleGenericComment(t *testing.T) { State: "open", IsPR: true, ShouldBuild: true, + PruneHelp: true, }, { name: "reject /test from non-trusted member when PR author is untrusted", @@ -141,6 +151,7 @@ func TestHandleGenericComment(t *testing.T) { State: "open", IsPR: true, ShouldBuild: true, + PruneHelp: true, IssueLabels: issueLabels(labels.OkToTest), }, { @@ -151,6 +162,7 @@ func TestHandleGenericComment(t *testing.T) { State: "open", IsPR: true, ShouldBuild: true, + PruneHelp: true, IssueLabels: issueLabels(labels.NeedsOkToTest, labels.OkToTest), RemovedLabels: issueLabels(labels.NeedsOkToTest), }, @@ -201,6 +213,7 @@ func TestHandleGenericComment(t *testing.T) { State: "open", IsPR: true, ShouldBuild: true, + PruneHelp: true, }, { name: "Wrong branch", @@ -221,6 +234,7 @@ func TestHandleGenericComment(t *testing.T) { IsPR: true, ShouldBuild: true, StartsExactly: "pull-jib", + PruneHelp: true, }, { name: "Retest with one running and one failed, trailing space.", @@ -231,6 +245,7 @@ func TestHandleGenericComment(t *testing.T) { IsPR: true, ShouldBuild: true, StartsExactly: "pull-jib", + PruneHelp: true, }, { name: "test of silly regex job", @@ -255,6 +270,10 @@ func TestHandleGenericComment(t *testing.T) { }, ShouldBuild: true, StartsExactly: "pull-jab", + + // We only add/remove help comments for things that look like the + // normal triggers. + PruneHelp: false, }, { name: "needs-ok-to-test label is removed when no presubmit runs by default", @@ -345,6 +364,7 @@ func TestHandleGenericComment(t *testing.T) { }, ShouldBuild: true, StartsExactly: "pull-jab", + PruneHelp: true, }, { name: "Retest of skip_if_only_changed job that hasn't run. Changes require job", @@ -372,6 +392,7 @@ func TestHandleGenericComment(t *testing.T) { }, ShouldBuild: true, StartsExactly: "pull-jab", + PruneHelp: true, }, { name: "Retest of run_if_changed job that failed. Changes require job", @@ -398,6 +419,7 @@ func TestHandleGenericComment(t *testing.T) { }, ShouldBuild: true, StartsExactly: "pull-jib", + PruneHelp: true, }, { name: "Retest of skip_if_only_changed job that failed. Changes require job", @@ -424,6 +446,7 @@ func TestHandleGenericComment(t *testing.T) { }, ShouldBuild: true, StartsExactly: "pull-jib", + PruneHelp: true, }, { name: "/test of run_if_changed job that has passed", @@ -450,6 +473,7 @@ func TestHandleGenericComment(t *testing.T) { }, ShouldBuild: true, StartsExactly: "pull-jub", + PruneHelp: true, }, { name: "/test of skip_if_only_changed job that has passed", @@ -476,6 +500,7 @@ func TestHandleGenericComment(t *testing.T) { }, ShouldBuild: true, StartsExactly: "pull-jub", + PruneHelp: true, }, { name: "Retest triggers failed job", @@ -498,6 +523,7 @@ func TestHandleGenericComment(t *testing.T) { }, }, ShouldBuild: true, + PruneHelp: true, }, { name: "Retest triggers failed job that is optional", @@ -521,6 +547,7 @@ func TestHandleGenericComment(t *testing.T) { }, }, ShouldBuild: true, + PruneHelp: true, }, { name: "Retest-Required doesn't triggers failed job", @@ -543,6 +570,7 @@ func TestHandleGenericComment(t *testing.T) { }, }, ShouldBuild: true, + PruneHelp: true, }, { name: "Retest-Required doesn't trigger failed job that is optional", @@ -565,6 +593,7 @@ func TestHandleGenericComment(t *testing.T) { }, }, }, + PruneHelp: true, }, { name: "Retest of run_if_changed job that failed. Changes do not require the job", @@ -590,6 +619,7 @@ func TestHandleGenericComment(t *testing.T) { }, }, ShouldBuild: true, + PruneHelp: true, }, { name: "Retest of skip_if_only_changed job that failed. Changes do not require the job", @@ -615,6 +645,7 @@ func TestHandleGenericComment(t *testing.T) { }, }, ShouldBuild: true, + PruneHelp: true, }, { name: "Run if changed job triggered by /ok-to-test", @@ -708,6 +739,7 @@ func TestHandleGenericComment(t *testing.T) { }, ShouldBuild: true, StartsExactly: "pull-jab", + PruneHelp: true, }, { name: "branch-sharded job. no shard matches base branch", @@ -767,6 +799,7 @@ func TestHandleGenericComment(t *testing.T) { }, }, }, + PruneHelp: true, }, { name: "/retest of SkipIfOnlyChanged job that doesn't need to run and hasn't run", @@ -792,6 +825,7 @@ func TestHandleGenericComment(t *testing.T) { }, }, }, + PruneHelp: true, }, { name: "explicit /test for RunIfChanged job that doesn't need to run", @@ -870,6 +904,7 @@ func TestHandleGenericComment(t *testing.T) { }, ShouldBuild: true, StartsExactly: "pull-jub", + PruneHelp: true, }, { name: "/test all of skip_if_only_changed job that has passed and needs to run", @@ -896,6 +931,7 @@ func TestHandleGenericComment(t *testing.T) { }, ShouldBuild: true, StartsExactly: "pull-jub", + PruneHelp: true, }, { name: "/test all of run_if_changed job that has passed and doesn't need to run", @@ -953,6 +989,7 @@ func TestHandleGenericComment(t *testing.T) { State: "open", IsPR: true, ShouldBuild: true, + PruneHelp: true, }, { name: `Non-trusted member after "/lgtm" and "/approve"`, @@ -1087,7 +1124,7 @@ func TestHandleGenericComment(t *testing.T) { AddedComment: pjutil.TestWithoutTargetNote + helpComment + helpTestAllWithJobsComment, }, { - name: "ignore `/test` with no target results in a code block", + name: "ignore `/test` with no target in a code block", Author: "trusted-member", Body: "```\n/test\n```", State: "open", @@ -1095,7 +1132,7 @@ func TestHandleGenericComment(t *testing.T) { AddedComment: shouldNotAddComment, }, { - name: "ignore `/test` with no target results in a tilda code block", + name: "ignore `/test` with no target in a tilde code block", Author: "trusted-member", Body: "~~~\n/test\n~~~", State: "open", @@ -1119,7 +1156,7 @@ func TestHandleGenericComment(t *testing.T) { AddedComment: pjutil.RetestWithTargetNote + helpComment + helpTestAllWithJobsComment, }, { - name: "/retest with trailing words results in a code block", + name: "/retest with trailing words in a code block", Author: "trusted-member", Body: produceCodeBlock("/retest FOO", false), State: "open", @@ -1134,6 +1171,7 @@ func TestHandleGenericComment(t *testing.T) { IsPR: true, ShouldBuild: true, StartsExactly: "pull-jib", + PruneHelp: true, }, { name: "/test with unknown target results in a help message", @@ -1144,7 +1182,7 @@ func TestHandleGenericComment(t *testing.T) { AddedComment: pjutil.TargetNotFoundNote + helpComment + helpTestAllWithJobsComment, }, { - name: "/test with unknown target results in code block. Should be ignored", + name: "/test with unknown target in code block. Should be ignored", Author: "trusted-member", Body: produceCodeBlock("/test FOO", false), State: "open", @@ -1152,7 +1190,7 @@ func TestHandleGenericComment(t *testing.T) { AddedComment: shouldNotAddComment, }, { - name: "two `/test` with unknown target results. One in a code block, and one is not.", + name: "two `/test` with unknown target. One in a code block, and one is not.", Author: "trusted-member", Body: produceCodeBlock("/test FOO", true) + "/test BAR", State: "open", @@ -1160,7 +1198,7 @@ func TestHandleGenericComment(t *testing.T) { AddedComment: pjutil.TargetNotFoundNote + helpComment + helpTestAllWithJobsComment, }, { - name: "two `/test` with unknown target results. One out of a code block, and one is inside.", + name: "two `/test` with unknown target. One out of a code block, and one is inside.", Author: "trusted-member", Body: "/test FOO\n" + produceCodeBlock("/test BAR", false), State: "open", @@ -1485,23 +1523,25 @@ func TestHandleGenericComment(t *testing.T) { } trigger.SetDefaults() + cp := &fakeCommentPruner{} + log.Printf("running case %s", tc.name) // In some cases handleGenericComment can be called twice for the same event. // For instance on Issue/PR creation and modification. // Let's call it twice to ensure idempotency. - if err := handleGenericComment(c, trigger, event); err != nil { + if err := handleGenericComment(c, cp, trigger, event); err != nil { t.Fatalf("%s: didn't expect error: %s", tc.name, err) } - validate(t, fakeProwJobClient.Fake.Actions(), g, tc) - if err := handleGenericComment(c, trigger, event); err != nil { + validate(t, fakeProwJobClient.Fake.Actions(), g, cp, tc) + if err := handleGenericComment(c, cp, trigger, event); err != nil { t.Fatalf("%s: didn't expect error: %s", tc.name, err) } - validate(t, fakeProwJobClient.Fake.Actions(), g, tc) + validate(t, fakeProwJobClient.Fake.Actions(), g, cp, tc) }) } } -func validate(t *testing.T, actions []clienttesting.Action, g *fakegithub.FakeClient, tc testcase) { +func validate(t *testing.T, actions []clienttesting.Action, g *fakegithub.FakeClient, cp *fakeCommentPruner, tc testcase) { startedContexts := sets.New[string]() for _, action := range actions { switch action := action.(type) { @@ -1541,6 +1581,11 @@ func validate(t *testing.T, actions []clienttesting.Action, g *fakegithub.FakeCl } } } + if tc.PruneHelp && !cp.called { + t.Errorf("expected to prune old help comment on successful trigger, but did not") + } else if !tc.PruneHelp && cp.called { + t.Errorf("expected to not prune old help comment, but did") + } } func TestRetestFilter(t *testing.T) { diff --git a/pkg/plugins/trigger/trigger.go b/pkg/plugins/trigger/trigger.go index 484ca8b9f3..2e5bf11165 100644 --- a/pkg/plugins/trigger/trigger.go +++ b/pkg/plugins/trigger/trigger.go @@ -210,7 +210,11 @@ func handlePullRequest(pc plugins.Agent, pr github.PullRequestEvent) error { } func handleGenericCommentEvent(pc plugins.Agent, gc github.GenericCommentEvent) error { - return handleGenericComment(getClient(pc), pc.PluginConfig.TriggerFor(gc.Repo.Owner.Login, gc.Repo.Name), gc) + cp, err := pc.CommentPruner() + if err != nil { + return err + } + return handleGenericComment(getClient(pc), cp, pc.PluginConfig.TriggerFor(gc.Repo.Owner.Login, gc.Repo.Name), gc) } func handlePush(pc plugins.Agent, pe github.PushEvent) error {