Skip to content

LCORE-1279: add unit tests for is_moderation_id and check_suid_prompt#1932

Open
alessandralanz wants to merge 3 commits into
lightspeed-core:mainfrom
alessandralanz:lcore-1279
Open

LCORE-1279: add unit tests for is_moderation_id and check_suid_prompt#1932
alessandralanz wants to merge 3 commits into
lightspeed-core:mainfrom
alessandralanz:lcore-1279

Conversation

@alessandralanz

@alessandralanz alessandralanz commented Jun 15, 2026

Copy link
Copy Markdown

Description

Follows up on the prior PR #1107 for LCORE-1279 which added tests for check_suid, normalize_conversation_id, and to_llama_stack_conversation_id. This PR completes the ticket by adding tests for the two remaining untested functions:

  • is_moderation_id: valid prefix, partial prefix, case sensitivity, empty string, cross-format rejection
  • check_suid_prompt: valid input, wrong prefix, wrong hex length (off-by-one), non-hex characters, cross-format rejection

Type of change

  • Refactor
  • New feature
  • Bug fix
  • CVE fix
  • Optimization
  • Documentation Update
  • Configuration Update
  • Bump-up service version
  • Bump-up dependent library
  • Bump-up library or tool used for development (does not change the final image)
  • CI configuration change
  • Konflux configuration change
  • Unit tests improvement
  • Integration tests improvement
  • End to end tests improvement
  • Benchmarks improvement

Tools used to create PR

Identify any AI code assistants used in this PR (for transparency and review context)

  • Assisted-by: Claude
  • Generated by: N/A

Related Tickets & Documents

  • Related Issue #LCORE-1279

Checklist before requesting a review

  • I have performed a self-review of my code.
  • PR has passed all pre-merge test jobs.
  • If it is a core feature, I have added thorough tests.

Testing

  • Please provide detailed steps to perform tests related to this code change.
  • How were the fix/results from this change verified? Please provide relevant screenshots or results.

Run the suid unit tests:
uv run pytest tests/unit/utils/test_suid.py -v

Run with coverage to verify 100% line and branch coverage:
uv run pytest tests/unit/utils/test_suid.py --cov=utils.suid --cov-report=term-missing --cov-branch

All 23 tests pass. Coverage output:

Name Statements Miss Branch BrPart Cover
src/utils/suid.py 40 0 12 0 100%

Summary by CodeRabbit

  • Tests
    • Expanded unit test coverage for identifier and prompt validation, including positive cases and edge conditions, plus comprehensive negative tests for empty inputs, prefix/casing mismatches, incorrect length, invalid characters, and unsupported ID formats.

@coderabbitai

coderabbitai Bot commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: b7f98b75-3cbd-4f96-8a40-d2cd9cf06591

📥 Commits

Reviewing files that changed from the base of the PR and between 305c9df and a608e12.

📒 Files selected for processing (1)
  • tests/unit/utils/test_suid.py
📜 Recent review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
  • GitHub Check: E2E: server mode / ci / group 2
  • GitHub Check: E2E: server mode / ci / group 3
  • GitHub Check: E2E: library mode / ci / group 3
  • GitHub Check: E2E: server mode / ci / group 1
  • GitHub Check: E2E: library mode / ci / group 2
  • GitHub Check: E2E: library mode / ci / group 1
  • GitHub Check: E2E Tests for Lightspeed Evaluation job
🧰 Additional context used
📓 Path-based instructions (1)
tests/**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

tests/**/*.py: Use pytest for all unit and integration tests; do not use unittest
Use pytest.mark.asyncio marker for async tests

Files:

  • tests/unit/utils/test_suid.py
🔇 Additional comments (1)
tests/unit/utils/test_suid.py (1)

137-141: LGTM!

Also applies to: 154-155


Walkthrough

Adds 53 lines of unit tests to tests/unit/utils/test_suid.py for two existing utility functions: is_moderation_id (valid modr-prefixed strings and various invalid cases) and check_suid_prompt (pmpt_ prefix with exactly 48 hex characters and all rejection cases).

Changes

SUID Utility Test Coverage

Layer / File(s) Summary
is_moderation_id unit tests
tests/unit/utils/test_suid.py
Tests assert True for lowercase modr-prefixed strings and False for empty, truncated/wrong-case prefixes, embedded occurrences, and UUID-shaped strings.
check_suid_prompt unit tests
tests/unit/utils/test_suid.py
Tests assert True only for pmpt_ followed by exactly 48 hex characters, and False for missing/wrong prefix (including case variants), empty input, incorrect hex lengths, non-hex characters in payload, and IDs from other supported formats (UUID, modr-..., conv_...).

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically identifies the main change: adding unit tests for two functions (is_moderation_id and check_suid_prompt) as part of the LCORE-1279 ticket.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
✨ Simplify code
  • Create PR with simplified code

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@asimurka

Copy link
Copy Markdown
Contributor

/ok-to-test

@asimurka asimurka 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.

LGTM, please fix failing linters (use make verify)

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants