Conversation
cameledev
force-pushed
the
add-tests-and-ci-testing
branch
from
666bc78 to
0b0c25d
Compare
cameledev
force-pushed
the
add-tests-and-ci-testing
branch
2 times, most recently
from
867b9fb to
9aa4109
Compare
cameledev
force-pushed
the
add-tests-and-ci-testing
branch
from
9aa4109 to
dd718e7
Compare
FloChehab
left a comment
FloChehab
left a comment
There was a problem hiding this comment.
Quick review as I was digging into summary
| @$(COMPOSE_RUN_APP) pylint meet demo core | ||
| .PHONY: lint-pylint | ||
|
|
||
| test: ## run project tests |
There was a problem hiding this comment.
Could make test also run tests for summary?
There was a problem hiding this comment.
yes. will change
cameledev
force-pushed
the
add-tests-and-ci-testing
branch
from
0f57548 to
1c2a0de
Compare
Update OpenSSL and libssl3t64 package versions to resolve a build failure caused by version regression.
Summary service currently has no tests. Add unit and API tests to summary service.
Add summary service testing to CI.
cameledev
force-pushed
the
add-tests-and-ci-testing
branch
from
7856de6 to
efa0d48
Compare
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThis pull request introduces comprehensive test infrastructure and refactors configuration access patterns across the summary service. The changes include: (1) CI/CD workflow updates renaming the backend test job to "test-backend" and adding a new "test-summary" job; (2) Makefile enhancements adding a test-summary target and database migration improvements; (3) refactoring of direct settings references to dynamic get_settings() calls across 8+ core modules to support environment-specific configurations; (4) introduction of a TestSettings class for test environments; (5) addition of new test dependencies (pytest, responses) and pytest configuration in pyproject.toml; (6) expansion of test suite with package initializers, fixture definitions, and comprehensive unit and integration tests covering health checks, task creation, transcription workflows, and summarization flows. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
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 Tip CodeRabbit can use Trivy to scan for security misconfigurations and secrets in Infrastructure as Code files.Add a .trivyignore file to your project to customize which findings Trivy reports. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/summary/summary/core/file_service.py (1)
173-182:⚠️ Potential issue | 🟡 MinorKeep
metadata["extension"]in sync with the extracted file.When a video is converted, the yielded handle points to
processed_path, butmetadata["extension"]still reports the source video suffix. That makes the metadata inconsistent with the file the caller actually receives.💡 Proposed fix
if extension in get_settings().recording_video_extensions: logger.info("Video file detected, extracting audio...") extracted_audio_path = self._extract_audio_from_video(downloaded_path) processed_path = extracted_audio_path + extension = processed_path.suffix.lower() else: processed_path = downloaded_path metadata = {"duration": duration, "extension": extension}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/summary/summary/core/file_service.py` around lines 173 - 182, The metadata currently uses the original downloaded_path.suffix even when you convert a video to audio; update metadata after possible conversion so it reflects the actual processed file: inside the same method where downloaded_path, processed_path and _extract_audio_from_video are used (the block that checks get_settings().recording_video_extensions and sets processed_path), set metadata["extension"] = processed_path.suffix.lower() (and adjust any other metadata keys if needed) so the yielded handle and metadata remain consistent.src/summary/summary/core/webhook_service.py (2)
49-57:⚠️ Potential issue | 🟠 MajorStop logging raw webhook payloads.
This debug log serializes the full transcript/summary body plus
sub. That's a privacy leak, and it pays the JSON serialization cost even when the payload is large.💡 Proposed fix
- logger.debug("Request payload: %s", json.dumps(data, indent=2)) + logger.debug( + "Webhook submit | content_length=%d has_email=%s has_sub=%s", + len(content), + bool(email), + bool(sub), + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/summary/summary/core/webhook_service.py` around lines 49 - 57, The debug lines in webhook_service.py currently serialize and log the full payload variable `data` (includes `title`, `content`, `email`, `sub`) which leaks sensitive info and costs time; change the `logger.debug` calls that reference `get_settings().webhook_url` and `json.dumps(data, ...)` to avoid dumping raw payloads—either remove the payload log entirely or log a redacted summary (e.g., keys and lengths or masked `email`/`sub`) and avoid calling `json.dumps` on the full `data` object so large bodies are not serialized unnecessarily.
28-39:⚠️ Potential issue | 🟠 MajorAdd a timeout to the webhook POST call.
session.post(url, json=data)will wait indefinitely unless a timeout is specified. A slow or stalled webhook can pin a worker permanently, preventing it from processing other requests even though retries are configured.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/summary/summary/core/webhook_service.py` around lines 28 - 39, In _post_with_retries update the session.post call to include a timeout so the request cannot hang indefinitely: obtain a timeout value (preferably from configuration, e.g., get_settings().webhook_timeout with a sensible default like 10 seconds) and pass it as the timeout argument to session.post(url, json=data, timeout=timeout); ensure the timeout is applied consistently when calling _create_retry_session() and keep existing error handling/raise_for_status behavior intact.
♻️ Duplicate comments (1)
src/summary/summary/core/config.py (1)
95-119:⚠️ Potential issue | 🟠 Major
TestSettingsstill reads process env, so “safe defaults” are not guaranteed.
SettingsConfigDict(env_file=None)stops.envloading only. Environment variables still override defaults, which can accidentally point tests to non-test services/secrets.💡 Proposed hardening
class TestSettings(Settings): """Settings with safe defaults for testing.""" model_config = SettingsConfigDict(env_file=None) + + `@classmethod` + def settings_customise_sources( + cls, + settings_cls, + init_settings, + env_settings, + dotenv_settings, + file_secret_settings, + ): + # Keep tests isolated from host/CI environment variables. + return (init_settings,)Verification (read-only): confirm
TestSettingsdoes not currently customize settings sources.#!/usr/bin/env bash set -euo pipefail python - <<'PY' import ast from pathlib import Path path = Path("src/summary/summary/core/config.py") tree = ast.parse(path.read_text()) for node in tree.body: if isinstance(node, ast.ClassDef) and node.name == "TestSettings": has_method = any( isinstance(item, ast.FunctionDef) and item.name == "settings_customise_sources" for item in node.body ) print(f"TestSettings.settings_customise_sources present: {has_method}") break PY rg -n -C2 'class TestSettings|SettingsConfigDict\(env_file=None\)|settings_customise_sources' src/summary/summary/core/config.pyExpected result:
present: Falseand nosettings_customise_sourcesdefinition.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/summary/summary/core/config.py` around lines 95 - 119, TestSettings currently uses SettingsConfigDict(env_file=None) but still reads process env vars; add a classmethod settings_customise_sources on TestSettings to override Pydantic's sources and exclude env_settings_source (keep init_settings_source and file_secret_settings_source as needed) so defaults can't be overridden by environment variables; reference TestSettings, settings_customise_sources, and SettingsConfigDict(env_file=None) to locate where to add this classmethod.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/agents/Dockerfile`:
- Line 8: The Dockerfile currently runs apt-get upgrade with package names (the
line containing "apt-get upgrade -y openssl libssl3t64"), which unintentionally
upgrades all packages; change that command to use apt-get install --only-upgrade
-y openssl libssl3t64 so only the specified packages are upgraded, preserving
flags and any surrounding apt-get update/clean steps.
In `@src/summary/summary/core/llm_service.py`:
- Around line 34-57: In __init__ of LLM service, cache get_settings() to a local
variable (e.g., settings = get_settings()) and replace repeated calls to
get_settings() inside the masking_function and Langfuse initialization with that
local settings variable; update references used for langfuse_enabled check,
langfuse_environment, langfuse_secret_key, langfuse_public_key, and
langfuse_host so masking_function and the Langfuse(...) call use settings
instead of repeated get_settings() calls (leave function name masking_function
and class Langfuse as-is).
In `@src/summary/tests/integration/test_api_health.py`:
- Around line 1-21: Add the pytest integration marker to these health-check
integration tests by importing pytest and decorating the test classes or their
test methods (e.g., add `@pytest.mark.integration` above class TestHeartbeat and
above class TestLBHeartbeat or above their methods test_returns_200) so they are
discoverable as integration tests; ensure you place the import pytest at the top
of the file and apply the marker consistently to both TestHeartbeat and
TestLBHeartbeat.
In `@src/summary/tests/unit/test_celery_worker.py`:
- Around line 143-157: The test_empty_segments currently uses a single combined
assertion with OR which can mask regressions; update it to assert the two
expectations independently: call format_transcript in the same way and then
assert that the returned content contains the expected empty-transcript message
(e.g., "No audio content") and separately assert that the returned title
contains the expected fallback title (e.g., "Transcription"), referring to the
test function test_empty_segments and the format_transcript function to locate
the code to change.
---
Outside diff comments:
In `@src/summary/summary/core/file_service.py`:
- Around line 173-182: The metadata currently uses the original
downloaded_path.suffix even when you convert a video to audio; update metadata
after possible conversion so it reflects the actual processed file: inside the
same method where downloaded_path, processed_path and _extract_audio_from_video
are used (the block that checks get_settings().recording_video_extensions and
sets processed_path), set metadata["extension"] = processed_path.suffix.lower()
(and adjust any other metadata keys if needed) so the yielded handle and
metadata remain consistent.
In `@src/summary/summary/core/webhook_service.py`:
- Around line 49-57: The debug lines in webhook_service.py currently serialize
and log the full payload variable `data` (includes `title`, `content`, `email`,
`sub`) which leaks sensitive info and costs time; change the `logger.debug`
calls that reference `get_settings().webhook_url` and `json.dumps(data, ...)` to
avoid dumping raw payloads—either remove the payload log entirely or log a
redacted summary (e.g., keys and lengths or masked `email`/`sub`) and avoid
calling `json.dumps` on the full `data` object so large bodies are not
serialized unnecessarily.
- Around line 28-39: In _post_with_retries update the session.post call to
include a timeout so the request cannot hang indefinitely: obtain a timeout
value (preferably from configuration, e.g., get_settings().webhook_timeout with
a sensible default like 10 seconds) and pass it as the timeout argument to
session.post(url, json=data, timeout=timeout); ensure the timeout is applied
consistently when calling _create_retry_session() and keep existing error
handling/raise_for_status behavior intact.
---
Duplicate comments:
In `@src/summary/summary/core/config.py`:
- Around line 95-119: TestSettings currently uses
SettingsConfigDict(env_file=None) but still reads process env vars; add a
classmethod settings_customise_sources on TestSettings to override Pydantic's
sources and exclude env_settings_source (keep init_settings_source and
file_secret_settings_source as needed) so defaults can't be overridden by
environment variables; reference TestSettings, settings_customise_sources, and
SettingsConfigDict(env_file=None) to locate where to add this classmethod.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c7e4930f-24d7-4e9e-8aaf-0c3af8970304
📒 Files selected for processing (22)
.github/workflows/meet.ymlMakefilebin/pytestbin/pytest-summarysrc/agents/Dockerfilesrc/summary/pyproject.tomlsrc/summary/summary/api/route/tasks.pysrc/summary/summary/core/analytics.pysrc/summary/summary/core/celery_worker.pysrc/summary/summary/core/config.pysrc/summary/summary/core/file_service.pysrc/summary/summary/core/llm_service.pysrc/summary/summary/core/transcript_formatter.pysrc/summary/summary/core/webhook_service.pysrc/summary/tests/__init__.pysrc/summary/tests/conftest.pysrc/summary/tests/integration/__init__.pysrc/summary/tests/integration/conftest.pysrc/summary/tests/integration/test_api_health.pysrc/summary/tests/integration/test_api_tasks.pysrc/summary/tests/unit/__init__.pysrc/summary/tests/unit/test_celery_worker.py
| && apt-get install -y --no-install-recommends \ | ||
| libglib2.0-0 \ | ||
| libgobject-2.0-0 \ | ||
| && apt-get upgrade -y openssl libssl3t64 \ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Confirm current Dockerfile commands
rg -n 'apt-get (update|install|upgrade|clean)' src/agents/Dockerfile -C2
# Verify apt-get command semantics in this environment (read-only)
if command -v apt-get >/dev/null 2>&1; then
apt-get --version | head -n 1
echo "=== Simulate current command shape ==="
apt-get -s upgrade openssl libssl3t64 2>&1 | sed -n '1,80p' || true
echo "=== Simulate recommended command shape ==="
apt-get -s install --only-upgrade openssl libssl3t64 2>&1 | sed -n '1,80p' || true
else
echo "apt-get not available in this sandbox."
fiRepository: suitenumerique/meet
Length of output: 395
🌐 Web query:
apt-get upgrade vs install --only-upgrade package names semantics
💡 Result:
-
apt-get upgrade
Upgrades all currently installed packages to the newest available versions but will not install any new packages or remove any packages. Anything that would require new deps (or removals) is kept back and listed. [1] -
apt-get install <pkg> …
For each named package,installmeans “ensure this package is installed at the candidate version”: if it’s not installed, it will be installed; if it is installed, it may be upgraded/downgraded as needed, and APT may install additional dependencies to satisfy it. [2][1] -
apt-get install --only-upgrade <pkg> …
Restrictsinstallso it only upgrades packages that are already installed and ignores requests for packages not currently installed (“do not install new packages”). In practice: good for “upgrade just these packages”, but if upgrading them would require pulling in new dependencies, APT won’t add those new packages under this option, so the upgrade may be refused/kept back. [2]
Rule of thumb:
- Use
apt-get upgradeto do a conservative whole-system upgrade (no new pkgs, no removals). [1] - Use
apt-get install --only-upgrade foo barto target specific installed packages only. [2]
Sources
[1] Debian APT Guide, “apt-get upgrade / install / dist-upgrade” (Chapter 2) (debian.org)
[2] Ubuntu apt-get(8) man page, options install and --only-upgrade (manpages.staging.ubuntu.com)
Citations:
- 1: https://www.debian.org/doc/manuals/apt-guide/ch2.en.html
- 2: https://manpages.staging.ubuntu.com/manpages/oracular/man8/apt-get.8.html
Replace apt-get upgrade with a package-scoped upgrade command.
Line 8 uses apt-get upgrade, which upgrades all currently installed packages regardless of the package names provided (upgrade ignores package arguments). To upgrade only the specified packages, use apt-get install --only-upgrade.
Suggested patch
RUN apt-get update \
&& apt-get install -y --no-install-recommends \
libglib2.0-0 \
libgobject-2.0-0 \
- && apt-get upgrade -y openssl libssl3t64 \
+ && apt-get install -y --no-install-recommends --only-upgrade \
+ openssl \
+ libssl3t64 \
&& apt-get clean \
&& rm -rf /var/lib/apt/lists/*📝 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.
| && apt-get upgrade -y openssl libssl3t64 \ | |
| && apt-get install -y --no-install-recommends --only-upgrade \ | |
| openssl \ | |
| libssl3t64 \ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/agents/Dockerfile` at line 8, The Dockerfile currently runs apt-get
upgrade with package names (the line containing "apt-get upgrade -y openssl
libssl3t64"), which unintentionally upgrades all packages; change that command
to use apt-get install --only-upgrade -y openssl libssl3t64 so only the
specified packages are upgraded, preserving flags and any surrounding apt-get
update/clean steps.
| if get_settings().langfuse_enabled: | ||
|
|
||
| def masking_function(data, **kwargs): | ||
| if ( | ||
| user_has_tracing_consent | ||
| or settings.langfuse_environment != "production" | ||
| or get_settings().langfuse_environment != "production" | ||
| ): | ||
| return data | ||
|
|
||
| return "[REDACTED]" | ||
|
|
||
| if not settings.langfuse_secret_key: | ||
| if not get_settings().langfuse_secret_key: | ||
| raise ValueError( | ||
| "langfuse_secret_key is not configured. " | ||
| "Please set the secret key or disable Langfuse." | ||
| ) | ||
|
|
||
| self._observability_client = Langfuse( | ||
| secret_key=settings.langfuse_secret_key.get_secret_value(), | ||
| public_key=settings.langfuse_public_key, | ||
| host=settings.langfuse_host, | ||
| environment=settings.langfuse_environment, | ||
| secret_key=get_settings().langfuse_secret_key.get_secret_value(), | ||
| public_key=get_settings().langfuse_public_key, | ||
| host=get_settings().langfuse_host, | ||
| environment=get_settings().langfuse_environment, | ||
| mask=masking_function, | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider caching get_settings() in a local variable within __init__.
Multiple calls to get_settings() within the same method (lines 34, 39, 45, 52-55) could be consolidated into a single local variable for improved readability. While @lru_cache ensures no performance penalty, a local reference makes the code cleaner.
♻️ Optional refactor
def __init__(
self,
session_id: str,
user_id: str,
user_has_tracing_consent: bool = False,
):
"""Initialize the LLMObservability client."""
self._observability_client: Optional[Langfuse] = None
self.session_id = session_id
self.user_id = user_id
+ settings = get_settings()
- if get_settings().langfuse_enabled:
+ if settings.langfuse_enabled:
def masking_function(data, **kwargs):
if (
user_has_tracing_consent
- or get_settings().langfuse_environment != "production"
+ or settings.langfuse_environment != "production"
):
return data
return "[REDACTED]"
- if not get_settings().langfuse_secret_key:
+ if not settings.langfuse_secret_key:
raise ValueError(
"langfuse_secret_key is not configured. "
"Please set the secret key or disable Langfuse."
)
self._observability_client = Langfuse(
- secret_key=get_settings().langfuse_secret_key.get_secret_value(),
- public_key=get_settings().langfuse_public_key,
- host=get_settings().langfuse_host,
- environment=get_settings().langfuse_environment,
+ secret_key=settings.langfuse_secret_key.get_secret_value(),
+ public_key=settings.langfuse_public_key,
+ host=settings.langfuse_host,
+ environment=settings.langfuse_environment,
mask=masking_function,
)📝 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.
| if get_settings().langfuse_enabled: | |
| def masking_function(data, **kwargs): | |
| if ( | |
| user_has_tracing_consent | |
| or settings.langfuse_environment != "production" | |
| or get_settings().langfuse_environment != "production" | |
| ): | |
| return data | |
| return "[REDACTED]" | |
| if not settings.langfuse_secret_key: | |
| if not get_settings().langfuse_secret_key: | |
| raise ValueError( | |
| "langfuse_secret_key is not configured. " | |
| "Please set the secret key or disable Langfuse." | |
| ) | |
| self._observability_client = Langfuse( | |
| secret_key=settings.langfuse_secret_key.get_secret_value(), | |
| public_key=settings.langfuse_public_key, | |
| host=settings.langfuse_host, | |
| environment=settings.langfuse_environment, | |
| secret_key=get_settings().langfuse_secret_key.get_secret_value(), | |
| public_key=get_settings().langfuse_public_key, | |
| host=get_settings().langfuse_host, | |
| environment=get_settings().langfuse_environment, | |
| mask=masking_function, | |
| ) | |
| settings = get_settings() | |
| if settings.langfuse_enabled: | |
| def masking_function(data, **kwargs): | |
| if ( | |
| user_has_tracing_consent | |
| or settings.langfuse_environment != "production" | |
| ): | |
| return data | |
| return "[REDACTED]" | |
| if not settings.langfuse_secret_key: | |
| raise ValueError( | |
| "langfuse_secret_key is not configured. " | |
| "Please set the secret key or disable Langfuse." | |
| ) | |
| self._observability_client = Langfuse( | |
| secret_key=settings.langfuse_secret_key.get_secret_value(), | |
| public_key=settings.langfuse_public_key, | |
| host=settings.langfuse_host, | |
| environment=settings.langfuse_environment, | |
| mask=masking_function, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/summary/summary/core/llm_service.py` around lines 34 - 57, In __init__ of
LLM service, cache get_settings() to a local variable (e.g., settings =
get_settings()) and replace repeated calls to get_settings() inside the
masking_function and Langfuse initialization with that local settings variable;
update references used for langfuse_enabled check, langfuse_environment,
langfuse_secret_key, langfuse_public_key, and langfuse_host so masking_function
and the Langfuse(...) call use settings instead of repeated get_settings() calls
(leave function name masking_function and class Langfuse as-is).
| """Integration tests for the health check endpoints.""" | ||
|
|
||
|
|
||
| class TestHeartbeat: | ||
| """Tests for the /__heartbeat__ endpoint.""" | ||
|
|
||
| def test_returns_200(self, client): | ||
| """The heartbeat endpoint responds with 200 OK without a token.""" | ||
| response = client.get("/__heartbeat__") | ||
|
|
||
| assert response.status_code == 200 | ||
|
|
||
|
|
||
| class TestLBHeartbeat: | ||
| """Tests for the /__lbheartbeat__ endpoint.""" | ||
|
|
||
| def test_returns_200(self, client): | ||
| """The load-balancer heartbeat endpoint responds with 200 OK without a token.""" | ||
| response = client.get("/__lbheartbeat__") | ||
|
|
||
| assert response.status_code == 200 |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding @pytest.mark.integration marker.
The pyproject.toml defines markers for unit and integration tests. Adding the @pytest.mark.integration decorator would allow selective test execution and maintain consistency with the defined test categorization.
♻️ Proposed fix to add integration marker
"""Integration tests for the health check endpoints."""
+
+import pytest
+@pytest.mark.integration
class TestHeartbeat:
"""Tests for the /__heartbeat__ endpoint."""
def test_returns_200(self, client):
"""The heartbeat endpoint responds with 200 OK without a token."""
response = client.get("/__heartbeat__")
assert response.status_code == 200
+@pytest.mark.integration
class TestLBHeartbeat:
"""Tests for the /__lbheartbeat__ endpoint."""
def test_returns_200(self, client):
"""The load-balancer heartbeat endpoint responds with 200 OK without a token."""
response = client.get("/__lbheartbeat__")
assert response.status_code == 200📝 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.
| """Integration tests for the health check endpoints.""" | |
| class TestHeartbeat: | |
| """Tests for the /__heartbeat__ endpoint.""" | |
| def test_returns_200(self, client): | |
| """The heartbeat endpoint responds with 200 OK without a token.""" | |
| response = client.get("/__heartbeat__") | |
| assert response.status_code == 200 | |
| class TestLBHeartbeat: | |
| """Tests for the /__lbheartbeat__ endpoint.""" | |
| def test_returns_200(self, client): | |
| """The load-balancer heartbeat endpoint responds with 200 OK without a token.""" | |
| response = client.get("/__lbheartbeat__") | |
| assert response.status_code == 200 | |
| """Integration tests for the health check endpoints.""" | |
| import pytest | |
| `@pytest.mark.integration` | |
| class TestHeartbeat: | |
| """Tests for the /__heartbeat__ endpoint.""" | |
| def test_returns_200(self, client): | |
| """The heartbeat endpoint responds with 200 OK without a token.""" | |
| response = client.get("/__heartbeat__") | |
| assert response.status_code == 200 | |
| `@pytest.mark.integration` | |
| class TestLBHeartbeat: | |
| """Tests for the /__lbheartbeat__ endpoint.""" | |
| def test_returns_200(self, client): | |
| """The load-balancer heartbeat endpoint responds with 200 OK without a token.""" | |
| response = client.get("/__lbheartbeat__") | |
| assert response.status_code == 200 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/summary/tests/integration/test_api_health.py` around lines 1 - 21, Add
the pytest integration marker to these health-check integration tests by
importing pytest and decorating the test classes or their test methods (e.g.,
add `@pytest.mark.integration` above class TestHeartbeat and above class
TestLBHeartbeat or above their methods test_returns_200) so they are
discoverable as integration tests; ensure you place the import pytest at the top
of the file and apply the marker consistently to both TestHeartbeat and
TestLBHeartbeat.
| def test_empty_segments(self): | ||
| """Returns empty-transcription message when there are no segments.""" | ||
| transcription = {"segments": []} | ||
|
|
||
| content, title = format_transcript( | ||
| transcription, | ||
| context_language="en", | ||
| language="en", | ||
| room=None, | ||
| recording_date=None, | ||
| recording_time=None, | ||
| download_link=None, | ||
| ) | ||
|
|
||
| assert "No audio content" in content or "Transcription" in title |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Strengthen the empty-transcript assertion.
The or here makes the test pass when only one fallback survives, so it can miss regressions in the empty-state contract. Please assert the intended content and title independently.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/summary/tests/unit/test_celery_worker.py` around lines 143 - 157, The
test_empty_segments currently uses a single combined assertion with OR which can
mask regressions; update it to assert the two expectations independently: call
format_transcript in the same way and then assert that the returned content
contains the expected empty-transcript message (e.g., "No audio content") and
separately assert that the returned title contains the expected fallback title
(e.g., "Transcription"), referring to the test function test_empty_segments and
the format_transcript function to locate the code to change.
| cache: "pip" | ||
|
|
||
| - name: Install development dependencies | ||
| run: pip install --user .[dev] |
There was a problem hiding this comment.
why do we need the --user arg ?
| -e DJANGO_CONFIGURATION=Test \ | ||
| app-dev \ | ||
| pytest "$@" | ||
| pytest "$@" |
There was a problem hiding this comment.
nit: is this change necessary?
| @@ -0,0 +1,7 @@ | |||
| #!/usr/bin/env bash | |||
|
|
|||
There was a problem hiding this comment.
| # NB: this file is used locally only. In CI, it is overwritten by pytest install |
|
|
||
|
|
||
| def format_actions(llm_output: dict) -> str: | ||
| def _format_actions(llm_output: dict) -> str: |
There was a problem hiding this comment.
It is a nice improvement, but it's a behavioural change bundled into a test PR. It should ideally be a separate commit or at least explicitly noted next time.
| access_key=get_settings().aws_s3_access_key_id, | ||
| secret_key=get_settings().aws_s3_secret_access_key.get_secret_value(), | ||
| secure=get_settings().aws_s3_secure_access, | ||
| ) |
There was a problem hiding this comment.
it feels weird to call the function several times per scope, even if it's a singleton.
| if os.environ.get("SUMMARY_ENV") == "test": | ||
| return TestSettings() |
There was a problem hiding this comment.
I'm not huge fan of it, as it's a test-only flag in production code. That's test-induced damage.
| if get_settings().langfuse_enabled: | ||
|
|
||
| def masking_function(data, **kwargs): | ||
| if ( | ||
| user_has_tracing_consent | ||
| or settings.langfuse_environment != "production" | ||
| or get_settings().langfuse_environment != "production" | ||
| ): | ||
| return data | ||
|
|
||
| return "[REDACTED]" | ||
|
|
||
| if not settings.langfuse_secret_key: | ||
| if not get_settings().langfuse_secret_key: | ||
| raise ValueError( | ||
| "langfuse_secret_key is not configured. " | ||
| "Please set the secret key or disable Langfuse." | ||
| ) | ||
|
|
||
| self._observability_client = Langfuse( | ||
| secret_key=settings.langfuse_secret_key.get_secret_value(), | ||
| public_key=settings.langfuse_public_key, | ||
| host=settings.langfuse_host, | ||
| environment=settings.langfuse_environment, | ||
| secret_key=get_settings().langfuse_secret_key.get_secret_value(), | ||
| public_key=get_settings().langfuse_public_key, | ||
| host=get_settings().langfuse_host, | ||
| environment=get_settings().langfuse_environment, | ||
| mask=masking_function, | ||
| ) |
|
Partially implemented in #1162 , skipping the integration tests included here. |
3 participants
Purpose