Skip to content

fix(logging): fix f-string logger calls and add missing boundary tests#28

Open
Kunal-Somani wants to merge 1 commit intoruxailab:mainfrom
Kunal-Somani:fix/logger-formatting-and-audio-boundary-tests
Open

fix(logging): fix f-string logger calls and add missing boundary tests#28
Kunal-Somani wants to merge 1 commit intoruxailab:mainfrom
Kunal-Somani:fix/logger-formatting-and-audio-boundary-tests

Conversation

@Kunal-Somani
Copy link
Copy Markdown

Summary

Two issues found across the service layer:

Issue 1 — f-string logger anti-pattern in AudioService and TranscriptService

Both services were using f-strings inside logger.error():

# Before (incorrect)
logger.error(f"[error] [Service Layer] [AudioService] ... {str(e)}")

Python's logging module performs lazy string interpolation only when %s placeholders are used. Passing an f-string means the string is always constructed, even when the ERROR level is disabled — wasting CPU on every exception path.

# After (correct)
logger.error(
    "[Service Layer] [AudioService] [extract_audio] An error occurred: %s",
    str(e)
)

This is consistent with the fix applied to AudioTranscriptionSentimentPipeline in PR #27.

Issue 2 — Three missing boundary tests in TestAudioService

The original test suite left three edge cases completely untested:

1. start_time_ms=0
The guard clause rejects start_time_ms < 0, making 0 the valid lower boundary. No test verified that 0 passes through and produces a correct result.

2. start_time_ms as float
The validation explicitly uses isinstance(start_time_ms, (int, float)) allowing floats, but no test ever passed a float value through the full extract_audio flow.

3. user_id=None through extract_audio
The _save_audio unit tests cover the None branch internally, but extract_audio was never called with user_id=None end-to-end. This left the forwarding of None to _save_audio unverified.

Files Changed

  • app/services/audio_service.py
  • app/services/transcript_service.py
  • tests/unit/test_services/test_audio_service.py

- Fix logger.error() calls in AudioService and TranscriptService that
  were using f-strings with str(e) — replaced with % formatting which
  is Python logging best practice and avoids string interpolation cost
  when the log level is disabled

- Add three boundary tests to TestAudioService that were missing from
  the original test suite:
    * test_extract_audio_start_time_zero: start_time_ms=0 is a valid
      lower boundary but was never tested — the guard only rejects < 0
    * test_extract_audio_float_start_time: validation explicitly allows
      isinstance(start_time_ms, (int, float)) but no test exercised a
      float value passing through successfully
    * test_extract_audio_no_user_id: _save_audio unit tests cover the
      None branch internally but extract_audio was never called with
      user_id=None end-to-end — verifies None is forwarded correctly
      to _save_audio rather than silently dropped
Kunal-Somani added a commit to Kunal-Somani/sentiment-analysis-api that referenced this pull request Mar 22, 2026
…point

The original input guard used 'if not text' which correctly rejects
None and empty strings but silently passes whitespace-only input
(e.g. '   ') straight to the ML model as valid text. This causes the
model to run inference on meaningless input with no error returned to
the caller.

Fix: replace 'if not text' with 'if not text or not text.strip()' so
whitespace-only strings are rejected with a 400 Bad Request — consistent
with how missing and empty text is already handled.

Also fix logger.error() f-string anti-pattern in the exception handler,
replacing it with % formatting for consistency with the rest of the
service layer (PRs ruxailab#27, ruxailab#28).

Tests added:
- test_sentiment_analyze_whitespace_only_text: verifies '   ' returns
  400 and service is never called
- test_sentiment_analyze_empty_string_text: verifies '' returns 400
  and service is never called
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