Skip to content

Conversation

@gflarity
Copy link
Contributor

@gflarity gflarity commented Nov 6, 2025

What type of PR is this?

Testing

What this PR does / why we need it:

Increased test coverage for internal/controller

Special notes for your reviewer:

Prompt:

Please create an idiomatic golang unit tests for this specific directory/package. Herea are the instructions:

  • Start by revieiwng the state of test coverage with golang tooling.
  • Be sure to document the fields in the anonymous test structs and how they'll be used below.
  • Each Test* function should also have some concise documentation as well. Each test case should have a brief explanation and expectation explaine. This should be right before the go struct definition for that test if it's table driven tests.
  • Each test should have a name field for cross referencing failures.
  • Rather than having a description field, the description should just be in comments above the name in the struct declaration.
  • Don't forget to run the tests and confirm they're working, but focus on just testing this package as others might have issues.
  • Please avoid creating skipped tests.
  • Refactor code to make it more testable if necessary. However keep these refactors to a minimum and as simple as possible.
  • Specifically hardcoded constants etc are good candidates to be refactor out of testable code. As are interface that can be mocked in a straight forward way. However you must remember to ensure backwards compatibility and new functions rather than changing the signature of public functions. Avoid making these test helpers public though. Only make such refactors when there's a good return on investment.
  • Please ensure coverage is as good as possible using golang tooling.
  • Avoid creating table driven tests when the test function just has a single test in it.
  • Avoid test that don't really test anything.
  • Avoid describing tests as edge cases.
  • Avoid tests for command line arguments. Try to keep test cases unique and adding value (avoid duplicate test cases).

Does this PR introduce a API change?

None

Copy link

@shayasoolin shayasoolin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

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.

2 participants