Skip to content

Conversation

@JAORMX
Copy link
Collaborator

@JAORMX JAORMX commented Oct 1, 2025

This PR fixes the staticcheck linter errors that were blocking CI:

Error: pkg/runner/retriever/retriever_test.go:39:5: SA5011(related information): this check suggests that the pointer can be nil (staticcheck)
Error: pkg/runner/retriever/retriever_test.go:45:15: SA5011: possible nil pointer dereference (staticcheck)

What was the problem?

The test had two separate if statements checking for nil conditions:

  1. First checking if testGroupName is empty
  2. Then checking if group is nil

Both checks call t.Skip() to exit the test early. However, the staticcheck linter couldn't understand that t.Skip() terminates execution, so it thought the code could continue and potentially dereference a nil group pointer later in the test.

How did we fix it?

We combined both checks into a single if statement using the OR operator (||). This makes it crystal clear to the linter that if group is nil, the test will skip before any code tries to access group.Servers.

The test behavior is exactly the same - it still skips when there are no groups or when the group is nil. We just made the logic clearer for the static analyzer.

Verification

  • ✅ Staticcheck now passes with zero SA5011 errors
  • ✅ The test still works correctly and skips appropriately
  • ✅ No functional changes to the test logic

Combine two separate nil checks into a single condition to eliminate
staticcheck SA5011 warnings about possible nil pointer dereference.

The previous code had two sequential checks:
1. if testGroupName == "" { t.Skip(...) }
2. if group == nil { t.Skip(...) }

Staticcheck couldn't determine that t.Skip() exits the test, so it
flagged the later dereference of group.Servers as potentially unsafe.

By combining these into a single condition (testGroupName == "" ||
group == nil), the linter can clearly see that group cannot be nil
when execution continues past the check.

This maintains the same test behavior while satisfying the linter.

Signed-off-by: Juan Antonio Osorio <ozz@stacklok.com>
@codecov
Copy link

codecov bot commented Oct 1, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 48.20%. Comparing base (be1e1a5) to head (78ce5d2).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2050      +/-   ##
==========================================
+ Coverage   48.13%   48.20%   +0.06%     
==========================================
  Files         233      233              
  Lines       29240    29240              
==========================================
+ Hits        14075    14095      +20     
+ Misses      14133    14110      -23     
- Partials     1032     1035       +3     

☔ View full report in Codecov by Sentry.
📢 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.

@JAORMX JAORMX requested review from blkt, dmjb, rdimitrov and yrobla October 1, 2025 12:08
@JAORMX JAORMX merged commit a8b8d38 into main Oct 1, 2025
26 checks passed
@JAORMX JAORMX deleted the fix-staticcheck-nil-pointer branch October 1, 2025 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants