fix: Make error handling and tests more robust #2207

chmouel · 2025-08-06T08:32:05Z

Added nil checks to prevent panics from dereferencing null pointers
Improved error handling logic and messages in various components
Enhanced test suite robustness by adding nil checks to assertions
Migrated Gitea ACL tests to use golden files for improved maintainability
Adjusted golden file update script to target specific packages
use the new nilness analyzer from goland to list them and ask gemini/cursor to fix it

Co-Authored-By: gemini, cursor

📝 Description of the Change

🔗 Linked GitHub Issue

Fixes #

👨🏻‍ Linked Jira

🚀 Type of Change

🧪 Testing Strategy

✅ Submitter Checklist

Copilot

Pull Request Overview

This PR improves error handling and test robustness across the codebase by adding nil checks to prevent panics, enhancing error messages, and migrating Gitea ACL tests to use golden files for better maintainability.

Added nil checks to prevent panics from dereferencing null pointers in various components
Enhanced error handling logic and improved error messages across test files and providers
Migrated Gitea ACL tests to use golden files and adjusted the golden file update script

Reviewed Changes

Copilot reviewed 33 out of 33 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
`test/repository_webhook_test.go`	Added nil check before accessing error message in test assertion
`test/pkg/github/setup.go`	Improved error handling for repository owner string splitting
`test/pkg/gitea/scm.go`	Added nil checks for team creation response and moved assertion order
`test/pkg/bitbucketdatacenter/pr.go`	Enhanced error message formatting with nil response checks
`test/pkg/bitbucketdatacenter/crd.go`	Improved error handling with null response protection
`test/github_config_maxkeepruns_test.go`	Added nil check for PipelineRun list response
`test/bitbucket_datacenter_*.go`	Enhanced error message formatting with nil response checks
`pkg/provider/gitlab/acl*.go`	Improved error handling for OWNERS_ALIASES file retrieval
`pkg/provider/github/*.go`	Added nil checks in test assertions and response handling
`pkg/provider/gitea/acl*.go`	Migrated ACL tests to golden files and improved error handling
`pkg/pipelineascode/pipelineascode.go`	Added nil check before accessing PipelineRun name
`pkg/matcher/*.go`	Improved test assertions and moved operations after error checks
`pkg/adapter/adapter.go`	Separated error and nil provider checks for better error reporting
`hack/update-golden.sh`	Restricted golden file updates to pkg directory only

Copilot · 2025-08-06T08:32:40Z

pkg/provider/github/github_test.go

+				assert.Assert(t, err != nil, "we should have get an error here")
+				if err != nil {
+					assert.Assert(t, strings.Contains(err.Error(), tt.wantErrStr), err.Error(), tt.wantErrStr)


This assertion is duplicated on line 478 and 479. Remove the duplicate assertion.

Suggested change

assert.Assert(t, err != nil, "we should have get an error here")

if err != nil {

assert.Assert(t, strings.Contains(err.Error(), tt.wantErrStr), err.Error(), tt.wantErrStr)

if err != nil {

assert.Assert(t, strings.Contains(err.Error(), tt.wantErrStr), err.Error(), tt.wantErrStr)

assert.Assert(t, strings.Contains(err.Error(), tt.wantErrStr), err.Error(), tt.wantErrStr)

gemini-code-assist

Summary of Changes

Hello @chmouel, 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!

I have focused on enhancing the robustness of the codebase by improving error handling mechanisms and strengthening the test suite. This includes adding explicit nil checks to prevent panics, refining error messages, and making test assertions more resilient. A significant change involves migrating Gitea ACL tests to use golden files for better maintainability.

Highlights

Improved Nil Checks: Added explicit nil checks in pkg/adapter/adapter.go and pkg/pipelineascode/pipelineascode.go to prevent panics when dereferencing potentially null pointers, particularly for gitProvider and PipelineRun objects.
Enhanced Test Robustness: Refactored numerous test files (e.g., pkg/cli/webhook/*_test.go, pkg/provider/bitbucketdatacenter/bitbucketdatacenter_test.go, pkg/provider/github/github_test.go) to include if err != nil checks before asserting error messages, ensuring tests handle nil errors gracefully.
Golden File Migration for Gitea ACL Tests: Migrated the CheckPolicyAllowing tests in pkg/provider/gitea/acl_test.go to utilize golden files, improving the maintainability and readability of test expectations.
Refined Error Handling Logic: Reordered error checks in pkg/matcher/repo_runinfo_matcher.go and pkg/provider/gitea/acl.go to ensure errors are handled before subsequent operations, and improved OWNERS_ALIASES file handling in pkg/provider/gitlab/acl.go to better manage "not found" scenarios.
Test Setup and Logging Improvements: Enhanced error messages in test helper functions (test/pkg/bitbucketdatacenter/crd.go, test/pkg/bitbucketdatacenter/pr.go) to provide more context (e.g., HTTP status codes) when creating branches or repositories.

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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

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 issue 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 or fill out our survey to provide feedback.

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. ↩

chmouel · 2025-08-06T08:32:46Z

hack/update-golden.sh

@@ -2,4 +2,4 @@
 # This will run `go test a/package -test.update-golden=true` on all packages that are importing `gotest.tools/v3/golden`
 # This will update the golden files with the current output.
 # Run this only when you are sure the output is meant to change.
-go test $(go list -f '{{ .ImportPath }} {{ .TestImports }}' ./... | grep gotest.tools/v3/golden | awk '{print $1}' | tr '\n' ' ') -test.update-golden=true


make update-golden don't work on macos anymore but that fix it and it's safe to do (we don't do golden update in cmd/ or test/

gemini-code-assist

Code Review

This pull request significantly improves the robustness of the codebase by adding numerous nil checks to prevent panics, particularly in error handling paths and tests. The changes are well-executed and enhance code stability. The migration of Gitea ACL tests to use golden files is a commendable improvement for maintainability. I've identified a few instances of redundant nil checks in the tests that could be simplified for better clarity.

gemini-code-assist · 2025-08-06T08:34:51Z

pkg/pipelineascode/errors_test.go

+					if err != nil { // Add this check
+						assert.Assert(t, err.Error() != "", "Expected error message but got empty string")
+					}


The assert.Assert(t, err != nil, ...) on line 102 already ensures that err is not nil for the subsequent code in this block, as it would cause the test to fail and stop. The added if err != nil check is therefore redundant and can be removed.

assert.Assert(t, err.Error() != "", "Expected error message but got empty string")

gemini-code-assist · 2025-08-06T08:34:51Z

pkg/provider/gitea/detect_test.go

+				if err != nil {
+					assert.Assert(t, strings.Contains(err.Error(), tt.wantErrSubstr), err.Error(), "doesn't have", tt.wantErrSubstr)
+				}


The assert.Assert(t, err != nil) on line 200 already ensures that err is not nil for the subsequent code in this block, as it would cause the test to fail and stop. The added if err != nil check is therefore redundant and can be removed.

assert.Assert(t, strings.Contains(err.Error(), tt.wantErrSubstr), err.Error(), "doesn't have", tt.wantErrSubstr)

gemini-code-assist · 2025-08-06T08:34:52Z

pkg/provider/github/github_test.go

+				assert.Assert(t, err != nil, "we should have get an error here")
+				if err != nil {
+					assert.Assert(t, strings.Contains(err.Error(), tt.wantErrStr), err.Error(), tt.wantErrStr)
+				}


This assert.Assert call is a duplicate of the one on the preceding line. Additionally, the if err != nil check is redundant because the assertion on line 478 already ensures err is not nil by failing the test if it is. Please remove the duplicated assertion and the redundant if block.

Suggested change

assert.Assert(t, err != nil, "we should have get an error here")

if err != nil {

assert.Assert(t, strings.Contains(err.Error(), tt.wantErrStr), err.Error(), tt.wantErrStr)

}

assert.Assert(t, strings.Contains(err.Error(), tt.wantErrStr), err.Error(), tt.wantErrStr)

gemini-code-assist · 2025-08-06T08:34:52Z

test/repository_webhook_test.go

+	if err != nil {
+		assert.Equal(t, err.Error(), "admission webhook \"validation.pipelinesascode.tekton.dev\" denied the request: repository already exists with URL: https://pac.test/pac/app")
+	}


The assert.Assert(t, err != nil) on line 46 already ensures that err is not nil for the subsequent code in this block, as it would cause the test to fail and stop. The added if err != nil check is therefore redundant and can be removed.

assert.Equal(t, err.Error(), "admission webhook \"validation.pipelinesascode.tekton.dev\" denied the request: repository already exists with URL: https://pac.test/pac/app")

- Added nil checks to prevent panics from dereferencing null pointers - Improved error handling logic and messages in various components - Enhanced test suite robustness by adding nil checks to assertions - Migrated Gitea ACL tests to use golden files for improved maintainability - Adjusted golden file update script to target specific packages Signed-off-by: Chmouel Boudjnah <[email protected]>

Copilot AI review requested due to automatic review settings August 6, 2025 08:32

Copilot AI reviewed Aug 6, 2025

View reviewed changes

gemini-code-assist bot reviewed Aug 6, 2025

View reviewed changes

chmouel commented Aug 6, 2025

View reviewed changes

gemini-code-assist bot reviewed Aug 6, 2025

View reviewed changes

chmouel force-pushed the nilness-check branch from a95da5b to 0b3a2e4 Compare August 11, 2025 04:23

Merge branch 'main' into nilness-check

d1b9a0d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: Make error handling and tests more robust #2207

fix: Make error handling and tests more robust #2207

Uh oh!

chmouel commented Aug 6, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Aug 6, 2025

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

chmouel Aug 6, 2025

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

gemini-code-assist bot Aug 6, 2025

Uh oh!

gemini-code-assist bot Aug 6, 2025

Uh oh!

gemini-code-assist bot Aug 6, 2025

Uh oh!

gemini-code-assist bot Aug 6, 2025

Uh oh!

Uh oh!

fix: Make error handling and tests more robust #2207

Are you sure you want to change the base?

fix: Make error handling and tests more robust #2207

Uh oh!

Conversation

chmouel commented Aug 6, 2025

📝 Description of the Change

🔗 Linked GitHub Issue

👨🏻‍ Linked Jira

🚀 Type of Change

🧪 Testing Strategy

✅ Submitter Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Aug 6, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Summary of Changes

Highlights

Footnotes

Uh oh!

chmouel Aug 6, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Aug 6, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Aug 6, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Aug 6, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Aug 6, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!