Skip to content

feat(e2e): Add comprehensive signal-decision engine test coverage #695

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 2 commits into from
Nov 20, 2025
Merged

feat(e2e): Add comprehensive signal-decision engine test coverage #695

merged 2 commits into from
Nov 20, 2025

Conversation

@yehudit1987
Copy link
Contributor

@yehudit1987 yehudit1987 commented Nov 19, 2025 •
edited
Loading

Implements 6 new E2E test suites to validate the signal-decision
engine :

New Test Files:

  • decision_priority.go: Priority-based routing (4 cases)
  • plugin_chain_execution.go: Plugin ordering & blocking (4 cases)
  • rule_condition_logic.go: AND/OR operators (6 cases)
  • decision_fallback.go: Fallback behavior (5 cases)
  • keyword_routing.go: Keyword matching (6 cases)
  • plugin_config_variations.go: Plugin configs (6 cases)

Key Fixes:

  • Fix keyword_rules field: 'category' → 'name'
  • Fix rule condition field: 'rule_name' → 'name'
  • Add keyword conditions to thinking_decision
  • Standardize fallback decision name: 'general_decision' → 'other_decision'

Additional Changes:

  • Add signal-decision tests to all 3 profiles (ai-gateway, aibrix, dynamic-config)
  • Fix markdown formatting in e2e/README.md and website/docs/api/crd-reference.md

Resolve #692

@netlify
Copy link

netlify bot commented Nov 19, 2025 •
edited
Loading

✅ Deploy Preview for vllm-semantic-router ready!

Name Link
🔨 Latest commit 0ba3985
🔍 Latest deploy log https://app.netlify.com/projects/vllm-semantic-router/deploys/691efd92eab26f00084c1a36
😎 Deploy Preview https://deploy-preview-695--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.

@github-actions
Copy link

github-actions bot commented Nov 19, 2025 •
edited
Loading

👥 vLLM Semantic Team Notification

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

📁 e2e

Owners: @Xunzhuo
Files changed:

  • e2e/testcases/decision_fallback.go
  • e2e/testcases/decision_priority.go
  • e2e/testcases/keyword_routing.go
  • e2e/testcases/plugin_chain_execution.go
  • e2e/testcases/plugin_config_variations.go
  • e2e/testcases/rule_condition_logic.go
  • e2e/README.md
  • e2e/profiles/ai-gateway/profile.go
  • e2e/profiles/ai-gateway/values.yaml
  • e2e/profiles/aibrix/profile.go
  • e2e/profiles/dynamic-config/crds/intelligentroute.yaml
  • e2e/profiles/dynamic-config/profile.go
  • e2e/testcases/common.go

📁 deploy

Owners: @rootfs, @Xunzhuo
Files changed:

  • deploy/kubernetes/aibrix/semantic-router-values/values.yaml

📁 website

Owners: @Xunzhuo, @rootfs, @yuluo-yx
Files changed:

  • website/docs/api/crd-reference.md
vLLM

🎉 Thanks for your contributions!

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

@yehudit1987 yehudit1987 force-pushed the test_coverage_sd branch 3 times, most recently from be04d47 to bb8d3d2 Compare November 19, 2025 14:43
Xunzhuo
Xunzhuo reviewed Nov 19, 2025
View reviewed changes
Copy link
Member

@Xunzhuo Xunzhuo left a comment

Choose a reason for hiding this comment

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

hopefully you can catch some bugs after adding the testcases, happy hacking! thanks

@yehudit1987 yehudit1987 force-pushed the test_coverage_sd branch 2 times, most recently from 674043b to bc5f724 Compare November 19, 2025 18:10
@Xunzhuo
Copy link
Member

Xunzhuo commented Nov 20, 2025

We need to add this to dynamic config profile as well, plz check the relevant files

Xunzhuo
Xunzhuo reviewed Nov 20, 2025
View reviewed changes
fix wrong header in test
0ba3985 
Signed-off-by: Yehudit Kerido <[email protected]>
@yehudit1987 yehudit1987 marked this pull request as ready for review November 20, 2025 12:07
@yehudit1987 yehudit1987 requested a review from rootfs as a code owner November 20, 2025 12:07
@yehudit1987
Copy link
Contributor Author

yehudit1987 commented Nov 20, 2025 •
edited
Loading

Hi @Xunzhuo.
I've added 6 new signal-decision engine tests across all 3 profiles. While implementing them, I discovered some (probably) backend issues:

Test Accuracy:

• ✅ decision-priority-selection: 100% (working perfectly)
• ⚠️ plugin-chain-execution: 75% (US_SSN detection works, EMAIL_ADDRESS doesn't)
• 🔴 keyword-routing: 17% (only "ASAP" keyword works)
• 🔴 rule-condition-logic: 33% (keywords work sporadically)
• 🔴 decision-fallback-behavior: 40% (classifier unreliable)
• ⚠️ plugin-config-variations: 50% (same classifier issues)

Quick Summary of Issues:

1. PII: EMAIL_ADDRESS not blocking (returns 200 instead of 403), but US_SSN works fine
2. Keywords: Only "ASAP" matches consistently, others ("urgent", "think", "careful") fail
3. Domain Classifier: Returns empty/wrong classifications for obvious queries

I hope I have verified correctly the test implementations (proper headers, config, model parameter). I suggest the issues are backend bugs, but feel free to double check this conclusion. Should I open separate issues for these backend bugs, or would you prefer to fix them before merging? Tests currently "pass" in CI (0% threshold) so they won't block anything. Anyway I will be happy to take part in those future investigations.

@Xunzhuo
Copy link
Member

Xunzhuo commented Nov 20, 2025

Thanks!

@Xunzhuo Xunzhuo merged commit a149800 into vllm-project:main Nov 20, 2025
19 checks passed
@Xunzhuo
Copy link
Member

Xunzhuo commented Nov 20, 2025

@yehudit1987 would you like to try adding test cases around embedding signals?

@Xunzhuo
Copy link
Member

Xunzhuo commented Nov 20, 2025

And plz create issues for your findings, if u can dig more that will be helpful

@yehudit1987
Copy link
Contributor Author

yehudit1987 commented Nov 20, 2025

And plz create issues for your findings, if u can dig more that will be helpful

I will, please leave it for me. Will create those next week and ask for your approval.

@yehudit1987 yehudit1987 deleted the test_coverage_sd branch November 23, 2025 06:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Xunzhuo Xunzhuo Xunzhuo approved these changes

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

Assignees

@rootfs rootfs

@Xunzhuo Xunzhuo

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[E2E] Add Signal-Decision Engine Test Coverage for New Plugin Architecture

3 participants

@yehudit1987 @Xunzhuo @rootfs