Skip to content

LCORE-1375: Updated e2e test#1625

Merged
radofuchs merged 1 commit intolightspeed-core:mainfrom
tisnik:lcore-1375-updated-e2e-test
Apr 29, 2026
Merged

LCORE-1375: Updated e2e test#1625
radofuchs merged 1 commit intolightspeed-core:mainfrom
tisnik:lcore-1375-updated-e2e-test

Conversation

@tisnik
Copy link
Copy Markdown
Contributor

@tisnik tisnik commented Apr 29, 2026

Description

LCORE-1375: Updated e2e test

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

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

Related Tickets & Documents

  • Related Issue #LCORE-1375

Summary by CodeRabbit

  • Tests
    • Updated test utility code for improved maintainability.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 29, 2026

Walkthrough

A type annotation in the request_with_transient_retry function is updated from using union syntax (BaseException | None) to Optional[BaseException]. No runtime behavior changes.

Changes

Cohort / File(s) Summary
Type Annotation Update
tests/e2e/utils/utils.py
Re-annotated local variable last_err from `BaseException

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive The title is vague and generic, lacking specificity about what was actually updated in the e2e test. Revise the title to be more specific, such as 'Update type annotation for last_err variable in e2e test utils' to clearly convey the actual change.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
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
Review rate limit: 0/1 reviews remaining, refill in 60 minutes.

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

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
tests/e2e/utils/utils.py (1)

83-92: 🧹 Nitpick | 🔵 Trivial

Minor: narrow last_err type to match the actual exception.

You only ever assign last_err from requests.exceptions.ConnectionError, but the annotation is Optional[BaseException]. Tightening this to Optional[requests.exceptions.ConnectionError] would better align with the function docstring (“last ConnectionError”) and improve type precision.

Suggested tweak
 def request_with_transient_retry(
     **kwargs: Any,
 ) -> requests.Response:
@@
-    last_err: Optional[BaseException] = None
+    last_err: Optional[requests.exceptions.ConnectionError] = None
     for attempt in range(E2E_HTTP_TRANSIENT_MAX_ATTEMPTS):
         try:
             return requests.request(**kwargs)
         except requests.exceptions.ConnectionError as exc:
             last_err = exc
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/e2e/utils/utils.py` around lines 83 - 92, The variable annotation
last_err should be narrowed to match the actual exception type assigned
(requests.exceptions.ConnectionError) rather than BaseException; update the
declaration of last_err to use Optional[requests.exceptions.ConnectionError] and
keep the existing logic in the retry loop that assigns ConnectionError to
last_err and later asserts/raises it, referencing the same symbols (last_err,
requests.exceptions.ConnectionError, E2E_HTTP_TRANSIENT_MAX_ATTEMPTS) so static
typing reflects the documented “last ConnectionError.”
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@tests/e2e/utils/utils.py`:
- Around line 83-92: The variable annotation last_err should be narrowed to
match the actual exception type assigned (requests.exceptions.ConnectionError)
rather than BaseException; update the declaration of last_err to use
Optional[requests.exceptions.ConnectionError] and keep the existing logic in the
retry loop that assigns ConnectionError to last_err and later asserts/raises it,
referencing the same symbols (last_err, requests.exceptions.ConnectionError,
E2E_HTTP_TRANSIENT_MAX_ATTEMPTS) so static typing reflects the documented “last
ConnectionError.”

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: ed9000e1-9bec-4cf7-87af-0a9f28aa5277

📥 Commits

Reviewing files that changed from the base of the PR and between 93b2bb5 and 73c119c.

📒 Files selected for processing (1)
  • tests/e2e/utils/utils.py
📜 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). (19)
  • GitHub Check: bandit
  • GitHub Check: integration_tests (3.13)
  • GitHub Check: build-pr
  • GitHub Check: radon
  • GitHub Check: unit_tests (3.12)
  • GitHub Check: unit_tests (3.13)
  • GitHub Check: integration_tests (3.12)
  • GitHub Check: Pylinter
  • GitHub Check: Pyright
  • GitHub Check: pydocstyle
  • GitHub Check: mypy
  • GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
  • GitHub Check: E2E Tests for Lightspeed Evaluation job
  • GitHub Check: E2E: library mode / ci / group 2
  • GitHub Check: E2E: server mode / ci / group 1
  • GitHub Check: E2E: library mode / ci / group 1
  • GitHub Check: E2E: server mode / ci / group 2
  • GitHub Check: E2E: server mode / ci / group 3
  • GitHub Check: E2E: library mode / ci / group 3
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

**/*.py: Use absolute imports for internal modules: from authentication import get_auth_dependency
Import FastAPI dependencies with: from fastapi import APIRouter, HTTPException, Request, status, Depends
Import Llama Stack client with: from llama_stack_client import AsyncLlamaStackClient
Check constants.py for shared constants before defining new ones
All modules start with descriptive docstrings explaining purpose
Use logger = get_logger(__name__) from log.py for module logging
Type aliases defined at module level for clarity
Use Final[type] as type hint for all constants
All functions require docstrings with brief descriptions
Complete type annotations for parameters and return types in functions
Use typing_extensions.Self for model validators in Pydantic models
Use modern union type syntax str | int instead of Union[str, int]
Use Optional[Type] for optional type hints
Use snake_case with descriptive, action-oriented function names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead
Use async def for I/O operations and external API calls
Handle APIConnectionError from Llama Stack in error handling
Use standard log levels with clear purposes: debug, info, warning, error
All classes require descriptive docstrings explaining purpose
Use PascalCase for class names with standard suffixes: Configuration, Error/Exception, Resolver, Interface
Use ABC for abstract base classes with @abstractmethod decorators
Use @model_validator and @field_validator for Pydantic model validation
Complete type annotations for all class attributes; use specific types, not Any
Follow Google Python docstring conventions with Parameters, Returns, Raises, and Attributes sections

Files:

  • tests/e2e/utils/utils.py
tests/e2e/**/*.{py,feature}

📄 CodeRabbit inference engine (AGENTS.md)

Use behave (BDD) framework for end-to-end testing

Files:

  • tests/e2e/utils/utils.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-19T15:40:25.624Z
Learning: Applies to tests/e2e/test_list.txt : Maintain test list in `tests/e2e/test_list.txt`
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-19T15:40:25.624Z
Learning: Applies to **/*.py : Use `Optional[Type]` for optional type hints
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-19T15:40:25.624Z
Learning: Applies to **/*.py : Use modern union type syntax `str | int` instead of `Union[str, int]`
🔇 Additional comments (1)
tests/e2e/utils/utils.py (1)

68-93: LGTM: transient retry logic + type annotation change is safe.

The request_with_transient_retry control flow is unchanged and the last_err annotation now uses Optional[...], matching the repo’s typing guidance for optional type hints.

Copy link
Copy Markdown
Contributor

@radofuchs radofuchs left a comment

Choose a reason for hiding this comment

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

LGTM

@radofuchs radofuchs merged commit a306803 into lightspeed-core:main Apr 29, 2026
34 of 36 checks passed
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.

2 participants