Skip to content

chore: rename Go files to use underscore file naming convention #1361

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Aug 6, 2025
Merged

chore: rename Go files to use underscore file naming convention #1361

merged 1 commit into from
Aug 6, 2025

Conversation

Jont828
Copy link
Contributor

@Jont828 Jont828 commented Aug 5, 2025

Reason for Change:

Requirements

  • added unit tests and e2e tests (if applicable).

Issue Fixed:

Notes for Reviewers:

@kaito-pr-agent Kaito PR Agent
Copy link

kaito-pr-agent bot commented Aug 5, 2025

Title

Add extensive test cases and utilities for preset inference and tuning

Description

  • Added comprehensive test cases for preset inference generation

  • Introduced utility functions for testing Kubernetes resources

  • Enhanced preset tuning with detailed test scenarios

  • Defined default constants and variables for inference and tuning

Changes walkthrough 📝

Relevant files
Tests
4 files
preset_inferences_test.go
Added extensive test cases for preset inference                   
[link]   
test_utils.go
Introduced utility functions for testing Kubernetes resources
 [link]   
preset_tuning_test.go
Enhanced preset tuning with detailed test scenarios           
[link]   
mock_client.go
Created mock client for Kubernetes testing                             
[link]   
Enhancement
2 files
preset_inference_types.go
Defined default constants and variables for inference       
[link]   
preset_tuning_types.go
Defined default constants and variables for tuning             
[link]   
Additional files
3 files
test_model.go [link]   
preset_inferences.go [link]   
preset_tuning.go [link]   
Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
    		•

    @kaito-pr-agent Kaito PR Agent
    Copy link

    kaito-pr-agent bot commented Aug 5, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Check pullerContainer nil

    Add a check to ensure pullerContainer is not nil before comparing its Command field
    to prevent potential nil pointer dereference.

    pkg/workspace/inference/preset_inferences_test.go [338-340]

    -if !reflect.DeepEqual(pullerContainer.Command, expectedPullerCmd) {
+if pullerContainer == nil {
+    t.Errorf("%s: Puller container is nil", k)
+} else if !reflect.DeepEqual(pullerContainer.Command, expectedPullerCmd) {
     t.Errorf("%s: Puller command is not expected, got %v, expect %v", k, pullerContainer.Command, expectedPullerCmd)
 }
    Suggestion importance[1-10]: 8

    __

    Why: Adding a nil check for pullerContainer prevents a potential nil pointer dereference, which is a critical issue that could cause the test to crash.

    		Medium
    Replace panics with errors

    Implement the unimplemented methods to provide full functionality.

    pkg/utils/test/mock_client.go [166]

    -panic("unimplemented")
+return nil, fmt.Errorf("method not implemented")
    Suggestion importance[1-10]: 3

    __

    Why: Replacing panics with errors allows the program to handle the situation more gracefully.

    		Low
    General
    Verify DefaultWeightsVolumePath

    Ensure that utils.DefaultWeightsVolumePath is correctly defined and accessible in
    the test environment to avoid runtime errors.

    pkg/workspace/inference/preset_inferences_test.go [329-334]

    +expectedPullerCmd := []string{
+    "oras",
+    "pull",
+    tc.expectedModelImage,
+    "-o",
+    utils.DefaultWeightsVolumePath,
+}
 
-
    Suggestion importance[1-10]: 6

    __

    Why: Ensuring utils.DefaultWeightsVolumePath is correctly defined and accessible is important for preventing runtime errors, but it doesn't address a critical issue.

    		Low
    Make gpuNodeCount configurable

    Consider making gpuNodeCount a configurable parameter or a function return value to
    allow flexibility in testing different scenarios.

    pkg/utils/test/test_utils.go [136]

    -gpuNodeCount = 1
+func GetGPUNodeCount() int {
+    return 1
+}
    Suggestion importance[1-10]: 5

    __

    Why: Making gpuNodeCount configurable can improve test flexibility, but it doesn't address a critical issue or bug.

    		Low
    Use dynamic hash generation

    Replace hardcoded hash values with dynamically generated hashes based on the content
    of the objects to ensure consistency and reduce maintenance overhead.

    pkg/utils/test/test_utils.go [250]

    -MockWorkspaceWithPresetHash = "89ae127050ec264a5ce84db48ef7226574cdf1299e6bd27fe90b927e34cc8adb"
+MockWorkspaceWithPresetHash = generateHash(MockWorkspaceWithPreset)
    Suggestion importance[1-10]: 5

    __

    Why: Using dynamic hash generation can reduce maintenance overhead, but it doesn't address a critical issue or bug.

    		Low
    Improve error messages in assertions

    Consider adding more detailed error messages for better debugging.

    pkg/workspace/tuning/preset_tuning_test.go [151-153]

     if tc.expectedError {
-  assert.Error(t, err)
+  assert.Error(t, err, "Expected error but got none")
   return
 }
-assert.NoError(t, err)
+assert.NoError(t, err, "Expected no error but got: %v", err)
    Suggestion importance[1-10]: 5

    __

    Why: Adding detailed error messages can help in debugging test failures more effectively.

    		Low
    Log error when volume mount is missing

    Handle the case where the results volume mount is not found more gracefully.

    pkg/workspace/tuning/preset_tuning.go [323-325]

     if resultVolumeMount == nil {
+  klog.ErrorS(fmt.Errorf("results volume mount not found in pod spec"), "Failed to find results volume mount")
   return fmt.Errorf("results volume mount not found in pod spec")
 }
    Suggestion importance[1-10]: 5

    __

    Why: Logging the error provides more context and helps in diagnosing issues.

    		Low
    Validate probes before setting

    Ensure that the container name matches the workspace name before modifying probes
    and environment variables.

    pkg/workspace/inference/preset_inferences.go [265-271]

     if spec.Containers[i].Name == ctx.Workspace.Name {
-  spec.Containers[i].LivenessProbe = livenessProbe
-  spec.Containers[i].ReadinessProbe = readinessProbe
+  if spec.Containers[i].LivenessProbe == nil {
+    spec.Containers[i].LivenessProbe = livenessProbe
+  }
+  if spec.Containers[i].ReadinessProbe == nil {
+    spec.Containers[i].ReadinessProbe = readinessProbe
+  }
   spec.Containers[i].Env = append(spec.Containers[i].Env, envVar)
   break
 }
    Suggestion importance[1-10]: 4

    __

    Why: Ensuring probes are not overwritten unnecessarily can prevent unintended behavior.

    		Low

    @zhuangqh
    Copy link
    Collaborator

    zhuangqh commented Aug 6, 2025

    Thanks for that. 😄

    @zhuangqh zhuangqh merged commit 5acd3c7 into kaito-project:main Aug 6, 2025
    13 of 14 checks passed
    farooqameen pushed a commit to farooqameen/kaito that referenced this pull request Aug 7, 2025
    @Jont828 @farooqameen
    chore: rename Go files to use underscore file naming convention (kait…
    0a874f2 
    …o-project#1361)
    farooqameen pushed a commit to farooqameen/kaito that referenced this pull request Aug 7, 2025
    @Jont828 @farooqameen
    chore: rename Go files to use underscore file naming convention (kait…
    5742028 
    …o-project#1361)
    @Jont828 Jont828 mentioned this pull request Aug 13, 2025
    Merged
    1 task
    chewong pushed a commit that referenced this pull request Aug 13, 2025
    @Jont828
    chore: rename files to use underscore instead of dashes (#1389)
    026be28 
    **Reason for Change**:
<!-- What does this PR improve or fix in KAITO? Why is it needed? -->

Missed a few files in #1361 

**Requirements**

- [ ] added unit tests and e2e tests (if applicable).

**Issue Fixed**:
<!-- If this PR fixes GitHub issue 4321, add "Fixes #4321" to the next
line. -->

**Notes for Reviewers**:
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Reviewers

    @chewong chewong chewong approved these changes

    @Fei-Guo Fei-Guo Awaiting requested review from Fei-Guo Fei-Guo is a code owner

    @zhuangqh zhuangqh Awaiting requested review from zhuangqh zhuangqh is a code owner

    Assignees
    No one assigned
    Labels
    None yet
    Projects
    KAITO Roadmap
    Status: Done
    Milestone
    No milestone
    Development

    Successfully merging this pull request may close these issues.
    3 participants
    @Jont828 @zhuangqh @chewong