test: add unit tests for StartPR #2298

theakshaypant · 2025-10-24T07:21:41Z

📝 Description of the Change

Add comprehensive unit test coverage for StartPR functionality in new pipelineascode_startpr_test.go file.

👨🏻‍ Linked Jira

https://issues.redhat.com/browse/SRVKP-9060

🔗 Linked GitHub Issue

N/A

🚀 Type of Change

🧪 Testing Strategy

🤖 AI Assistance

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

If you have used AI assistance, please provide the following details:

Which LLM was used?

Extent of AI Assistance:

Documentation and research only
Unit tests or E2E tests only
Code generation (parts of the code)
Full code generation (most of the PR)
PR description and comments
Commit message(s)

Important

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: Gemini [email protected]
Co-authored-by: ChatGPT [email protected]
Co-authored-by: Claude [email protected]
Co-authored-by: Cursor [email protected]
Co-authored-by: Copilot [email protected]

**💡You can use the script ./hack/add-llm-coauthor.sh to automatically add
these co-author trailers to your commits.

✅ Submitter Checklist

gemini-code-assist · 2025-10-24T07:22:01Z

Summary of Changes

Hello @theakshaypant, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly enhances the test coverage for the StartPR function within the pipelineascode package. By introducing a new, dedicated test file and a comprehensive suite of unit tests, it ensures the robustness and reliability of the PipelineRun initiation process. The tests validate behavior across various operational and error conditions, including concurrent executions, secret management, and status reporting, thereby improving the overall stability of the system.

Highlights

New Unit Test File: A new file, pipelineascode_startpr_test.go, has been added to house comprehensive unit tests specifically for the StartPR functionality.
Comprehensive Test Coverage: The new tests cover a wide array of scenarios, including successful PipelineRun creation, secret auto-creation (success, failure, and already exists cases), handling of concurrency limits (pending vs. immediate start), propagation of GitLab project IDs, and various error conditions during PipelineRun creation, secret ownerRef updates, and status reporting.
Concurrency Testing: Dedicated tests have been added to validate the behavior of StartPR under concurrent calls, ensuring proper isolation when creating multiple PipelineRuns with unique secrets, and graceful handling of race conditions when multiple calls attempt to create the same shared secret.
Refactoring of Test Utilities: The KinterfaceTestWithError struct and its CreateSecret method have been moved from pipelineascode_test.go to the new pipelineascode_startpr_test.go file, centralizing test utilities relevant to StartPR.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This pull request introduces a comprehensive suite of unit tests for the startPR function in a new pipelineascode_startpr_test.go file. The tests are well-structured, covering a wide range of success, failure, and concurrency scenarios, which significantly improves the robustness and maintainability of the startPR logic. The changes also include refactoring some test helper code into the new file, which is a good cleanup. I've found a couple of minor issues in a test setup that could be improved for clarity.

pkg/pipelineascode/pipelineascode_startpr_test.go

chmouel · 2025-10-24T08:01:51Z

a lot of those fakes are already provided in pkg/test/ and i believe a lot of those unittests are covered in https://github.com/openshift-pipelines/pipelines-as-code/blob/6c646e453/pkg/pipelineascode/pipelineascode_test.go isnt it ?

theakshaypant · 2025-10-24T08:48:00Z

a lot of those fakes are already provided in pkg/test/ and i believe a lot of those unittests are covered in https://github.com/openshift-pipelines/pipelines-as-code/blob/6c646e453/pkg/pipelineascode/pipelineascode_test.go isnt it ?

Wanted to put this in draft. Haven't cleaned up the redundant tests yet.
Will also look at the fakes and make changes accordingly.

theakshaypant · 2025-10-27T06:41:55Z

/gemini review

zakisk · 2025-10-27T06:43:07Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces a comprehensive suite of unit tests for the startPR function in a new pipelineascode_startpr_test.go file. This is an excellent addition that significantly improves test coverage for this critical functionality. The changes also include refactoring the existing pipelineascode_test.go to remove tests that are now covered in the new file, which is a good cleanup.

My review focuses on improving the maintainability and clarity of the newly added tests. I've identified a few areas where the tests can be made more readable and robust, such as reducing repetitive setup code, clarifying the intent of some tests, and simplifying complex test logic. Overall, this is a high-quality contribution.

pkg/pipelineascode/pipelineascode_startpr_test.go

gemini-code-assist

Code Review

This pull request introduces a comprehensive suite of unit tests for the startPR function, significantly improving test coverage and code reliability. The refactoring of existing tests into a new, dedicated file (pipelineascode_startpr_test.go) is a great organizational improvement. The tests are well-structured, covering a wide range of success paths, error conditions, and concurrency scenarios. I've included a few suggestions to further enhance the new test suite by reducing redundancy and improving clarity. Overall, this is a high-quality contribution.

pkg/pipelineascode/pipelineascode_startpr_test.go

theakshaypant · 2025-10-27T08:53:18Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces a new test file, pipelineascode_startpr_test.go, to provide comprehensive unit test coverage for the StartPR functionality. The changes also include modifications to pipelineascode_test.go and kubernetesint.go to support the new tests. I have identified some areas where improvements can be made to enhance the code's robustness and clarity.

pkg/pipelineascode/pipelineascode_startpr_test.go

gemini-code-assist · 2025-10-27T08:55:33Z

pkg/pipelineascode/pipelineascode_startpr_test.go

+func (k *KinterfaceTestWithSecretTracking) CreateSecret(ctx context.Context, ns string, secret *corev1.Secret) error {
+	// Atomically increment and get the new count
+	count := k.secretCreationCount.Add(1)
+
+	// First creation succeeds, subsequent ones return AlreadyExists to simulate race condition
+	if count > 1 {
+		return errors.NewAlreadyExists(schema.GroupResource{Group: "", Resource: "secrets"}, secret.GetName())
+	}


Consider adding a more descriptive error message when count > 1 to provide better context for debugging. For example, include the secret name in the error message.

return errors.NewAlreadyExists(schema.GroupResource{Group: "", Resource: "secrets"}, secret.GetName())

zakisk · 2025-10-27T10:48:27Z

I see that you have created multiple funcs for every scenario can you please make them in one func like we usually do in PaC

Add comprehensive unit test coverage for StartPR functionality in new pipelineascode_startpr_test.go file. Signed-off-by: Akshay Pant <[email protected]> Co-authored-by: Claude-4.5-Sonnet (via Cursor)

chmouel · 2025-10-28T10:08:47Z

pkg/pipelineascode/pipelineascode_startpr_test.go

+		test func(t *testing.T)
+	}{
+		{
+			name: "success - comprehensive success test covering the full startPR flow",


This is a very weird way to do testcases, you redoing the testing constructions all the time.. it's not supposed to work like that

chmouel · 2025-10-29T08:08:10Z

This look a bit vibe coded, which is fine, no problem with that... but you may need to be careful to make sure it output the code the same way as what we have before.. (which is again fine, you just need to know how the project structure works to be able to drive the LLM to output things as expected)

theakshaypant · 2025-10-29T08:37:10Z

This look a bit vibe coded, which is fine, no problem with that... but you may need to be careful to make sure it output the code the same way as what we have before.. (which is again fine, you just need to know how the project structure works to be able to drive the LLM to output things as expected)

Yes, tried to restructure the tests into a single one using Cursor. Have gone through the individual cases in detail though.
Will make changes to the file to conform to the existing test structure in the repo.

theakshaypant · 2025-10-30T11:21:51Z

@chmouel @zakisk How do we feel about going back to a previous version of these tests where we have multiple test funcs? Given the amount of functionality covered in startPR, I feel it would be a bit hard to consolidate all the scenarios into a single table driven test func.

For your reference, the test funcs earlier were the following with fairly different assertion and configuration involved.

TestStartPR - Basic success case
TestStartPR_MissingSecretAnnotation
TestStartPR_CreationFailureWithSecretCleanup
TestStartPR_SecretCleanupError
TestStartPR_StatusCreationFailure
TestStartPR_GitHubAppLogURLHandling
TestStartPR_LoggingWithConditions
TestStartPR_MultipleCustomAnnotations
TestStartPR_ConcurrentCreation
TestStartPR_ConcurrentWithSameSecret
TestStartPR_SecretCreationScenarios - 3 subtests
TestStartPR_AnnotationAndLabelPropagation - 3 subtests
TestStartPR_ConcurrencyLimitBehavior - 5 subtests
TestStartPR_SecretOwnerRefUpdateErrors - 2 subtests
TestStartPR_StateManagement - 2 subtests
TestStartPR_PatchBehavior - 2 subtests

chmouel · 2025-10-31T13:50:41Z

I am okay with that, it's ncie to have testcase but if the test construtions cannot be the same across all tests then it defeat to use testcase... i am guessing all LLMs failed to do it properly?

theakshaypant · 2025-10-31T16:30:33Z

I am okay with that, it's ncie to have testcase but if the test construtions cannot be the same across all tests then it defeat to use testcase... i am guessing all LLMs failed to do it properly?

Yes, the current code is the best version that was generated.

pipelines-as-code bot added enhancement New feature or request testing labels Oct 24, 2025

gemini-code-assist bot reviewed Oct 24, 2025

View reviewed changes

pkg/pipelineascode/pipelineascode_startpr_test.go Outdated Show resolved Hide resolved

pkg/pipelineascode/pipelineascode_startpr_test.go Outdated Show resolved Hide resolved

theakshaypant marked this pull request as draft October 24, 2025 08:46

theakshaypant force-pushed the SRVKP-9060-unit-tests-startpr branch from 0c0a38b to c2eb052 Compare October 27, 2025 06:32

pipelines-as-code bot added the go Pull requests that update Go code label Oct 27, 2025

gemini-code-assist bot reviewed Oct 27, 2025

View reviewed changes

pkg/pipelineascode/pipelineascode_startpr_test.go Outdated Show resolved Hide resolved

pkg/pipelineascode/pipelineascode_startpr_test.go Outdated Show resolved Hide resolved

pkg/pipelineascode/pipelineascode_startpr_test.go Outdated Show resolved Hide resolved

gemini-code-assist bot reviewed Oct 27, 2025

View reviewed changes

pkg/pipelineascode/pipelineascode_startpr_test.go Outdated Show resolved Hide resolved

pkg/pipelineascode/pipelineascode_startpr_test.go Outdated Show resolved Hide resolved

pkg/pipelineascode/pipelineascode_startpr_test.go Outdated Show resolved Hide resolved

theakshaypant force-pushed the SRVKP-9060-unit-tests-startpr branch 2 times, most recently from 0d61aee to 72dc237 Compare October 27, 2025 07:39

gemini-code-assist bot reviewed Oct 27, 2025

View reviewed changes

theakshaypant force-pushed the SRVKP-9060-unit-tests-startpr branch from 72dc237 to 3d3528c Compare October 27, 2025 09:09

theakshaypant marked this pull request as ready for review October 27, 2025 11:03

test: add unit tests for StartPR

554009e

Add comprehensive unit test coverage for StartPR functionality in new pipelineascode_startpr_test.go file. Signed-off-by: Akshay Pant <[email protected]> Co-authored-by: Claude-4.5-Sonnet (via Cursor)

theakshaypant force-pushed the SRVKP-9060-unit-tests-startpr branch from 3d3528c to 554009e Compare October 28, 2025 04:27

chmouel reviewed Oct 28, 2025

View reviewed changes

test: add unit tests for StartPR #2298

Are you sure you want to change the base?

test: add unit tests for StartPR #2298

Uh oh!

Conversation

theakshaypant commented Oct 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📝 Description of the Change

👨🏻‍ Linked Jira

🔗 Linked GitHub Issue

🚀 Type of Change

🧪 Testing Strategy

🤖 AI Assistance

✅ Submitter Checklist

Uh oh!

gemini-code-assist bot commented Oct 24, 2025

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

chmouel commented Oct 24, 2025

Uh oh!

theakshaypant commented Oct 24, 2025

Uh oh!

theakshaypant commented Oct 27, 2025

Uh oh!

zakisk commented Oct 27, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

theakshaypant commented Oct 27, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

gemini-code-assist bot Oct 27, 2025

Choose a reason for hiding this comment

Uh oh!

zakisk commented Oct 27, 2025

Uh oh!

chmouel Oct 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chmouel commented Oct 29, 2025

Uh oh!

theakshaypant commented Oct 29, 2025

Uh oh!

theakshaypant commented Oct 30, 2025

Uh oh!

chmouel commented Oct 31, 2025

Uh oh!

theakshaypant commented Oct 31, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

theakshaypant commented Oct 24, 2025 •

edited

Loading

chmouel Oct 28, 2025 •

edited

Loading