Skip to content

LCORE-1051: optional types in tests#976

Merged
tisnik merged 4 commits into
lightspeed-core:mainfrom
tisnik:lcore-1051-optional-types-in-tests
Jan 11, 2026
Merged

LCORE-1051: optional types in tests#976
tisnik merged 4 commits into
lightspeed-core:mainfrom
tisnik:lcore-1051-optional-types-in-tests

Conversation

@tisnik
Copy link
Copy Markdown
Contributor

@tisnik tisnik commented Jan 11, 2026

Description

LCORE-1051: optional types in tests

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

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

Related Tickets & Documents

  • Related Issue #LCORE-1051

Summary by CodeRabbit

  • Tests
    • Updated type annotations in test files for improved type consistency.

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

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Jan 11, 2026

Walkthrough

Type hints are updated across four test files, converting union syntax (str | None) to explicit Optional[str] annotations with corresponding typing imports. No behavioral or logic changes introduced.

Changes

Cohort / File(s) Summary
E2E Feedback Test
tests/e2e/features/steps/feedback.py
Updated access_feedback_post_endpoint parameter type hint from str | None to Optional[str], added typing import
Unit Test Type Annotations
tests/unit/cache/test_noop_cache.py, tests/unit/models/rlsapi/test_requests.py, tests/unit/utils/test_responses.py
Updated function parameter type hints from union syntax (str | None) to Optional[str] across cache, request validation, and response utility test modules; added Optional typing imports

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~4 minutes

Suggested reviewers

  • umago
🚥 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 concisely describes the main change: refactoring optional type hints from union syntax to Optional across test files.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

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

✨ Finishing touches
  • 📝 Generate docstrings

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.

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.

Actionable comments posted: 0

🧹 Nitpick comments (4)
tests/unit/cache/test_noop_cache.py (1)

3-3: Consider using modern union syntax str | None instead of Optional[str].

The PR changes test type hints from modern union syntax (str | None) to the older Optional[str] style. Based on retrieved learnings, source files in src/**/*.py use modern union syntax. Modern Python (3.10+) prefers Type | None over Optional[Type] for better readability and consistency with PEP 604.

Consider keeping the union syntax (str | None) in test files to maintain consistency with modern Python practices and the coding style used in source files, unless there's a specific project requirement to use Optional in tests.

Based on learnings, src/**/*.py files use modern union syntax. Should test files follow the same convention?

Also applies to: 203-203, 210-210, 217-217

tests/unit/utils/test_responses.py (1)

4-4: Consider using modern union syntax str | None instead of Optional[str].

Similar to other test files in this PR, these helper functions convert from modern union syntax to Optional. Modern Python (3.10+) prefers Type | None for better readability and alignment with PEP 604.

Consider maintaining the union syntax for consistency with modern Python practices and to avoid unnecessary import overhead.

Based on learnings, source files use modern union syntax. Verify if test files should follow the same convention.

Also applies to: 12-12, 28-28

tests/unit/models/rlsapi/test_requests.py (1)

4-4: Consider using modern union syntax str | None instead of Optional[str].

This test function converts from modern union syntax to Optional[str]. Modern Python (3.10+) recommends using Type | None over Optional[Type] for improved readability and consistency with PEP 604.

To align with modern Python practices and maintain consistency across the codebase, consider retaining the union syntax.

Based on learnings, source files prefer modern union syntax. Should test files follow the same convention?

Also applies to: 272-274

tests/e2e/features/steps/feedback.py (1)

3-3: Consider using modern union syntax str | None instead of Optional[str].

This behave step helper function converts from modern union syntax to Optional[str]. Modern Python (3.10+) favors Type | None over Optional[Type] for better consistency with PEP 604 and reduced import dependencies.

Consider keeping the union syntax to align with modern Python best practices and maintain consistency with source file conventions.

Based on learnings, source files use modern union syntax. Verify if the same convention should apply to e2e test files.

Also applies to: 76-76

📜 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 4e5629e and ac291b8.

📒 Files selected for processing (4)
  • tests/e2e/features/steps/feedback.py
  • tests/unit/cache/test_noop_cache.py
  • tests/unit/models/rlsapi/test_requests.py
  • tests/unit/utils/test_responses.py
🧰 Additional context used
📓 Path-based instructions (3)
tests/unit/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

Use pytest for all unit and integration tests

Files:

  • tests/unit/utils/test_responses.py
  • tests/unit/cache/test_noop_cache.py
  • tests/unit/models/rlsapi/test_requests.py
tests/**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

tests/**/*.py: Do not use unittest - pytest is the standard for this project
Use pytest-mock for AsyncMock objects in tests

Files:

  • tests/unit/utils/test_responses.py
  • tests/unit/cache/test_noop_cache.py
  • tests/unit/models/rlsapi/test_requests.py
  • tests/e2e/features/steps/feedback.py
tests/e2e/**

📄 CodeRabbit inference engine (CLAUDE.md)

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

Files:

  • tests/e2e/features/steps/feedback.py
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T15:39:01.298Z
Learning: Applies to src/**/*.py : Use modern union syntax: `str | int` instead of `Union[str, int]`
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T15:39:01.298Z
Learning: Applies to src/**/*.py : Use `Optional[Type]` or `Type | None` for optional types
📚 Learning: 2026-01-09T15:39:01.298Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-09T15:39:01.298Z
Learning: Applies to src/**/*.py : Use `Optional[Type]` or `Type | None` for optional types

Applied to files:

  • tests/unit/utils/test_responses.py
  • tests/unit/models/rlsapi/test_requests.py
  • tests/e2e/features/steps/feedback.py
📚 Learning: 2025-09-02T11:14:17.117Z
Learnt from: radofuchs
Repo: lightspeed-core/lightspeed-stack PR: 485
File: tests/e2e/features/steps/common_http.py:244-255
Timestamp: 2025-09-02T11:14:17.117Z
Learning: The POST step in tests/e2e/features/steps/common_http.py (`access_rest_api_endpoint_post`) is intentionally designed as a general-purpose HTTP POST method, not specifically for REST API endpoints, so it should not include context.api_prefix in the URL construction.

Applied to files:

  • tests/e2e/features/steps/feedback.py
🧬 Code graph analysis (2)
tests/unit/utils/test_responses.py (1)
src/utils/responses.py (1)
  • extract_text_from_response_output_item (6-56)
tests/unit/cache/test_noop_cache.py (1)
src/cache/noop_cache.py (1)
  • NoopCache (12-128)
⏰ 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). (4)
  • GitHub Check: build-pr
  • GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
  • GitHub Check: E2E: server mode / ci
  • GitHub Check: E2E: library mode / ci

@tisnik tisnik merged commit 78665d9 into lightspeed-core:main Jan 11, 2026
21 of 23 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