Skip to content

Conversation

aeft
Copy link
Contributor

@aeft aeft commented Sep 5, 2025

What type of PR is this?

feat(classifier): add unit tests

What this PR does / why we need it:
Add Ginkgo test framework for the classifier component with proper dependency injection pattern.

Which issue(s) this PR fixes:

Fixes #45

Release Notes: No

Copy link

netlify bot commented Sep 5, 2025

Deploy Preview for vllm-semantic-router ready!

Name Link
🔨 Latest commit 15a7437
🔍 Latest deploy log https://app.netlify.com/projects/vllm-semantic-router/deploys/68bb47c85863da0008f8ca61
😎 Deploy Preview https://deploy-preview-57--vllm-semantic-router.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

Copy link

github-actions bot commented Sep 5, 2025

👥 vLLM Semantic Team Notification

The following members have been identified for the changed files in this PR and have been automatically assigned:

📁 src

Owners: @rootfs, @Xunzhuo, @wangchen615
Files changed:

  • src/semantic-router/pkg/utils/classification/classifier_test.go
  • src/semantic-router/pkg/utils/classification/classifier.go

This comment was automatically generated based on the OWNER files in the repository.

@rootfs
Copy link
Collaborator

rootfs commented Sep 5, 2025

@aeft thanks for contributing! can you sign DCO and run pre-commit run --all-files?

Ensure you have a local copy of your branch by [checking out the pull request locally via command line](https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/checking-out-pull-requests-locally).
In your local branch, run: git rebase HEAD~1 --signoff
Force push your changes to overwrite the branch: git push --force-with-lease origin feature/classifier-test-framework

@aeft aeft force-pushed the feature/classifier-test-framework branch from 00acea6 to 41213b1 Compare September 5, 2025 18:36
Signed-off-by: Alex Wang <[email protected]>
@aeft aeft force-pushed the feature/classifier-test-framework branch from 41213b1 to 69e8e92 Compare September 5, 2025 18:42
@@ -0,0 +1,408 @@
package classification
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you toggle this test through golang tags? So we can run both mock and real inference in CI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean I should add something like

//go:build !integration  
// +build !integration  

so that we can toggle these tests with -tags=integration when we want to run real inference?
If that’s the case, would you prefer the tag name to be integration, or should I use another name that matches our project’s convention?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, something like. You can choose a tag like mock, bert, etc that explains the nature of the tests

Signed-off-by: Alex Wang <[email protected]>
@aeft aeft force-pushed the feature/classifier-test-framework branch from d8efbb7 to 39911f4 Compare September 5, 2025 19:49
@aeft aeft marked this pull request as ready for review September 5, 2025 20:14
@aeft
Copy link
Contributor Author

aeft commented Sep 5, 2025

Hi. This PR is ready for review. The unit tests have passed, and I also tested it with the real model (using make run-router and curl for the corresponding API).

@aeft
Copy link
Contributor Author

aeft commented Sep 5, 2025

If this PR looks good, my next plan is to improve the unit tests for the remaining methods in classifier.go.

@rootfs rootfs merged commit ea956c0 into vllm-project:main Sep 5, 2025
9 checks passed
@rootfs
Copy link
Collaborator

rootfs commented Sep 5, 2025

@aeft sounds good! when you have additional tests, please use tags so we know which tests are tested in what condition. Thanks

@aeft
Copy link
Contributor Author

aeft commented Sep 5, 2025

@rootfs Thanks for your review!

I want to confirm whether this aligns with your expectations (since this convention doesn't seem to be established in the project yet, I wanted to check with you. Thanks):

We can add //go:build !integration to mock-based tests (they are unit tests, quick and no dependency) to indicate that they should not run during integration tests (go test -tags=integration ./...). These tests will run when no tags are specified (go test ./...).

For real inference-based tests, we can later add //go:build integration, which means they will require go test -tags=integration ./... to run. If no tag is provided (i.e., running go test -v), these tests won’t run.

@aeft aeft deleted the feature/classifier-test-framework branch September 5, 2025 21:04
@aeft
Copy link
Contributor Author

aeft commented Sep 5, 2025

image

@rootfs Would you like to only enable "allow squash merging" in the project settings? This might make the commit history more readable. (Sorry. I assumed the merge would squash commits, so some of my commit messages are a bit vague.)

@rootfs
Copy link
Collaborator

rootfs commented Sep 5, 2025

go test -tags=integration ./... to run. If no tag is provided (i.e., running go test -v), these tests won’t run.

Yes, that looks good

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.

[v0.1]Test and Release: Classifier test framework
4 participants