Skip to content

LCORE-2899: Added missing type arguments - #2052

Merged
tisnik merged 1 commit into
lightspeed-core:mainfrom
tisnik:lcore-2899
Jul 3, 2026
Merged

tisnik merged 1 commit into
lightspeed-core:mainfrom
tisnik:lcore-2899

Conversation

@tisnik

@tisnik tisnik commented Jul 3, 2026 •

Copy link
Copy Markdown
Contributor

Description

LCORE-2899: Added missing type arguments

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-2899

Summary by CodeRabbit

  • Tests
    • Improved unit test coverage and type validation around response text extraction and tool merge behavior.
    • No user-facing behavior changes were introduced.

@coderabbitai

coderabbitai Bot commented Jul 3, 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: 231e1b26-e804-43cb-a06c-f31c3871a96c

📥 Commits

Reviewing files that changed from the base of the PR and between c62c782 and 2530dc7.

📒 Files selected for processing (1)
  • tests/unit/utils/test_responses.py
📜 Recent review details
⏰ Context from checks skipped due to timeout. (18)
  • GitHub Check: Pyright
  • GitHub Check: build-pr
  • GitHub Check: spectral
  • GitHub Check: unit_tests (3.12)
  • GitHub Check: unit_tests (3.13)
  • GitHub Check: Pylinter
  • GitHub Check: mypy
  • GitHub Check: integration_tests (3.13)
  • GitHub Check: integration_tests (3.12)
  • GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-0-6-on-pull-request
  • GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
  • GitHub Check: E2E: server mode / ci / group 3
  • GitHub Check: E2E: library mode / ci / group 3
  • GitHub Check: E2E: server mode / ci / group 2
  • GitHub Check: E2E: server mode / ci / group 1
  • GitHub Check: E2E: library mode / ci / group 1
  • GitHub Check: E2E: library mode / ci / group 2
  • 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_responses.py
🧠 Learnings (1)
📚 Learning: 2026-06-24T13:45:37.249Z
Learnt from: Jdubrick
Repo: lightspeed-core/lightspeed-stack PR: 1971
File: src/utils/markdown_repair.py:31-36
Timestamp: 2026-06-24T13:45:37.249Z
Learning: In the lightspeed-stack repository, docstrings must use the section header name "Parameters:" (not "Args:") for function arguments, even if the project references Google Python docstring conventions. Ensure docstrings follow the project’s established "Parameters:" header format for any documented function parameters.

Applied to files:

  • tests/unit/utils/test_responses.py
🪛 ast-grep (0.44.0)
tests/unit/utils/test_responses.py

[warning] 3314-3314: Do not make http calls without encryption
Context: "http://client"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.

(requests-http)


[warning] 3317-3317: Do not make http calls without encryption
Context: "http://server"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.

(requests-http)


[warning] 3330-3330: Do not make http calls without encryption
Context: "http://client"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.

(requests-http)


[warning] 3333-3333: Do not make http calls without encryption
Context: "http://server"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.

(requests-http)


[warning] 3360-3360: Do not make http calls without encryption
Context: "http://client"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.

(requests-http)


[warning] 3369-3369: Do not make http calls without encryption
Context: "http://server"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.

(requests-http)


[warning] 3380-3380: Do not make http calls without encryption
Context: "http://a"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.

(requests-http)


[warning] 3381-3381: Do not make http calls without encryption
Context: "http://b"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.

(requests-http)


[warning] 3384-3384: Do not make http calls without encryption
Context: "http://c"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.

(requests-http)

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

53-53: LGTM!

Also applies to: 257-257, 278-278, 296-296, 319-319, 330-330, 341-341, 352-352, 365-365, 3314-3317, 3330-3333, 3345-3348, 3360-3360, 3369-3369, 3380-3384


Walkthrough

This PR modifies test_responses.py by adding a BaseModel import, removing unnecessary type: ignore comments from extract_text test assertions, and updating client_tools/server_tools fixture annotations from untyped list to list[BaseModel] in TestMergeTools tests.

Changes

Test typing cleanup

Layer / File(s) Summary
Extract text tests: remove type-ignore comments
tests/unit/utils/test_responses.py
BaseModel import added alongside AnyUrl; inline # type: ignore[...] comments removed from several extract_text_from_response_item/items test assignments without changing logic or assertions.
Merge tools tests: list[BaseModel] annotations
tests/unit/utils/test_responses.py
client_tools and server_tools fixtures in TestMergeTools tests re-typed from untyped list to list[BaseModel], including empty-list cases, while keeping the same constructed tool objects.

Estimated code review effort: 1 (Trivial) | ~3 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 accurately summarizes the main change: adding missing type arguments in the test file.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.
✨ 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.

@tisnik
tisnik merged commit eab02ba into lightspeed-core:main Jul 3, 2026
32 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.

1 participant