Skip to content

Conversation

@andyzhangx
Copy link
Collaborator

@andyzhangx andyzhangx commented Nov 7, 2025

Reason for Change:

test: add InferenceSet e2e test

Requirements

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

Issue Fixed:

Notes for Reviewers:

@kaito-pr-agent
Copy link

kaito-pr-agent bot commented Nov 7, 2025

Title

test: Add e2e tests for InferenceSet resource


Description

  • Added e2e tests for InferenceSet resource

  • Implemented helper functions for InferenceSet lifecycle

  • Added Phi-2 InferenceSet test case

  • Enhanced test utilities with InferenceSet support


Changes walkthrough 📝

Relevant files
Tests
preset_test.go
Added InferenceSet test helpers                                                   

test/e2e/preset_test.go

  • Added createAndValidateInferenceSet function
  • Implemented validateInferenceSetStatus helper
  • Added cleanupResourcesForInferenceSet function
  • Created deleteInferenceSet method
  • +78/-0   
    preset_vllm_test.go
    Implemented InferenceSet e2e test                                               

    test/e2e/preset_vllm_test.go

  • Added new e2e test for Phi-2 InferenceSet
  • Implemented createPhi2InferenceSetWithPresetPublicModeAndVLLM
  • Added InferenceSet status validation
  • +24/-0   
    utils.go
    Added InferenceSet manifest utilities                                       

    test/e2e/utils/utils.go

  • Added GenerateInferenceSetManifestWithVLLM
  • Created GenerateInferenceSetManifest helper
  • Implemented InferenceSet CRD generation utilities
  • +48/-0   

    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
    Copy link

    kaito-pr-agent bot commented Nov 7, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Hardcoded Replicas

    The GenerateInferenceSetManifest function sets Spec.Replicas to a fixed value of 1, ignoring the passed replicas parameter. This may cause test behavior to differ from intended replica counts.

    Replicas: 1,
    Potential Resource Leak

    The cleanupResourcesForInferenceSet function only deletes the InferenceSet but not its underlying Workspace resources. This could leave orphaned resources after test execution.

    func cleanupResourcesForInferenceSet(inferenceSetObj *kaitov1alpha1.InferenceSet) {
    	By("Cleaning up InferenceSet resources", func() {
    		if !CurrentSpecReport().Failed() {
    			// delete InferenceSet
    			err := deleteInferenceSet(inferenceSetObj)
    			Expect(err).NotTo(HaveOccurred(), "Failed to delete InferenceSet")
    		} else {
    			GinkgoWriter.Printf("test failed, keep %s \n", inferenceSetObj.Name)
    		}
    	})
    }

    @kaito-pr-agent
    Copy link

    kaito-pr-agent bot commented Nov 7, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Fix hardcoded replica count

    The Replicas field is hardcoded to 1, ignoring the replicas parameter passed to
    GenerateInferenceSetManifest. This prevents creating InferenceSets with different
    replica counts during testing. Update the code to use the function's replicas
    parameter instead of the hardcoded value.

    test/e2e/utils/utils.go [349]

     Spec: kaitov1alpha1.InferenceSetSpec{
    -    Replicas: 1,
    +    Replicas: replicas,
         Selector: labelSelector,
         Template: kaitov1alpha1.InferenceSetTemplate{
           ...
         }
     }
    Suggestion importance[1-10]: 10

    __

    Why: The hardcoded replica count of 1 would cause tests requiring multiple replicas to fail. Using the replicas parameter ensures the InferenceSet is created with the correct replica count as specified in test cases.

    High

    @andyzhangx andyzhangx changed the title [WIP] test: add InferenceSet e2e test test: add InferenceSet e2e test Nov 25, 2025
    })

    It("should create a Phi-2 InferenceSet with preset public mode successfully", utils.GinkgoLabelFastCheck, func() {
    numOfReplicas := 1
    Copy link
    Collaborator

    Choose a reason for hiding this comment

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

    You should create 2 replica and check if each deployment is ready.

    The test now don't cover the critial path of inferenceset.

    @Fei-Guo Fei-Guo merged commit 23ea347 into kaito-project:main Nov 27, 2025
    17 of 18 checks passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Projects

    Status: Done

    Development

    Successfully merging this pull request may close these issues.

    3 participants