Skip to content

Add unit-tests for initc and improve in-line doc strings#204

Merged
renormalize merged 6 commits into
ai-dynamo:mainfrom
gflarity:gflarity/docs_initc_v2
Sep 29, 2025
Merged

Add unit-tests for initc and improve in-line doc strings#204
renormalize merged 6 commits into
ai-dynamo:mainfrom
gflarity:gflarity/docs_initc_v2

Conversation

@gflarity

@gflarity gflarity commented Sep 23, 2025

Copy link
Copy Markdown
Contributor

What type of PR is this?

/kind documentation
/kind tests

What this PR does / why we need it:

This PR is follow up to #193

It improves to documentation and tests for the initc directory. The prompt has been updated to avoid obvious/non-valuable comments and avoid duplication. Updated documentation prompt below. Please feel free to make suggestions to the prompt. Note the goal isn't to be perfect, just practical and useful.

I also fixed the bugs highlighted in the PR as the result of tests.

Special notes for your reviewer:

Documentation Prompt:

Improve the inline documentation for for all golang files under this directory/package. Here are the rules to follow: 

* Use concise idiomatic go documentation principles.  
* Ensure correctness. 
* Add some minimal inline documentation above complex code blocks when to explain the intent of the code. 
* Don't document code that is clearly self documenting. 
* Blocks of code should only have a comment describing them if it is practical and valuable. 
* Don't change the code or variable names. 
* All functions to should have concise accurate documentation with the exception of command "main" functions which are obviously entry points. 
* Document field structs but don't duplicate the field documentation above. 
* Avoid duplicate documentation, just pick the most appropriate place.
* Large or complex blocks of code that aren't self documenting should be documented. 


Example of documentation that isn't useful and should be avoided: 
// log is the global logger instance configured for the grove init container.
// It uses JSON format at INFO level for structured logging.
var (
 log = logger.MustNewLogger(false, configv1alpha1.InfoLevel, configv1alpha1.LogFormatJSON).WithName("grove-initc")
)

Examples of documentation that doesn't add value: 
   // Parse the replica count as an integer
    replicas, err := strconv.Atoi(nameAndMinAvailable[1])
    if err != nil {
     return nil, groveerr.WrapError(err, errCodeInvalidInput, operationParseFlag, "failed to convert replicas to int")
    }
  
  
    // Store the PodClique name and its minimum available replicas
    podCliqueDependencies[strings.TrimSpace(nameAndMinAvailable[0])] = replicas
  
  // Initialize the configuration with empty PodClique list
   config := CLIOptions{
    podCliques: make([]string, 0),
   }
  
   // Register all command line flags
   config.RegisterFlags(pflag.CommandLine)
   // Parse the command line arguments
   pflag.Parse()
  
   // Get the in-cluster REST configuration
   restConfig, err := rest.InClusterConfig()
  // Get the pod informer from the factory
  typedInformer := factory.Core().V1().Pods().Informer()

Systematically check every golang file under this directory is documented as per the instructions above. 

Test 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

Signed-off-by: Geoff Flarity <gflarity@nvidia.com>
Signed-off-by: Geoff Flarity <gflarity@nvidia.com>
Signed-off-by: Geoff Flarity <gflarity@nvidia.com>
Signed-off-by: Geoff Flarity <gflarity@nvidia.com>
@gflarity gflarity force-pushed the gflarity/docs_initc_v2 branch from 314bf01 to 873a07c Compare September 24, 2025 14:26
renormalize
renormalize previously approved these changes Sep 26, 2025

@renormalize renormalize left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the PR @gflarity!

I've added a test case for replica count as an empty string, and one for white space around the colon.

I've also renamed 2 generated test functions to follow Go conventions.

…tions in `wait_test.go`.

Signed-off-by: Saketh Kalaga <saketh.kalaga@sap.com>
renormalize
renormalize previously approved these changes Sep 26, 2025
@unmarshall unmarshall changed the title improve documentation for initc Add unit-tests for initc and improve in-line doc strings Sep 26, 2025
Signed-off-by: Saketh Kalaga <saketh.kalaga@sap.com>
@renormalize renormalize merged commit 0686148 into ai-dynamo:main Sep 29, 2025
4 checks passed
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