feat: harden KIS US OHLCV cache behavior and source mapping#164
Open
robin-watcha wants to merge 1 commit intomainfrom
Open
feat: harden KIS US OHLCV cache behavior and source mapping#164robin-watcha wants to merge 1 commit intomainfrom
robin-watcha wants to merge 1 commit intomainfrom
Conversation
📝 WalkthroughWalkthroughThe pull request introduces a Redis-backed OHLCV cache layer for US equity data and migrates US market data sources from Yahoo Finance to KIS. Configuration options for cache management are added, along with a new caching module featuring distributed locking and probe-based refresh logic. Multiple tool handlers are updated to use the new KIS data source. Changes
Sequence DiagramsequenceDiagram
participant MCP as MCP Server
participant QS as Quote Service
participant Cache as KIS OHLCV Cache
participant Lock as Redis Lock
participant Fetch as Raw Fetcher
participant Redis as Redis Store
MCP->>QS: get_ohlcv(symbol, count, period, end_date)
QS->>Cache: get_closed_daily_candles(symbol, exchange_code, count, raw_fetcher)
Cache->>Cache: Normalize inputs & compute target_asof
Cache->>Redis: Check cache availability
Redis-->>Cache: Return cached rows (if available)
alt Cache available and fresh
Cache-->>QS: Return cached data
else Cache stale or empty
Cache->>Cache: Check probe status
alt Probe active (recent fetch in progress)
Cache-->>Cache: Skip fetch, return existing cache
else Probe inactive
Cache->>Lock: Acquire distributed lock
alt Lock acquired
Lock-->>Cache: Lock granted
Cache->>Fetch: Call raw_fetcher (KIS API)
Fetch-->>Cache: Return fresh OHLCV data
Cache->>Cache: Normalize daily frame
Cache->>Redis: Upsert rows, update probe, set TTL
Redis-->>Cache: Confirm write
Cache->>Lock: Release lock
else Lock timeout
Lock-->>Cache: Lock failed
Cache-->>Cache: Return cached data if available
end
end
Cache-->>QS: Return final data (cached or fresh)
end
QS-->>MCP: Return OHLCV candles
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
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. Comment |
|
PR Review: KIS US OHLCV Cache & Source MappingSummaryThis PR introduces a sophisticated Redis-based caching layer for KIS US daily OHLCV data and unifies the data source for US equities from Yahoo to KIS across multiple tools. The implementation is well-architected with comprehensive test coverage. ✅ Strengths1. Excellent Architecture
2. Robust Caching Strategy
3. Comprehensive Testing
4. Documentation
🔍 Code Quality ObservationsPotential Issues1. Global State Management (Minor)Location: app/services/kis_ohlcv_cache.py:21 _REDIS_CLIENT: redis.Redis | None = NoneIssue: Global mutable state can cause issues in testing/concurrent contexts 2. Magic Numbers (Minor)Location: app/services/kis_ohlcv_cache.py:443 for _ in range(6):
await asyncio.sleep(0.05)Issue: Hardcoded retry count and sleep duration KIS_OHLCV_CACHE_LOCK_RETRY_COUNT=6
KIS_OHLCV_CACHE_LOCK_RETRY_SLEEP=0.053. Error Handling Breadth (Minor)Location: app/services/kis_ohlcv_cache.py:520-524 except Exception as exc:
logger.warning(...)
return NoneIssue: Catches all exceptions which might hide unexpected errors 4. Count Capping Inconsistency (Medium)Locations:
Issue: Different max counts for US equity OHLCV data 5. Missing Exchange Code Validation (Medium)Location: app/services/kis_ohlcv_cache.py:358-360 normalized_symbol = str(symbol or "").strip().upper()
normalized_exchange = str(exchange_code or "").strip().upper()
if not normalized_symbol or not normalized_exchange:
return _empty_dataframe()Issue: Returns empty DataFrame for invalid inputs, which might be indistinguishable from "no data available" 🔐 Security Review✅ No Security Concerns Identified
⚡ Performance ConsiderationsStrengths
Potential Optimizations
🧪 Test Coverage AssessmentExcellent Coverage
Missing Test Cases (Recommendations)
📝 Documentation & MaintenanceStrengths
Recommendations
🎯 Best Practices Adherence✅ Follows Project Conventions
✅ Follows CLAUDE.md Guidelines
🔄 Breaking Changes & MigrationBreaking Changes
Migration Impact
📊 Final Assessment
✅ RecommendationAPPROVE with minor suggestions This is high-quality work that significantly improves the system by:
The suggestions above are minor improvements that can be addressed in follow-up PRs if desired. The current implementation is production-ready. Pre-merge Checklist
Great work on this PR! The caching implementation is robust and well-tested. 🚀 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/services/kis.py (1)
1818-1835:⚠️ Potential issue | 🟡 MinorDocument the new
end_dateparameter in the docstring.The function signature now includes
end_date, but the Args list doesn’t reflect it.As per coding guidelines: Document complex analysis logic and API integration details in docstrings using Google style format.✏️ Suggested docstring update
Args: symbol: 종목 심볼 (예: "AAPL") exchange_code: 거래소 코드 (NASD/NYSE/AMEX 등) n: 조회할 캔들 수 (최소 200개 권장, 이동평균선 계산용) period: D(일봉)/W(주봉)/M(월봉) + end_date: 종료 날짜 (None이면 최신 기준)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/services/kis.py` around lines 1818 - 1835, The docstring for the method that accepts parameters (symbol, exchange_code, n, period, end_date) is missing documentation for the new end_date parameter; update the Args section to include end_date with its type (datetime.date | None), default (None), and behavior (the last date to include in the returned candles; if None uses the most recent available date), and briefly note how it affects returned data alongside existing params like symbol, exchange_code, n, period and the return DataFrame (date, open, high, low, close, volume).
🧹 Nitpick comments (1)
tests/test_kis_ohlcv_cache.py (1)
263-287: Extract repeated settings monkeypatching into a shared fixture.The same block of ~6
monkeypatch.setattr(kis_ohlcv_cache.settings, ...)calls is duplicated across five async tests. A shared fixture would reduce boilerplate and make it easier to adjust defaults in one place.Suggested fixture
`@pytest.fixture` def _patch_cache_settings(monkeypatch): """Enable cache with sensible test defaults.""" monkeypatch.setattr(kis_ohlcv_cache.settings, "kis_ohlcv_cache_enabled", True, raising=False) monkeypatch.setattr(kis_ohlcv_cache.settings, "kis_ohlcv_cache_ttl_seconds", 300, raising=False) monkeypatch.setattr(kis_ohlcv_cache.settings, "kis_ohlcv_cache_max_days", 400, raising=False) monkeypatch.setattr(kis_ohlcv_cache.settings, "kis_ohlcv_cache_lock_ttl_seconds", 10, raising=False) monkeypatch.setattr(kis_ohlcv_cache.settings, "kis_ohlcv_cache_probe_retry_seconds", 1800, raising=False)Then use it via
@pytest.mark.usefixtures("_patch_cache_settings")or as a parameter on each test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_kis_ohlcv_cache.py` around lines 263 - 287, Extract the repeated monkeypatch.setattr calls into a shared pytest fixture named _patch_cache_settings that uses monkeypatch to set kis_ohlcv_cache.settings.kis_ohlcv_cache_enabled, kis_ohlcv_cache.settings.kis_ohlcv_cache_ttl_seconds, kis_ohlcv_cache.settings.kis_ohlcv_cache_max_days, kis_ohlcv_cache.settings.kis_ohlcv_cache_lock_ttl_seconds, and kis_ohlcv_cache.settings.kis_ohlcv_cache_probe_retry_seconds to the test defaults shown; then remove the repeated monkeypatch blocks from the tests and apply the fixture via `@pytest.mark.usefixtures`("_patch_cache_settings") or by adding _patch_cache_settings as a test parameter so all tests reuse the same configuration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/mcp_server/tooling/market_data_quotes.py`:
- Around line 276-321: The nested function _raw_fetcher inside
_fetch_ohlcv_equity_us is missing a return type hint; update its signature to
add an explicit return type (e.g., -> Any or the precise response type returned
by KISClient.inquire_overseas_daily_price) and ensure parameter types match
(symbol: str, exchange_code: str, n: int, end_date: datetime.date | None) so the
nested function has full type annotations; adjust any imports if you choose a
concrete return type and run type checks to confirm compatibility with
kis_ohlcv_cache.get_closed_daily_candles and
KISClient.inquire_overseas_daily_price.
In `@app/services/kis_ohlcv_cache.py`:
- Around line 24-38: The public helper functions expected_asof_et and
get_closed_daily_candles lack Google-style docstrings; add concise Google-style
docstrings to each (function summary, Args with types and meaning, Returns with
type and semantics, Raises if any exceptions can be thrown, and a short Example
or Notes for edge cases) describing their purpose (how expected_asof_et computes
the previous ET trading date given a UTC datetime and how
get_closed_daily_candles retrieves/constructs cached daily OHLCV rows),
reference parameter names (now_utc for expected_asof_et and the public
parameters of get_closed_daily_candles), and document timezone/market-close
logic and weekend handling so callers understand behavior and return values.
- Around line 289-308: In _set_probe_meta ensure the Redis meta-key TTL cannot
be shorter than the probe retry window: compute retry_seconds from settings (as
already done) and set the expire TTL to
max(int(settings.kis_ohlcv_cache_ttl_seconds), retry_seconds, 1) so the stored
probe_until_ts (and related probe fields in _set_probe_meta) will never expire
before the retry window elapses; update the expire call in _set_probe_meta to
use that computed value.
In `@tests/test_kis_ohlcv_cache.py`:
- Around line 194-197: The tests in this file (e.g.,
test_expected_asof_et_handles_est_edt_and_weekend_rollbacks) lack pytest
markers; add a file-level marker by defining pytestmark = pytest.mark.unit near
the top of the file (after imports) or alternatively decorate each test with
`@pytest.mark.unit`; ensure pytest is imported in the file (import pytest) if not
already present so the marker resolves.
In `@tests/test_mcp_server_tools.py`:
- Around line 5741-5756: Add the pytest unit marker and parameter/return type
hints to the async test
"test_get_support_resistance_us_error_payload_source_is_kis": add the decorator
`@pytest.mark.unit` above `@pytest.mark.asyncio`, and update the function signature
to include the monkeypatch parameter type and return type (e.g. monkeypatch:
pytest.MonkeyPatch) -> None. Ensure the test still uses build_tools and
_patch_runtime_attr as before and that pytest is imported so pytest.MonkeyPatch
is available.
- Around line 1268-1286: Add the required pytest classification marker and type
hints to the test: annotate the async test function
test_get_ohlcv_us_equity_caps_count_to_200 with the unit marker (e.g. add
`@pytest.mark.unit` above the existing `@pytest.mark.asyncio`) and add
parameter/return type hints (change signature to async def
test_get_ohlcv_us_equity_caps_count_to_200(monkeypatch: pytest.MonkeyPatch) ->
None). Ensure pytest is imported so pytest.MonkeyPatch is available.
- Around line 5759-5774: Add the appropriate pytest marker and type hints for
the async test function test_get_correlation_us_metadata_source_is_kis: decorate
the function with a marker such as `@pytest.mark.unit` (ensure pytest is imported)
and change the signature to include type hints (e.g., monkeypatch:
pytest.MonkeyPatch) and an explicit return type of -> None (async def
test_get_correlation_us_metadata_source_is_kis(monkeypatch: pytest.MonkeyPatch)
-> None:). Keep the existing AsyncMock/monkeypatch usage and other symbols like
build_tools and _patch_runtime_attr unchanged.
- Around line 1288-1325: The new test function
test_get_ohlcv_us_equity_day_with_end_date_bypasses_cache is missing the
required pytest classification marker and lacks type hints on its signature; add
an appropriate pytest marker (e.g., `@pytest.mark.unit` or
`@pytest.mark.integration`) above the test and annotate the async function
signature and any parameters/returns used in the test (for example annotate the
test function parameters if any and specify -> None return) so that test files
follow the project's typing and marker rules; update the test declaration for
test_get_ohlcv_us_equity_day_with_end_date_bypasses_cache and ensure consistency
with other helpers like build_tools, _single_row_df, and
DummyKISClient/inquire_overseas_daily_price usage.
In `@tests/test_services.py`:
- Around line 1062-1107: Add a pytest marker and type hints to the test function
declaration for test_inquire_overseas_daily_price_uses_end_date_as_bymd:
annotate the test's parameters (self, mock_settings, mock_client_class) with
types (e.g., TestCase/self: Any or the test class type, mocks as
MagicMock/AsyncMock or appropriate typing from unittest.mock) and add a return
type of None, and add an appropriate pytest marker decorator (e.g.,
`@pytest.mark.unit`) above the function; ensure the function name and mocked
symbols (KISClient, inquire_overseas_daily_price, mock_client_class) remain
unchanged.
---
Outside diff comments:
In `@app/services/kis.py`:
- Around line 1818-1835: The docstring for the method that accepts parameters
(symbol, exchange_code, n, period, end_date) is missing documentation for the
new end_date parameter; update the Args section to include end_date with its
type (datetime.date | None), default (None), and behavior (the last date to
include in the returned candles; if None uses the most recent available date),
and briefly note how it affects returned data alongside existing params like
symbol, exchange_code, n, period and the return DataFrame (date, open, high,
low, close, volume).
---
Nitpick comments:
In `@tests/test_kis_ohlcv_cache.py`:
- Around line 263-287: Extract the repeated monkeypatch.setattr calls into a
shared pytest fixture named _patch_cache_settings that uses monkeypatch to set
kis_ohlcv_cache.settings.kis_ohlcv_cache_enabled,
kis_ohlcv_cache.settings.kis_ohlcv_cache_ttl_seconds,
kis_ohlcv_cache.settings.kis_ohlcv_cache_max_days,
kis_ohlcv_cache.settings.kis_ohlcv_cache_lock_ttl_seconds, and
kis_ohlcv_cache.settings.kis_ohlcv_cache_probe_retry_seconds to the test
defaults shown; then remove the repeated monkeypatch blocks from the tests and
apply the fixture via `@pytest.mark.usefixtures`("_patch_cache_settings") or by
adding _patch_cache_settings as a test parameter so all tests reuse the same
configuration.
Comment on lines
276
to
+321
| async def _fetch_ohlcv_equity_us( | ||
| symbol: str, count: int, period: str, end_date: datetime.datetime | None | ||
| ) -> dict[str, Any]: | ||
| """Fetch US equity OHLCV from Yahoo Finance.""" | ||
| capped_count = min(count, 100) | ||
| df = await yahoo_service.fetch_ohlcv( | ||
| ticker=symbol, days=capped_count, period=period, end_date=end_date | ||
| ) | ||
| capped_count = min(count, 200) | ||
| kis = KISClient() | ||
| exchange_code = get_exchange_by_symbol(symbol.upper()) or "NASD" | ||
|
|
||
| if period == "day" and end_date is None: | ||
|
|
||
| async def _raw_fetcher( | ||
| *, symbol: str, exchange_code: str, n: int, end_date: datetime.date | None | ||
| ): | ||
| return await kis.inquire_overseas_daily_price( | ||
| symbol=symbol, | ||
| exchange_code=exchange_code, | ||
| n=n, | ||
| period="D", | ||
| end_date=end_date, | ||
| ) | ||
|
|
||
| cached = await kis_ohlcv_cache.get_closed_daily_candles( | ||
| symbol=symbol, | ||
| exchange_code=exchange_code, | ||
| count=capped_count, | ||
| instrument_type="equity_us", | ||
| raw_fetcher=_raw_fetcher, | ||
| ) | ||
| if cached is not None: | ||
| df = cached | ||
| else: | ||
| df = await kis.inquire_overseas_daily_price( | ||
| symbol=symbol, | ||
| exchange_code=exchange_code, | ||
| n=capped_count, | ||
| period="D", | ||
| ) | ||
| else: | ||
| kis_period_map = {"day": "D", "week": "W", "month": "M"} | ||
| df = await kis.inquire_overseas_daily_price( | ||
| symbol=symbol, | ||
| exchange_code=exchange_code, | ||
| n=capped_count, | ||
| period=kis_period_map.get(period, "D"), | ||
| end_date=end_date.date() if end_date else None, | ||
| ) | ||
|
|
There was a problem hiding this comment.
Add a return type hint for the nested _raw_fetcher.
Project guidelines require type hints for all function params/returns.
🔧 Suggested tweak
async def _raw_fetcher(
*, symbol: str, exchange_code: str, n: int, end_date: datetime.date | None
- ):
+ ) -> pd.DataFrame:
return await kis.inquire_overseas_daily_price(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/mcp_server/tooling/market_data_quotes.py` around lines 276 - 321, The
nested function _raw_fetcher inside _fetch_ohlcv_equity_us is missing a return
type hint; update its signature to add an explicit return type (e.g., -> Any or
the precise response type returned by KISClient.inquire_overseas_daily_price)
and ensure parameter types match (symbol: str, exchange_code: str, n: int,
end_date: datetime.date | None) so the nested function has full type
annotations; adjust any imports if you choose a concrete return type and run
type checks to confirm compatibility with
kis_ohlcv_cache.get_closed_daily_candles and
KISClient.inquire_overseas_daily_price.
Comment on lines
+24
to
+38
| def expected_asof_et(now_utc: datetime) -> date: | ||
| base = now_utc | ||
| if base.tzinfo is None: | ||
| base = base.replace(tzinfo=UTC) | ||
| et_now = base.astimezone(_ET) | ||
|
|
||
| anchor = ( | ||
| et_now.date() | ||
| if et_now.time() >= time(16, 0) | ||
| else et_now.date() - timedelta(days=1) | ||
| ) | ||
|
|
||
| while anchor.weekday() >= 5: | ||
| anchor -= timedelta(days=1) | ||
| return anchor |
There was a problem hiding this comment.
Add Google‑style docstrings to the public cache helpers.
expected_asof_et and get_closed_daily_candles are part of the public API surface and should be documented.
📝 Suggested docstrings
def expected_asof_et(now_utc: datetime) -> date:
+ """Compute the expected closed trading date in ET.
+
+ Args:
+ now_utc: Current UTC timestamp.
+
+ Returns:
+ The ET trading date that should be treated as closed.
+ """
base = now_utc async def get_closed_daily_candles(
*,
symbol: str,
exchange_code: str,
count: int,
instrument_type: str,
raw_fetcher: Callable[..., Awaitable[pd.DataFrame]],
now_utc: datetime | None = None,
) -> pd.DataFrame | None:
+ """Return cached daily candles up to the last closed session.
+
+ Args:
+ symbol: Ticker symbol.
+ exchange_code: Exchange code (e.g., NASD).
+ count: Requested candle count.
+ instrument_type: Cache namespace (e.g., equity_us).
+ raw_fetcher: Fallback fetcher used to refresh cache.
+ now_utc: Override for current UTC time.
+
+ Returns:
+ Cached OHLCV frame, or None when cache is disabled/unavailable.
+ """
if not settings.kis_ohlcv_cache_enabled:Also applies to: 340-349
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/services/kis_ohlcv_cache.py` around lines 24 - 38, The public helper
functions expected_asof_et and get_closed_daily_candles lack Google-style
docstrings; add concise Google-style docstrings to each (function summary, Args
with types and meaning, Returns with type and semantics, Raises if any
exceptions can be thrown, and a short Example or Notes for edge cases)
describing their purpose (how expected_asof_et computes the previous ET trading
date given a UTC datetime and how get_closed_daily_candles retrieves/constructs
cached daily OHLCV rows), reference parameter names (now_utc for
expected_asof_et and the public parameters of get_closed_daily_candles), and
document timezone/market-close logic and weekend handling so callers understand
behavior and return values.
Comment on lines
+289
to
+308
| async def _set_probe_meta( | ||
| redis_client: redis.Redis, | ||
| meta_key: str, | ||
| target_asof: date, | ||
| now_utc: datetime, | ||
| ) -> None: | ||
| retry_seconds = max(int(settings.kis_ohlcv_cache_probe_retry_seconds), 1) | ||
| until_ts = int(now_utc.timestamp()) + retry_seconds | ||
| await redis_client.hset( | ||
| meta_key, | ||
| mapping={ | ||
| "last_probe_target_asof": target_asof.isoformat(), | ||
| # Backward compatibility for one release window. | ||
| "last_probe_asof": target_asof.isoformat(), | ||
| "probe_until_ts": str(until_ts), | ||
| "last_probe_ts": str(int(now_utc.timestamp())), | ||
| }, | ||
| ) | ||
| await redis_client.expire(meta_key, max(int(settings.kis_ohlcv_cache_ttl_seconds), 1)) | ||
|
|
There was a problem hiding this comment.
Probe suppression can expire before the retry window.
probe_until_ts can be set for longer than the meta-key TTL (default 1800s vs 300s). If the hash expires early, probe suppression is lost and the system may re-probe too aggressively.
🛠️ Suggested fix
retry_seconds = max(int(settings.kis_ohlcv_cache_probe_retry_seconds), 1)
until_ts = int(now_utc.timestamp()) + retry_seconds
await redis_client.hset(
meta_key,
mapping={
"last_probe_target_asof": target_asof.isoformat(),
# Backward compatibility for one release window.
"last_probe_asof": target_asof.isoformat(),
"probe_until_ts": str(until_ts),
"last_probe_ts": str(int(now_utc.timestamp())),
},
)
- await redis_client.expire(meta_key, max(int(settings.kis_ohlcv_cache_ttl_seconds), 1))
+ ttl_seconds = max(int(settings.kis_ohlcv_cache_ttl_seconds), 1)
+ expire_seconds = max(ttl_seconds, retry_seconds)
+ await redis_client.expire(meta_key, expire_seconds)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/services/kis_ohlcv_cache.py` around lines 289 - 308, In _set_probe_meta
ensure the Redis meta-key TTL cannot be shorter than the probe retry window:
compute retry_seconds from settings (as already done) and set the expire TTL to
max(int(settings.kis_ohlcv_cache_ttl_seconds), retry_seconds, 1) so the stored
probe_until_ts (and related probe fields in _set_probe_meta) will never expire
before the retry window elapses; update the expire call in _set_probe_meta to
use that computed value.
Comment on lines
+194
to
+197
| def test_expected_asof_et_handles_est_edt_and_weekend_rollbacks(): | ||
| assert kis_ohlcv_cache.expected_asof_et( | ||
| datetime(2026, 1, 15, 20, 30, tzinfo=UTC) | ||
| ) == date(2026, 1, 14) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Missing pytest markers on all test functions.
None of the tests in this file have @pytest.mark.unit (or other category markers). The coding guidelines require categorizing tests with @pytest.mark.unit, @pytest.mark.integration, or @pytest.mark.slow.
These are all unit-level tests (no real Redis, no network). Add @pytest.mark.unit to each, or apply it file-wide via pytestmark:
Proposed fix
Add near the top of the file (e.g., after the imports):
pytestmark = pytest.mark.unitAs per coding guidelines, tests/test_*.py: "Use pytest markers (@pytest.mark.unit, @pytest.mark.integration, @pytest.mark.slow) to categorize tests".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_kis_ohlcv_cache.py` around lines 194 - 197, The tests in this file
(e.g., test_expected_asof_et_handles_est_edt_and_weekend_rollbacks) lack pytest
markers; add a file-level marker by defining pytestmark = pytest.mark.unit near
the top of the file (after imports) or alternatively decorate each test with
`@pytest.mark.unit`; ensure pytest is imported in the file (import pytest) if not
already present so the marker resolves.
Comment on lines
+1268
to
+1286
| @pytest.mark.asyncio | ||
| async def test_get_ohlcv_us_equity_caps_count_to_200(monkeypatch): | ||
| tools = build_tools() | ||
| df = _single_row_df() | ||
| cache_mock = AsyncMock(return_value=df) | ||
| monkeypatch.setattr( | ||
| market_data_quotes.kis_ohlcv_cache, | ||
| "get_closed_daily_candles", | ||
| cache_mock, | ||
| ) | ||
|
|
||
| result = await tools["get_ohlcv"]("AAPL", count=999) | ||
|
|
||
| cache_mock.assert_awaited_once() | ||
| assert cache_mock.call_args is not None | ||
| assert cache_mock.call_args.kwargs["count"] == 200 | ||
| assert result["count"] == 200 | ||
| assert result["source"] == "kis" | ||
|
|
There was a problem hiding this comment.
Add required pytest marker + type hints for the new test.
New tests in tests/test_*.py should be categorized and typed per repo standards.
Suggested fix
`@pytest.mark.asyncio`
-async def test_get_ohlcv_us_equity_caps_count_to_200(monkeypatch):
+@pytest.mark.unit
+async def test_get_ohlcv_us_equity_caps_count_to_200(
+ monkeypatch: pytest.MonkeyPatch,
+) -> None:As per coding guidelines: tests/test_*.py: Use pytest markers (@pytest.mark.unit, @pytest.mark.integration, @pytest.mark.slow) to categorize tests; and **/*.py: Use type hints for all function parameters and return types in Python files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_mcp_server_tools.py` around lines 1268 - 1286, Add the required
pytest classification marker and type hints to the test: annotate the async test
function test_get_ohlcv_us_equity_caps_count_to_200 with the unit marker (e.g.
add `@pytest.mark.unit` above the existing `@pytest.mark.asyncio`) and add
parameter/return type hints (change signature to async def
test_get_ohlcv_us_equity_caps_count_to_200(monkeypatch: pytest.MonkeyPatch) ->
None). Ensure pytest is imported so pytest.MonkeyPatch is available.
Comment on lines
+1288
to
+1325
| @pytest.mark.asyncio | ||
| async def test_get_ohlcv_us_equity_day_with_end_date_bypasses_cache(monkeypatch): | ||
| tools = build_tools() | ||
| df = _single_row_df() | ||
| cache_mock = AsyncMock(side_effect=AssertionError("cache should be bypassed")) | ||
| monkeypatch.setattr( | ||
| market_data_quotes.kis_ohlcv_cache, | ||
| "get_closed_daily_candles", | ||
| cache_mock, | ||
| ) | ||
| called: dict[str, object] = {} | ||
|
|
||
| class DummyKISClient: | ||
| async def inquire_overseas_daily_price( | ||
| self, | ||
| symbol: str, | ||
| exchange_code: str = "NASD", | ||
| n: int = 200, | ||
| period: str = "D", | ||
| end_date: date | None = None, | ||
| ): | ||
| called["symbol"] = symbol | ||
| called["exchange_code"] = exchange_code | ||
| called["n"] = n | ||
| called["period"] = period | ||
| called["end_date"] = end_date | ||
| return df | ||
|
|
||
| _patch_runtime_attr(monkeypatch, "KISClient", DummyKISClient) | ||
|
|
||
| result = await tools["get_ohlcv"]("AAPL", count=5, period="day", end_date="2026-01-15") | ||
|
|
||
| cache_mock.assert_not_awaited() | ||
| assert called["symbol"] == "AAPL" | ||
| assert called["period"] == "D" | ||
| assert called["end_date"] == date(2026, 1, 15) | ||
| assert result["source"] == "kis" | ||
|
|
There was a problem hiding this comment.
Add required pytest marker + type hints for the new test.
Same compliance gap here for the new cache-bypass test.
Suggested fix
`@pytest.mark.asyncio`
-async def test_get_ohlcv_us_equity_day_with_end_date_bypasses_cache(monkeypatch):
+@pytest.mark.unit
+async def test_get_ohlcv_us_equity_day_with_end_date_bypasses_cache(
+ monkeypatch: pytest.MonkeyPatch,
+) -> None:As per coding guidelines: tests/test_*.py: Use pytest markers (@pytest.mark.unit, @pytest.mark.integration, @pytest.mark.slow) to categorize tests; and **/*.py: Use type hints for all function parameters and return types in Python files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_mcp_server_tools.py` around lines 1288 - 1325, The new test
function test_get_ohlcv_us_equity_day_with_end_date_bypasses_cache is missing
the required pytest classification marker and lacks type hints on its signature;
add an appropriate pytest marker (e.g., `@pytest.mark.unit` or
`@pytest.mark.integration`) above the test and annotate the async function
signature and any parameters/returns used in the test (for example annotate the
test function parameters if any and specify -> None return) so that test files
follow the project's typing and marker rules; update the test declaration for
test_get_ohlcv_us_equity_day_with_end_date_bypasses_cache and ensure consistency
with other helpers like build_tools, _single_row_df, and
DummyKISClient/inquire_overseas_daily_price usage.
Comment on lines
+5741
to
+5756
| @pytest.mark.asyncio | ||
| async def test_get_support_resistance_us_error_payload_source_is_kis(monkeypatch): | ||
| tools = build_tools() | ||
| _patch_runtime_attr( | ||
| monkeypatch, | ||
| "_fetch_ohlcv_for_indicators", | ||
| AsyncMock(side_effect=RuntimeError("fetch failed")), | ||
| ) | ||
|
|
||
| result = await tools["get_support_resistance"]("AAPL", market="us") | ||
|
|
||
| assert result["source"] == "kis" | ||
| assert result["instrument_type"] == "equity_us" | ||
| assert result["symbol"] == "AAPL" | ||
| assert "fetch failed" in result["error"] | ||
|
|
There was a problem hiding this comment.
Add required pytest marker + type hints for the new test.
This new async test needs the unit marker and parameter typing.
Suggested fix
`@pytest.mark.asyncio`
-async def test_get_support_resistance_us_error_payload_source_is_kis(monkeypatch):
+@pytest.mark.unit
+async def test_get_support_resistance_us_error_payload_source_is_kis(
+ monkeypatch: pytest.MonkeyPatch,
+) -> None:As per coding guidelines: tests/test_*.py: Use pytest markers (@pytest.mark.unit, @pytest.mark.integration, @pytest.mark.slow) to categorize tests; and **/*.py: Use type hints for all function parameters and return types in Python files.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @pytest.mark.asyncio | |
| async def test_get_support_resistance_us_error_payload_source_is_kis(monkeypatch): | |
| tools = build_tools() | |
| _patch_runtime_attr( | |
| monkeypatch, | |
| "_fetch_ohlcv_for_indicators", | |
| AsyncMock(side_effect=RuntimeError("fetch failed")), | |
| ) | |
| result = await tools["get_support_resistance"]("AAPL", market="us") | |
| assert result["source"] == "kis" | |
| assert result["instrument_type"] == "equity_us" | |
| assert result["symbol"] == "AAPL" | |
| assert "fetch failed" in result["error"] | |
| `@pytest.mark.asyncio` | |
| `@pytest.mark.unit` | |
| async def test_get_support_resistance_us_error_payload_source_is_kis( | |
| monkeypatch: pytest.MonkeyPatch, | |
| ) -> None: | |
| tools = build_tools() | |
| _patch_runtime_attr( | |
| monkeypatch, | |
| "_fetch_ohlcv_for_indicators", | |
| AsyncMock(side_effect=RuntimeError("fetch failed")), | |
| ) | |
| result = await tools["get_support_resistance"]("AAPL", market="us") | |
| assert result["source"] == "kis" | |
| assert result["instrument_type"] == "equity_us" | |
| assert result["symbol"] == "AAPL" | |
| assert "fetch failed" in result["error"] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_mcp_server_tools.py` around lines 5741 - 5756, Add the pytest unit
marker and parameter/return type hints to the async test
"test_get_support_resistance_us_error_payload_source_is_kis": add the decorator
`@pytest.mark.unit` above `@pytest.mark.asyncio`, and update the function signature
to include the monkeypatch parameter type and return type (e.g. monkeypatch:
pytest.MonkeyPatch) -> None. Ensure the test still uses build_tools and
_patch_runtime_attr as before and that pytest is imported so pytest.MonkeyPatch
is available.
Comment on lines
+5759
to
+5774
| async def test_get_correlation_us_metadata_source_is_kis(monkeypatch): | ||
| tools = build_tools() | ||
| frame = pd.DataFrame({"close": [100.0, 101.0, 102.0, 103.0]}) | ||
|
|
||
| _patch_runtime_attr( | ||
| monkeypatch, | ||
| "_fetch_ohlcv_for_indicators", | ||
| AsyncMock(return_value=frame), | ||
| ) | ||
|
|
||
| result = await tools["get_correlation"](["AAPL", "MSFT"], period=30) | ||
|
|
||
| assert result["success"] is True | ||
| assert result["metadata"]["sources"]["AAPL"] == "kis" | ||
| assert result["metadata"]["sources"]["MSFT"] == "kis" | ||
|
|
There was a problem hiding this comment.
Add required pytest marker + type hints for the new test.
Same compliance issue for the correlation metadata test.
Suggested fix
`@pytest.mark.asyncio`
-async def test_get_correlation_us_metadata_source_is_kis(monkeypatch):
+@pytest.mark.unit
+async def test_get_correlation_us_metadata_source_is_kis(
+ monkeypatch: pytest.MonkeyPatch,
+) -> None:As per coding guidelines: tests/test_*.py: Use pytest markers (@pytest.mark.unit, @pytest.mark.integration, @pytest.mark.slow) to categorize tests; and **/*.py: Use type hints for all function parameters and return types in Python files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_mcp_server_tools.py` around lines 5759 - 5774, Add the appropriate
pytest marker and type hints for the async test function
test_get_correlation_us_metadata_source_is_kis: decorate the function with a
marker such as `@pytest.mark.unit` (ensure pytest is imported) and change the
signature to include type hints (e.g., monkeypatch: pytest.MonkeyPatch) and an
explicit return type of -> None (async def
test_get_correlation_us_metadata_source_is_kis(monkeypatch: pytest.MonkeyPatch)
-> None:). Keep the existing AsyncMock/monkeypatch usage and other symbols like
build_tools and _patch_runtime_attr unchanged.
Comment on lines
+1062
to
+1107
| @pytest.mark.asyncio | ||
| @patch("app.services.kis.httpx.AsyncClient") | ||
| @patch("app.services.kis.settings") | ||
| async def test_inquire_overseas_daily_price_uses_end_date_as_bymd( | ||
| self, mock_settings, mock_client_class | ||
| ): | ||
| from datetime import date | ||
|
|
||
| from app.services.kis import KISClient | ||
|
|
||
| mock_settings.kis_account_no = "1234567890" | ||
| mock_settings.kis_access_token = "test_token" | ||
|
|
||
| mock_client = AsyncMock() | ||
| mock_client_class.return_value.__aenter__.return_value = mock_client | ||
|
|
||
| mock_response = MagicMock() | ||
| mock_response.status_code = 200 | ||
| mock_response.json.return_value = { | ||
| "rt_cd": "0", | ||
| "output2": [ | ||
| { | ||
| "xymd": "20260115", | ||
| "open": "190.0", | ||
| "high": "191.0", | ||
| "low": "189.0", | ||
| "clos": "190.5", | ||
| "tvol": "100", | ||
| } | ||
| ], | ||
| } | ||
| mock_client.get.return_value = mock_response | ||
|
|
||
| client = KISClient() | ||
| client._ensure_token = AsyncMock(return_value=None) | ||
| client._token_manager = AsyncMock() | ||
|
|
||
| end_date = date(2026, 1, 15) | ||
| result = await client.inquire_overseas_daily_price( | ||
| symbol="AAPL", n=1, end_date=end_date | ||
| ) | ||
|
|
||
| assert len(result) == 1 | ||
| params = mock_client.get.call_args.kwargs["params"] | ||
| assert params["BYMD"] == "20260115" | ||
|
|
There was a problem hiding this comment.
Add a pytest category marker and type hints for the new test.
This keeps the test suite compliant with marker policy and typing requirements.
✅ Suggested update
+ `@pytest.mark.unit`
`@pytest.mark.asyncio`
`@patch`("app.services.kis.httpx.AsyncClient")
`@patch`("app.services.kis.settings")
async def test_inquire_overseas_daily_price_uses_end_date_as_bymd(
- self, mock_settings, mock_client_class
- ):
+ self, mock_settings: MagicMock, mock_client_class: MagicMock
+ ) -> None:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_services.py` around lines 1062 - 1107, Add a pytest marker and
type hints to the test function declaration for
test_inquire_overseas_daily_price_uses_end_date_as_bymd: annotate the test's
parameters (self, mock_settings, mock_client_class) with types (e.g.,
TestCase/self: Any or the test class type, mocks as MagicMock/AsyncMock or
appropriate typing from unittest.mock) and add a return type of None, and add an
appropriate pytest marker decorator (e.g., `@pytest.mark.unit`) above the
function; ensure the function name and mocked symbols (KISClient,
inquire_overseas_daily_price, mock_client_class) remain unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test
Summary by CodeRabbit
New Features
Documentation