Skip to content

Conversation

@max-svistunov
Copy link
Contributor

@max-svistunov max-svistunov commented Jan 14, 2026

Description

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

Tools used to create PR

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

  • Assisted-by: Claude Opus 4.5
  • Generated by: Claude Opus 4.5

Related Tickets & Documents

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.

Summary by CodeRabbit

  • Tests
    • Expanded test coverage for error response handling, including invalid authentication credentials, missing authorization tokens, and non-existent models.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 14, 2026

Walkthrough

Three new end-to-end test scenarios are added to the query feature test suite, covering error cases: invalid authentication token (401 response), missing Authorization header token (401 response), and non-existent model requests (404 response).

Changes

Cohort / File(s) Summary
E2E Query Feature Tests
tests/e2e/features/query.feature
Adds three new test scenarios for error handling: invalid token authentication, missing bearer token in Authorization header, and non-existent model requests. Each scenario validates the expected HTTP status code and error message response.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

Possibly related PRs

Suggested reviewers

  • tisnik
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 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 describes the main change: adding e2e tests for error status codes for the /query endpoint.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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


🧹 Recent nitpick comments
tests/e2e/features/query.feature (3)

67-76: Consider using full JSON validation for consistency.

The existing unauthenticated scenario (lines 50-65) validates the complete JSON structure of 401 errors, while this scenario only checks for a substring match. For better test coverage and consistency, consider validating the full error response structure.

♻️ Suggested approach for full JSON validation
-      And The body of the response contains Missing or invalid credentials provided by client
+      And The body of the response is the following
+      """
+      {
+        "detail": {
+          "response": "Missing or invalid credentials provided by client",
+          "cause": "<expected cause message>"
+        }
+      }
+      """

Note: Replace <expected cause message> with the actual error cause returned by the API for invalid tokens.


77-86: Consider using full JSON validation for consistency.

Similar to the previous scenario, this test uses partial substring matching while the existing unauthenticated test (lines 50-65) validates the complete JSON structure. Consider using full JSON validation to catch potential structural issues in the error response.

♻️ Suggested approach for full JSON validation
-      And The body of the response contains No token found in Authorization header
+      And The body of the response is the following
+      """
+      {
+        "detail": {
+          "response": "Missing or invalid credentials provided by client",
+          "cause": "No token found in Authorization header"
+        }
+      }
+      """

87-96: Good test isolation, consider full JSON validation.

The test correctly uses a valid authentication token to isolate the model-not-found error from authentication issues. However, like the 401 test cases, consider using full JSON validation instead of substring matching to ensure the complete error response structure is correct.

♻️ Optional: Full JSON validation approach
-      And The body of the response contains Model not found
+      And The body of the response is the following
+      """
+      {
+        "detail": {
+          "response": "Model not found",
+          "cause": "<expected cause message>"
+        }
+      }
+      """

Note: Replace <expected cause message> with the actual error details returned by the API, or adjust the structure to match the actual 404 response format.


📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 28caabf and 791bd37.

📒 Files selected for processing (1)
  • tests/e2e/features/query.feature
🧰 Additional context used
📓 Path-based instructions (1)
tests/e2e/features/**/*.feature

📄 CodeRabbit inference engine (CLAUDE.md)

Use behave (BDD) framework with Gherkin feature files for end-to-end tests

Files:

  • tests/e2e/features/query.feature
🧠 Learnings (1)
📓 Common learnings
Learnt from: radofuchs
Repo: lightspeed-core/lightspeed-stack PR: 485
File: tests/e2e/features/environment.py:87-95
Timestamp: 2025-09-02T11:09:40.404Z
Learning: In the lightspeed-stack e2e tests, noop authentication tests use the default lightspeed-stack.yaml configuration, while noop-with-token tests use the Authorized tag to trigger a config swap to the specialized noop-with-token configuration file.
⏰ 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). (3)
  • GitHub Check: build-pr
  • GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
  • GitHub Check: E2E: library mode / ci

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.


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.

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.

1 participant