test(pipelineascode): assert on queued check-run status for pending PipelineRuns by pujitha24 · Pull Request #2904 · tektoncd/pipelines-as-code

pujitha24 · 2026-08-04T21:17:42Z

📝 Description of the Change

The check-run mock helper in pkg/pipelineascode/pipelineascode_test.go only asserted on a check-run write when its status was "completed", silently ignoring every other status. Two testcases exercise a PipelineRun left in the Pending spec state, which never reaches "completed" in this code path — it only ever emits a "queued" check-run. Because those two testcases also left finalStatus/finalStatusText unset, they passed without validating anything at all.

This adds an opt-in queuedStatusText field so those two testcases now assert the "queued" check-run text contains "has been queued". The assertion is opt-in (only applied when the field is non-empty) so it doesn't affect any other testcase, including an unrelated access-denied flow that also uses the "queued" status with different text.

This is a narrow fix, not a full resolution of #1957. As pointed out in review, the linked issue also asks for: (1) asserting on the "in_progress" status, not just "completed"/"queued", and (2) splitting the monolithic table-driven test into focused, individual tests. Neither of those is done here — this PR only closes the specific vacuous-assertion gap for the two pending-state testcases. #1957 should stay open to track the remaining in_progress coverage and the structural refactor.

🔗 Linked GitHub Issue

Relates to #1957 (does not close it — see scope note above; remaining work should stay tracked there)

🧪 Testing Strategy

🤖 AI Assistance

AI assistance can be used for various tasks, such as code generation,
documentation, or testing.

Please indicate whether you have used AI assistance
for this PR and provide details if applicable.

I have not used any AI assistance for this PR.
I have used AI assistance for this PR.

Important

Slop will be simply rejected, if you are using AI assistance you need to make sure you
understand the code generated and that it meets the project's standards. you
need at least know how to run the code and deploy it (if needed). See
startpaac to make it easy
to deploy and test your code changes.

If the majority of the code in this PR was generated by an AI, please add a Co-authored-by trailer to your commit message.
For example:

Co-authored-by: Claude noreply@anthropic.com

✅ Submitter Checklist

AI assistance: this change was drafted with Claude Code.

Copilot

Pull request overview

Strengthens pkg/pipelineascode unit tests by ensuring GitHub check-run writes are asserted for PipelineRuns that remain in Pending and therefore only emit a queued check-run status, closing a gap where those test cases previously passed without validating any check-run content.

Changes:

Extend the shared GitHub replies test helper to optionally assert check-run output text when status is queued.
Add a queuedStatusText opt-in field to the table-driven tests and populate it for the pending PipelineRun scenarios.
Keep existing behavior unchanged for other test cases (including other queued flows) by only asserting when the new field is set.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

theakshaypant

Thanks for working on this @pujitha24!

The fix for the two pending-state assertions is correct. However, I don't think this sufficiently addresses the linked issue to close it. The issue raises two problems:

The test only validates "completed" status: queued, in_progress, and error states are not checked
The test structure is a monolithic table-driven test that tries to do too much and should be split into focused tests

This PR addresses a narrow slice of problem 1 but the broader described gaps remain:

in_progress is still silently ignored
No status lifecycle tracking
No structural refactor

I'd suggest either expanding the scope here or keeping #1957 open for the remaining work.

theakshaypant · 2026-08-07T04:27:41Z

+			// testcases that expect that outcome opt into checking the "queued" status
+			// text instead via queuedStatusText.
+			switch created.GetStatus() {
+			case "completed":


[nit] Similar to queuedStatus on line 101, consider using a constant in place of the literal here.

… in test switch Addresses review nit on PR tektoncd#2904: use the existing CompletedStatus constant for the completed case, matching the queuedStatus constant already used in the same switch. Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>

pujitha24 · 2026-08-07T06:33:37Z

Thanks for the review! The constant nit is already handled — the latest commit (1b22e5a) replaced the "completed" literal with the CompletedStatus constant, matching queuedStatus on the next case.

On scope: agreed, I'd rather not stretch this PR to cover in_progress coverage and the test-splitting refactor — those are more involved changes that deserve their own focused PR and review. I'll leave #1957 open to track that remaining work, as noted in the PR description. Let me know if you'd prefer it handled differently.

theakshaypant · 2026-08-07T06:45:28Z

/ok-to-test

… in test switch Addresses review nit on PR tektoncd#2904: use the existing CompletedStatus constant for the completed case, matching the queuedStatus constant already used in the same switch. Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>

zakisk · 2026-08-11T16:25:55Z

/ok-to-test

codecov · 2026-08-11T16:28:54Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.66%. Comparing base (d799937) to head (ade1510).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2904   +/-   ##
=======================================
  Coverage   80.66%   80.66%           
=======================================
  Files         164      164           
  Lines       13910    13910           
=======================================
  Hits        11221    11221           
  Misses       1967     1967           
  Partials      722      722

Flag	Coverage Δ
unit-tests	`80.66% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

pujitha24 · 2026-08-12T00:16:42Z

Could a maintainer add the ok-to-test label when you get a chance? The e2e jobs are all waiting on it — happy to fix anything they surface.

chmouel · 2026-08-12T16:57:00Z

/ok-to-test

… in test switch Addresses review nit on PR tektoncd#2904: use the existing CompletedStatus constant for the completed case, matching the queuedStatus constant already used in the same switch. Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>

zakisk · 2026-08-14T06:08:28Z

/ok-to-test

… in test switch Addresses review nit on PR tektoncd#2904: use the existing CompletedStatus constant for the completed case, matching the queuedStatus constant already used in the same switch. Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>

pujitha24 · 2026-08-18T19:39:47Z

For anyone re-triggering this: the nine failing e2e tests legs aren't a test failure. They all stop at step 3, "Check user permissions on PRs" (pipelines-as-code/ok-to-test@v1, gated on TARGET_TEAM_SLUGS), and every step after it is skipped — hence the uniform 13-26s failures. That check is team-membership based, so the /ok-to-test comments above don't satisfy it and re-running won't change the result while I'm outside the org.

Flagging it so nobody spends more time re-triggering. The diff itself is test-only (asserting queued check-run status for pending PipelineRuns) and unit tests pass. Happy to leave this open or close it, whichever is less overhead for you.

… in test switch Addresses review nit on PR tektoncd#2904: use the existing CompletedStatus constant for the completed case, matching the queuedStatus constant already used in the same switch. Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>

zakisk · 2026-08-19T14:31:26Z

/ok-to-test

zakisk · 2026-08-19T14:31:55Z

For anyone re-triggering this: the nine failing e2e tests legs aren't a test failure. They all stop at step 3, "Check user permissions on PRs" (pipelines-as-code/ok-to-test@v1, gated on TARGET_TEAM_SLUGS), and every step after it is skipped — hence the uniform 13-26s failures. That check is team-membership based, so the /ok-to-test comments above don't satisfy it and re-running won't change the result while I'm outside the org.

Flagging it so nobody spends more time re-triggering. The diff itself is test-only (asserting queued check-run status for pending PipelineRuns) and unit tests pass. Happy to leave this open or close it, whichever is less overhead for you.

yeah, only unit test are required. triggered

zakisk · 2026-08-19T17:16:37Z

/ok-to-test

zakisk · 2026-08-20T07:19:11Z

/test go-testing

zakisk · 2026-08-25T10:40:16Z

/ok-to-test

Motivation: The check-run mock in pkg/pipelineascode/pipelineascode_test.go only asserted on a check-run write when its status was "completed", silently ignoring any other status (e.g. "queued"). Two testcases - "pull request/pipelinerun created in pending state (state changed by other controller)" and its "without installationID" variant - exercise a PipelineRun left in the Pending spec state, which never reaches "completed" in this code path (it only ever emits a "queued" check-run with the queuing-template text). Because those two testcases also left finalStatus/finalStatusText unset, they passed without validating anything at all. Approach: Added an opt-in queuedStatusText field to the testcase struct and a matching parameter to testSetupCommonGhReplies. The check-run handler now switches on status: the existing "completed" assertion is unchanged, and a new "queued" case asserts the check-run text contains queuedStatusText, but only when that field is non-empty - so none of the other testcases change behavior. The two pending-PipelineRun testcases now set queuedStatusText to "has been queued", closing the specific vacuous-assertion gap. This is a targeted fix, not the full per-status-transition refactor also suggested for this test file; splitting the table-driven test into smaller individual tests is left out of scope. Note the "queued" status is also used by an unrelated access-denied flow ("pending approval, waiting for an /ok-to-test") in pkg/pipelineascode/match.go, so the assertion is opt-in rather than unconditional to avoid asserting an unrelated message text on that path. Validation: - go build ./... - go test ./pkg/pipelineascode/... (full package, including TestRun) - go vet ./pkg/pipelineascode/... - gofmt -l and gofumpt -l on the changed file (clean) - Confirmed the new assertion has teeth: temporarily corrupted the "queued" status text in pkg/formatting/templates/queuing.go.tmpl (the HTML template actually used by the GitHub provider) and reran the two pending-PipelineRun subtests, which failed with a clear assertion message; restored the template and reran to confirm they pass again. Report: tektoncd#1957 Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>

zakisk · 2026-08-31T06:28:02Z

/ok-to-test

zakisk · 2026-08-31T10:43:09Z

/test go-testing

chmouel · 2026-09-02T09:34:13Z

+			// We create multiple statuses over the life of a testcase (e.g. queued,
+			// in_progress, completed), but "completed" is the only one guaranteed to
+			// be sent for every testcase, so it's asserted unconditionally. PipelineRuns
+			// left pending by an external controller never reach "completed" here, so
+			// testcases that expect that outcome opt into checking the "queued" status
+			// text instead via queuedStatusText.
+			switch created.GetStatus() {
+			case CompletedStatus:
 				assert.Equal(t, created.GetConclusion(), finalStatus, "we got the status `%s` but we should have get the status `%s`", created.GetConclusion(), finalStatus)
 				assert.Assert(t, strings.Contains(created.GetOutput().GetText(), finalStatusText),
 					"GetStatus/CheckRun %s != %s", created.GetOutput().GetText(), finalStatusText)
+			case queuedStatus:
+				if queuedStatusText != "" {
+					assert.Assert(t, strings.Contains(created.GetOutput().GetText(), queuedStatusText),
+						"GetStatus/CheckRun queued text %q does not contain %q", created.GetOutput().GetText(), queuedStatusText)
+				}
 			}


Nit: this can be flattened — the nested if inside the queued case only exists to make that assertion opt-in, which two plain guard clauses express more directly than a switch:

Suggested change

// We create multiple statuses over the life of a testcase (e.g. queued,

// in_progress, completed), but "completed" is the only one guaranteed to

// be sent for every testcase, so it's asserted unconditionally. PipelineRuns

// left pending by an external controller never reach "completed" here, so

// testcases that expect that outcome opt into checking the "queued" status

// text instead via queuedStatusText.

switch created.GetStatus() {

case CompletedStatus:

assert.Equal(t, created.GetConclusion(), finalStatus, "we got the status `%s` but we should have get the status `%s`", created.GetConclusion(), finalStatus)

assert.Assert(t, strings.Contains(created.GetOutput().GetText(), finalStatusText),

"GetStatus/CheckRun %s != %s", created.GetOutput().GetText(), finalStatusText)

case queuedStatus:

if queuedStatusText != "" {

assert.Assert(t, strings.Contains(created.GetOutput().GetText(), queuedStatusText),

"GetStatus/CheckRun queued text %q does not contain %q", created.GetOutput().GetText(), queuedStatusText)

}

}

// "completed" is asserted unconditionally; "queued" is opt-in via queuedStatusText,

// since PipelineRuns left pending by another controller never reach "completed" here.

status := created.GetStatus()

if status == CompletedStatus {

assert.Equal(t, created.GetConclusion(), finalStatus, "we got the status `%s` but we should have get the status `%s`", created.GetConclusion(), finalStatus)

assert.Assert(t, strings.Contains(created.GetOutput().GetText(), finalStatusText),

"GetStatus/CheckRun %s != %s", created.GetOutput().GetText(), finalStatusText)

}

if status == queuedStatus && queuedStatusText != "" {

assert.Assert(t, strings.Contains(created.GetOutput().GetText(), queuedStatusText),

"GetStatus/CheckRun queued text %q does not contain %q", created.GetOutput().GetText(), queuedStatusText)

}

chmouel · 2026-09-02T09:35:27Z

 				assert.Assert(t, strings.Contains(created.GetOutput().GetText(), finalStatusText),
 					"GetStatus/CheckRun %s != %s", created.GetOutput().GetText(), finalStatusText)
+			case queuedStatus:
+				if queuedStatusText != "" {


queuedStatusText only validates the payload if a queued request happens to arrive. If a regression stops emitting the queued check-run entirely, this handler is never invoked for that status and both pending-state tests still pass. Please record whether the expected queued update was received and assert that after Run completes, so these tests verify both the request and its text.

…us was sent Flattens the completed/queued check into two guard clauses per review feedback, and has testSetupCommonGhReplies report whether a queued check-run request was actually received so the pending-state testcases fail if that update stops being sent, instead of only validating its text when one happens to arrive. Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>

pujitha24 · 2026-09-02T09:50:06Z

Fixed both. Flattened the switch into two guard clauses like you suggested, and testSetupCommonGhReplies now returns a closure that reports whether a queued check-run was actually received — the pending-state cases assert that after Run returns, so they'd fail if the queued update stopped being sent entirely, not just if its text were wrong.

Copilot AI lite review requested due to automatic review settings August 4, 2026 21:17

Copilot started reviewing on behalf of pujitha24 August 4, 2026 21:18 View session

Copilot AI reviewed Aug 4, 2026

View reviewed changes

theakshaypant reviewed Aug 7, 2026

View reviewed changes

zakisk force-pushed the auto/issue-1957 branch from 1b22e5a to 1350d8b Compare August 11, 2026 16:25

zakisk added the ok-to-test label Aug 11, 2026

tekton-pac-bot removed the ok-to-test label Aug 11, 2026

zakisk approved these changes Aug 11, 2026

View reviewed changes

chmouel added the ok-to-test label Aug 12, 2026

tekton-pac-bot removed the ok-to-test label Aug 12, 2026

chmouel added the ok-to-test label Aug 12, 2026

tekton-pac-bot removed the ok-to-test label Aug 12, 2026

zakisk force-pushed the auto/issue-1957 branch from 824b1fc to 77ad1ee Compare August 14, 2026 06:07

zakisk added the ok-to-test label Aug 14, 2026

tekton-pac-bot removed the ok-to-test label Aug 14, 2026

pujitha24 force-pushed the auto/issue-1957 branch from 77ad1ee to 69c1442 Compare August 14, 2026 13:46

zakisk force-pushed the auto/issue-1957 branch from 69c1442 to 25e0f5f Compare August 19, 2026 14:31

zakisk force-pushed the auto/issue-1957 branch from 25e0f5f to d498c0b Compare August 19, 2026 17:15

zakisk added the ok-to-test label Aug 25, 2026

tekton-pac-bot removed the ok-to-test label Aug 25, 2026

zakisk force-pushed the auto/issue-1957 branch from 76a85f2 to ade1510 Compare August 31, 2026 06:27

zakisk added the ok-to-test label Aug 31, 2026

tekton-pac-bot removed the ok-to-test label Aug 31, 2026

chmouel reviewed Sep 2, 2026

View reviewed changes

pujitha24 mentioned this pull request Sep 2, 2026

Bracket IPv6 hosts when building https:// URLs k0sproject/k0smotron#1566

Open

Uh oh!

Conversation

pujitha24 commented Aug 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📝 Description of the Change

🔗 Linked GitHub Issue

🧪 Testing Strategy

🤖 AI Assistance

✅ Submitter Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

theakshaypant left a comment

Choose a reason for hiding this comment

Uh oh!

theakshaypant Aug 7, 2026

Choose a reason for hiding this comment

Uh oh!

pujitha24 commented Aug 7, 2026

Uh oh!

theakshaypant commented Aug 7, 2026

Uh oh!

zakisk commented Aug 11, 2026

Uh oh!

codecov Bot commented Aug 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

pujitha24 commented Aug 12, 2026

Uh oh!

chmouel commented Aug 12, 2026

Uh oh!

zakisk commented Aug 14, 2026

Uh oh!

pujitha24 commented Aug 18, 2026

Uh oh!

zakisk commented Aug 19, 2026

Uh oh!

zakisk commented Aug 19, 2026

Uh oh!

zakisk commented Aug 19, 2026

Uh oh!

zakisk commented Aug 20, 2026

Uh oh!

zakisk commented Aug 25, 2026

Uh oh!

zakisk commented Aug 31, 2026

Uh oh!

zakisk commented Aug 31, 2026

Uh oh!

chmouel Sep 2, 2026

Choose a reason for hiding this comment

Uh oh!

chmouel Sep 2, 2026

Choose a reason for hiding this comment

Uh oh!

pujitha24 commented Sep 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

pujitha24 commented Aug 4, 2026 •

edited

Loading

codecov Bot commented Aug 11, 2026 •

edited

Loading