LCORE-574: unit tests for customization #504
Conversation
WalkthroughAdds two test resource files containing single-line and multiline system prompts and new unit tests for a Customization model that validate default flags, path validation, and loading prompt content from provided file paths. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Test
participant Customization
participant FS as FileSystem
Test->>Customization: instantiate Customization(system_prompt_path=path?)
alt system_prompt_path provided
Customization->>FS: stat(path) / is_file?
alt not a file
Customization-->>Test: raise ValidationError("Path does not point to a file")
else file exists
FS-->>Customization: read file contents
Customization-->>Test: instance with system_prompt set to file contents
end
else no path provided
Customization-->>Test: instance with system_prompt=None
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
tisnik
force-pushed
the
lcore-574-unit-tests-for-customization
branch
from
e0cd8d9 to
f8d4365
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/configuration/multiline_system_prompt.txt (1)
1-15: Avoid time-sensitive facts in fixtures.Hardcoding “latest version is 4.19” will age and may cause churn when updating fixtures. Prefer a timeless phrasing.
Apply this diff:
- - The latest version of OpenShift is 4.19. + - OpenShift ships frequent minor releases; avoid claiming a specific "latest" version in static prompts.tests/unit/models/test_config.py (3)
1170-1185: Good coverage for default/disabled states; add an interaction case.Consider verifying behavior when both
disable_query_system_prompt=Trueand asystem_prompt_pathare provided (should the path be ignored or error?). This guards against ambiguous configurations.Proposed addition inside this test:
with subtests.test(msg="System prompt is disabled"): c = Customization(disable_query_system_prompt=True) assert c is not None assert c.disable_query_system_prompt is True assert c.system_prompt_path is None assert c.system_prompt is None + with subtests.test(msg="Disabled overrides provided path (ignore or error)"): + # If spec says "ignore", expect None; if it should "error", expect ValidationError. + try: + c = Customization( + disable_query_system_prompt=True, + system_prompt_path="tests/configuration/system_prompt.txt", + ) + # If constructible, assert prompt/path are not used. + assert c.system_prompt_path is None + assert c.system_prompt is None + except ValidationError: + # Also acceptable if the model enforces exclusivity. + pass
1187-1191: Nice negative test; consider directory-path case too.Mirroring your TLS tests, add a case where
system_prompt_pathpoints to a directory to ensure the validator rejects it.Apply:
def test_service_customization_wrong_system_prompt_path() -> None: """Check the service customization class.""" with pytest.raises(ValidationError, match="Path does not point to a file"): _ = Customization(system_prompt_path="/path/does/not/exists") + + # directory instead of file + with pytest.raises(ValidationError, match="Path does not point to a file"): + _ = Customization(system_prompt_path="tests/")
1193-1211: Make fixture paths robust to working directory.Deriving paths from this file avoids failures when pytest is invoked from non-repo roots.
Apply:
def test_service_customization_correct_system_prompt_path(subtests) -> None: """Check the service customization class.""" - with subtests.test(msg="One line system prompt"): - # pass a file containing system prompt - c = Customization(system_prompt_path="tests/configuration/system_prompt.txt") - assert c is not None - # check that the system prompt has been loaded from the provided file - assert c.system_prompt == "This is system prompt." + base_dir = Path(__file__).resolve().parents[2] / "configuration" + + with subtests.test(msg="One line system prompt"): + c = Customization(system_prompt_path=str(base_dir / "system_prompt.txt")) + assert c is not None + assert c.system_prompt == "This is system prompt." with subtests.test(msg="Multi line system prompt"): - # pass a file containing system prompt - c = Customization( - system_prompt_path="tests/configuration/multiline_system_prompt.txt" - ) + c = Customization(system_prompt_path=str(base_dir / "multiline_system_prompt.txt")) assert c is not None # check that the system prompt has been loaded from the provided file assert "You are OpenShift Lightspeed" in c.system_prompt assert "Here are your instructions" in c.system_prompt assert "Here are some basic facts about OpenShift" in c.system_prompt
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
tests/configuration/multiline_system_prompt.txt(1 hunks)tests/configuration/system_prompt.txt(1 hunks)tests/unit/models/test_config.py(2 hunks)
🧰 Additional context used
🪛 LanguageTool
tests/configuration/multiline_system_prompt.txt
[grammar] ~1-~1: There might be a mistake here.
Context: ...t assistant for question-answering tasks related to the OpenShift container orche...
(QB_NEW_EN)
tests/configuration/system_prompt.txt
[grammar] ~1-~1: There might be a mistake here.
Context: This is system prompt.
(QB_NEW_EN)
[grammar] ~1-~1: There might be a mistake here.
Context: This is system prompt.
(QB_NEW_EN)
⏰ 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). (2)
- GitHub Check: build-pr
- GitHub Check: e2e_tests
🔇 Additional comments (2)
tests/configuration/system_prompt.txt (1)
1-1: Fixture content LGTM; keep stable for exact-match assertion.This string is intentionally simple and is asserted verbatim in tests. Avoid “fixing” grammar, as it would break the equality check.
tests/unit/models/test_config.py (1)
38-39: New import usage is appropriate.
Customizationis exercised by tests below; import placement is fine.
tisnik
force-pushed
the
lcore-574-unit-tests-for-customization
branch
from
f8d4365 to
9744ee5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (10)
tests/configuration/system_prompt.txt (1)
1-1: Grammar nit: add missing article for clarity (fixture).Optional polish; if you change it, also update assertions in tests that compare the exact string.
Apply this diff if you want to fix the grammar:
-This is system prompt. +This is a system prompt.tests/configuration/multiline_system_prompt.txt (2)
13-13: Avoid hardcoding a time-sensitive version in test data.Embedding “latest version is 4.19” will age quickly and can confuse future readers. Prefer a non-committal phrasing.
Suggested change:
-- The latest version of OpenShift is 4.19. +- The latest version of OpenShift changes frequently; refer to official docs for the current release.
10-10: Rephrase unrealistic capability claim.“Forbid mentioning last update” is fine, but “You have the most recent information” is brittle as a general rule. Consider neutral wording.
Suggested change:
-Do not mention your last update. You have the most recent information on OpenShift. +Avoid mentioning a knowledge cutoff or “last update.”tests/unit/models/test_config.py (7)
1191-1191: Prefer Path objects over raw strings for paths (consistency with rest of tests).Apply:
- system_prompt_path="tests/configuration/system_prompt.txt", + system_prompt_path=Path("tests/configuration/system_prompt.txt"),
1195-1195: Make the equality robust to trailing newline in the fixture.If loader behavior changes (e.g., stops stripping), this stays green.
Apply:
- assert c.system_prompt == "This is system prompt." + assert c.system_prompt.strip() == "This is system prompt."
1201-1204: Use tmp_path to avoid OS-specific absolute paths in negative test.Keeps the test portable and hermetic.
Apply:
-def test_service_customization_wrong_system_prompt_path() -> None: +def test_service_customization_wrong_system_prompt_path(tmp_path) -> None: """Check the service customization class.""" - with pytest.raises(ValidationError, match="Path does not point to a file"): - _ = Customization(system_prompt_path="/path/does/not/exists") + with pytest.raises(ValidationError, match="Path does not point to a file"): + _ = Customization(system_prompt_path=tmp_path / "no-such-file.txt")
1210-1210: Nit: use Path for consistency.Apply:
- c = Customization(system_prompt_path="tests/configuration/system_prompt.txt") + c = Customization(system_prompt_path=Path("tests/configuration/system_prompt.txt"))
1213-1213: Same robustness tweak: strip newline in assertion.Apply:
- assert c.system_prompt == "This is system prompt." + assert c.system_prompt.strip() == "This is system prompt."
1218-1219: Nit: use Path for consistency.Apply:
- c = Customization( - system_prompt_path="tests/configuration/multiline_system_prompt.txt" + c = Customization( + system_prompt_path=Path("tests/configuration/multiline_system_prompt.txt") )
1170-1224: Optional: add a directory-path negative case for completeness.Covers the “path exists but is not a file” branch exercised elsewhere (TLS tests).
Add the following test (placement anywhere in this file is fine):
def test_service_customization_system_prompt_path_points_to_directory() -> None: with pytest.raises(ValidationError, match="Path does not point to a file"): _ = Customization(system_prompt_path=Path("tests/"))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
tests/configuration/multiline_system_prompt.txt(1 hunks)tests/configuration/system_prompt.txt(1 hunks)tests/unit/models/test_config.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/unit/models/test_config.py (1)
src/models/config.py (1)
Customization(416-431)
🪛 LanguageTool
tests/configuration/multiline_system_prompt.txt
[grammar] ~1-~1: There might be a mistake here.
Context: ...t assistant for question-answering tasks related to the OpenShift container orche...
(QB_NEW_EN)
tests/configuration/system_prompt.txt
[grammar] ~1-~1: There might be a mistake here.
Context: This is system prompt.
(QB_NEW_EN)
[grammar] ~1-~1: There might be a mistake here.
Context: This is system prompt.
(QB_NEW_EN)
⏰ 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). (2)
- GitHub Check: build-pr
- GitHub Check: e2e_tests
🔇 Additional comments (3)
tests/unit/models/test_config.py (3)
38-38: LGTM: importing Customization where used.
1170-1198: Solid coverage for Customization defaults and disabled-mode semantics.Nice use of subtests; behavior is clear and aligned with the model validator.
1170-1224: Confirmed get_attribute_from_file uses.rstrip()to trim trailing whitespace/newlines, so exact‐value assertions remain valid.
1 participant
Description
LCORE-574: unit tests for customization
Type of change
Related Tickets & Documents
Summary by CodeRabbit
No user-facing changes.