Conversation
tmikula-dev
requested review from
lsulak,
oto-macenauer-absa and
petr-pokorny-absa
as code owners
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a full integration test harness and CI split: container-backed fixtures (Postgres, Kafka, LocalStack), in-process mock JWT server, EventGateTestClient for direct lambda invocation, new integration test suites, CI workflow split (mypy / unit / integration), dependency updates, docs/config tweaks, and unit test reorganization. Changes
Sequence Diagram(s)sequenceDiagram
actor TestRunner
participant Fixture as "Fixture Setup (pytest)"
participant MockJWT as "Mock JWT Server"
participant Postgres as "Postgres (container)"
participant Kafka as "Kafka (container)"
participant LocalStack as "LocalStack (container)"
participant Lambda as "Lambda Handler (imported)"
participant TestClient as "EventGateTestClient"
participant Verifier as "Verification Layer"
TestRunner->>Fixture: start integration tests
Fixture->>Postgres: start container & apply schema
Fixture->>Kafka: start container
Fixture->>LocalStack: start container
Fixture->>MockJWT: start in-process JWT server
Fixture->>Lambda: import handler with env vars pointing to containers
TestRunner->>TestClient: construct client using handler
TestRunner->>TestClient: POST /topics/{topic} with token & payload
TestClient->>Lambda: invoke handler (API Gateway event)
Lambda->>MockJWT: validate token signature
MockJWT-->>Lambda: validation result
Lambda->>LocalStack: read/store secrets, call EventBridge
Lambda->>Postgres: insert event rows
Lambda->>Kafka: publish message
Lambda-->>TestClient: return 202 response
TestRunner->>Verifier: check Postgres, Kafka, EventBridge for records
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (6)
requirements.txt (1)
15-16: Test-only dependencies (testcontainers,docker) follow the existing pattern but will bloat Lambda packages.These are only needed for integration tests, not at runtime. This is a pre-existing issue (pytest, pylint, etc. are also here), but adding heavier packages like
dockerincreases the deployment artifact size. Consider splitting intorequirements.txt(runtime) andrequirements-dev.txt(test/dev) in a follow-up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@requirements.txt` around lines 15 - 16, requirements.txt currently contains test-only packages (testcontainers, docker) that bloat Lambda artifacts; remove these test/dev-only dependencies from requirements.txt and move them into a new requirements-dev.txt (or requirements.test.txt) listing testcontainers==4.9.0, docker==7.1.0 and other dev packages (pytest, pylint, etc.), update CI/test scripts to pip install -r requirements-dev.txt for test runs, and ensure deployment/package steps install only requirements.txt so Lambda artifacts stay lean..github/workflows/check_python.yml (1)
163-186: Integration tests may be flaky due to Docker image pulls — consider pre-pulling or caching.The 15-minute timeout is good. However, pulling
postgres:16,cp-kafka:7.6.0, andlocalstack/localstack:lateston every run can be slow and subject to registry rate limits or transient failures. Consider:
- Pinning
localstackto a specific tag instead oflatestfor reproducibility.- Adding a Docker layer cache step if flakiness becomes an issue.
This isn't blocking — just something to monitor.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/check_python.yml around lines 163 - 186, Update the integration-tests job to reduce flaky Docker pulls by (1) replacing the localstack image tag "latest" with a fixed, specific tag (e.g., "localstack/localstack:2.2.0" or another vetted version) wherever that image is referenced, and (2) adding a pre-pull step in the integration-tests job (before the "Run integration tests" step) that runs docker pull for the images used (postgres:16, cp-kafka:7.6.0, and the pinned localstack tag) so the images are fetched upfront; locate the integration-tests job and its steps (job name: integration-tests, step names: "Set up Python", "Install dependencies", "Run integration tests") and insert the new docker pull step and update the localstack tag accordingly.tests/integration/conftest.py (2)
154-162: Minor: unused variable and list concatenation style.Ruff flagged
image_nameon line 157 as unused — prefix with_. Also, line 162 can use spread syntax.Proposed fix
for task in as_completed(pull_tasks): - image_name, success, message = task.result() + _, success, message = task.result() if not success: failures.append(message) if failures: - pytest.exit("\n".join(["Docker image pre-pull failed:"] + failures), returncode=1) + pytest.exit("\n".join(["Docker image pre-pull failed:", *failures]), returncode=1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/conftest.py` around lines 154 - 162, Rename the unused loop variable image_name returned by _pull_image_with_retry to _image_name to satisfy Ruff, and change the final pytest.exit call to use list unpacking instead of concatenation (e.g. ["Docker image pre-pull failed:", *failures]) so the failure messages are expanded into the list before joining; update references in the block that create pull_tasks and iterate over as_completed(pull_tasks) accordingly.
98-103: Unpinnedlocalstack/localstack:latestreduces reproducibility.All other images are version-pinned, but LocalStack uses
:latest. This can cause flaky CI when LocalStack publishes a breaking release. Consider pinning to a specific version.Proposed fix
CONTAINER_IMAGES: List[str] = [ "testcontainers/ryuk:0.8.1", "postgres:16", "confluentinc/cp-kafka:7.6.0", - "localstack/localstack:latest", + "localstack/localstack:4.1.0", # pin to a specific version ]Also update line 215 to match:
- container = LocalStackContainer("localstack/localstack:latest") + container = LocalStackContainer("localstack/localstack:4.1.0")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/conftest.py` around lines 98 - 103, Replace the unpinned LocalStack image in the CONTAINER_IMAGES list (currently "localstack/localstack:latest") with a specific, version-pinned tag (e.g., "localstack/localstack:<desired-version>") to ensure reproducible CI, and update the other LocalStack image reference later in the file (the matching LocalStack entry around line 215) to use the same pinned tag so both places reference the identical, versioned image.tests/integration/schemas/postgres_schema.py (1)
19-29: Consider adding a PRIMARY KEY onevent_id.The test table has no primary key or unique constraint on
event_id. While this works for integration tests, addingPRIMARY KEY (event_id)would help catch duplicate-write bugs during testing, making the assertions intest_event_received_by_postgresmore meaningful.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/schemas/postgres_schema.py` around lines 19 - 29, The CREATE TABLE in SCHEMA_SQL for public_cps_za_test lacks a primary key; update the SCHEMA_SQL string so the table definition declares event_id as the primary key (e.g., add "PRIMARY KEY (event_id)" in the CREATE TABLE statement or mark event_id as "VARCHAR(255) NOT NULL PRIMARY KEY") to ensure duplicate-write issues surface during tests and make test_event_received_by_postgres assertions more meaningful.tests/integration/test_topics_endpoint.py (1)
171-199: Kafka verification is solid; consider logging swallowed errors.The consumer correctly uses
auto.offset.reset: earliestwith a uniquegroup.id. The 10-second polling deadline is reasonable. One minor note: line 190 silently swallowsmsg.error()— for diagnostic purposes during test failures, you might want to log those errors.Optional: log Kafka consumer errors
if msg.error(): + import logging + logging.getLogger(__name__).debug("Kafka consumer error: %s", msg.error()) continue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_topics_endpoint.py` around lines 171 - 199, The test_event_received_by_kafka test currently ignores Kafka consumer errors in the poll loop (the msg.error() branch); update the loop in test_event_received_by_kafka to log the error before continuing so diagnostics are available on failure: obtain a logger (e.g., logging.getLogger(__name__) or reuse an existing test logger), call logger.error(...) or logger.warning(...) with the msg.error() details when msg.error() is truthy, then continue; ensure any necessary import (import logging) is added and no test behavior changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/copilot-instructions.md:
- Line 35: The guidance line "Mirror src structure: `src/handlers/` ->
`tests/handlers/`" is stale; update it to reflect the new test layout by
changing the target path to `tests/unit/handlers/` (i.e., "Mirror src structure:
`src/handlers/` -> `tests/unit/handlers/`") so the documentation matches the
moved unit tests; locate that exact string in .github/copilot-instructions.md
and replace it accordingly.
In `@DEVELOPER.md`:
- Around line 112-115: The documentation text under the pytest command is
inconsistent: the command shown is "pytest tests/unit/" but the following
sentence says it runs "all tests located in the tests directory." Update the
sentence to accurately describe the command (e.g., "This will execute all unit
tests located in the tests/unit directory."), editing the paragraph that follows
the "pytest tests/unit/" line in DEVELOPER.md so the description matches the
command.
In `@tests/integration/conftest.py`:
- Around line 246-251: The health-check loop around req_lib.get against
f"{url}/health" may exit normally after 10 failures and still yield an unhealthy
URL; change it to fail explicitly when no attempt succeeds by detecting the
absence of a successful break (e.g., use the for/else pattern or a boolean flag)
and raise an exception (RuntimeError/AssertionError) with a clear message
including the url so the fixture does not yield an unreachable server; update
the block that calls req_lib.get, the loop, and the surrounding fixture logic to
raise on total failure.
- Line 2: Update the copyright header year from 2025 to 2026 in the new test
files; specifically edit the top-of-file header in conftest.py (and the other
new files in this PR: postgres_schema.py, test_health_endpoint.py,
test_token_endpoint.py, test_api_endpoint.py, test_topics_endpoint.py) so the
header reads "Copyright 2026 ABSA Group Limited".
- Around line 269-345: The fixture leaks several environment variables and
leaves imported module state cached; update the teardown to pop all env vars you
set (LOG_LEVEL, AWS_ENDPOINT_URL, AWS_DEFAULT_REGION, AWS_ACCESS_KEY_ID,
AWS_SECRET_ACCESS_KEY, CONF_DIR, POSTGRES_SECRET_NAME, POSTGRES_SECRET_REGION)
and also reset the imported module state by
importlib.reload(src.event_gate_lambda) (or delete it from sys.modules) after
yield so lambda_handler's module-level singletons/config are not reused across
tests; locate the imports/keys around the yield of lambda_handler in this
conftest fixture and add these cleanup steps.
In `@tests/integration/test_topics_endpoint.py`:
- Around line 201-212: The test test_event_received_by_eventbridge currently
only calls client.describe_event_bus and never uses the posted_event fixture
(posted_event) so it doesn't verify delivery; either remove the posted_event
parameter and rename the test to indicate it's a connectivity smoke test, or
(preferred) modify the test to create an EventBridge rule with an SQS target,
create an SQS queue, subscribe that queue as the target via put_targets, ensure
the rule is enabled, then poll the queue (using boto3 SQS client) for a message
matching posted_event and assert the event payload is present; update
setup/teardown in the test to delete the rule, target and queue afterwards.
Ensure you reference test_event_received_by_eventbridge, posted_event and
localstack_container when locating the code to change.
In `@tests/integration/utils/jwt_helper.py`:
- Around line 1-15: Update the copyright header in
tests/integration/utils/jwt_helper.py to use the creation year 2026 (replace
"2025" with "2026") so the file header reads "Copyright 2026 ABSA Group
Limited"; ensure the rest of the Apache License header remains unchanged.
In `@tests/unit/handlers/__init__.py`:
- Line 2: The file header currently uses the wrong year; update the top-line
copyright string in the file (the existing header line) from "Copyright 2025
ABSA Group Limited" to "Copyright 2026 ABSA Group Limited" so the header
reflects the file's 2026 creation year.
In `@tests/unit/utils/__init__.py`:
- Line 2: The copyright header in tests/unit/utils/__init__.py uses the year
2025 but this file was created in 2026; update the header line to read
"Copyright 2026 ABSA Group Limited" so the file's copyright year matches its
creation year.
In `@tests/unit/writers/__init__.py`:
- Line 2: Update the copyright header in the file to reflect the correct
creation year: change the single-line header currently reading "Copyright 2025
ABSA Group Limited" to "Copyright 2026 ABSA Group Limited" so it matches the
2026 file creation policy (same fix as applied in
tests/unit/handlers/__init__.py).
---
Nitpick comments:
In @.github/workflows/check_python.yml:
- Around line 163-186: Update the integration-tests job to reduce flaky Docker
pulls by (1) replacing the localstack image tag "latest" with a fixed, specific
tag (e.g., "localstack/localstack:2.2.0" or another vetted version) wherever
that image is referenced, and (2) adding a pre-pull step in the
integration-tests job (before the "Run integration tests" step) that runs docker
pull for the images used (postgres:16, cp-kafka:7.6.0, and the pinned localstack
tag) so the images are fetched upfront; locate the integration-tests job and its
steps (job name: integration-tests, step names: "Set up Python", "Install
dependencies", "Run integration tests") and insert the new docker pull step and
update the localstack tag accordingly.
In `@requirements.txt`:
- Around line 15-16: requirements.txt currently contains test-only packages
(testcontainers, docker) that bloat Lambda artifacts; remove these test/dev-only
dependencies from requirements.txt and move them into a new requirements-dev.txt
(or requirements.test.txt) listing testcontainers==4.9.0, docker==7.1.0 and
other dev packages (pytest, pylint, etc.), update CI/test scripts to pip install
-r requirements-dev.txt for test runs, and ensure deployment/package steps
install only requirements.txt so Lambda artifacts stay lean.
In `@tests/integration/conftest.py`:
- Around line 154-162: Rename the unused loop variable image_name returned by
_pull_image_with_retry to _image_name to satisfy Ruff, and change the final
pytest.exit call to use list unpacking instead of concatenation (e.g. ["Docker
image pre-pull failed:", *failures]) so the failure messages are expanded into
the list before joining; update references in the block that create pull_tasks
and iterate over as_completed(pull_tasks) accordingly.
- Around line 98-103: Replace the unpinned LocalStack image in the
CONTAINER_IMAGES list (currently "localstack/localstack:latest") with a
specific, version-pinned tag (e.g., "localstack/localstack:<desired-version>")
to ensure reproducible CI, and update the other LocalStack image reference later
in the file (the matching LocalStack entry around line 215) to use the same
pinned tag so both places reference the identical, versioned image.
In `@tests/integration/schemas/postgres_schema.py`:
- Around line 19-29: The CREATE TABLE in SCHEMA_SQL for public_cps_za_test lacks
a primary key; update the SCHEMA_SQL string so the table definition declares
event_id as the primary key (e.g., add "PRIMARY KEY (event_id)" in the CREATE
TABLE statement or mark event_id as "VARCHAR(255) NOT NULL PRIMARY KEY") to
ensure duplicate-write issues surface during tests and make
test_event_received_by_postgres assertions more meaningful.
In `@tests/integration/test_topics_endpoint.py`:
- Around line 171-199: The test_event_received_by_kafka test currently ignores
Kafka consumer errors in the poll loop (the msg.error() branch); update the loop
in test_event_received_by_kafka to log the error before continuing so
diagnostics are available on failure: obtain a logger (e.g.,
logging.getLogger(__name__) or reuse an existing test logger), call
logger.error(...) or logger.warning(...) with the msg.error() details when
msg.error() is truthy, then continue; ensure any necessary import (import
logging) is added and no test behavior changes.
# Conflicts: # .github/copilot-instructions.md
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/integration/conftest.py (2)
329-330: Deferred import inside fixture is intentional but violates the coding guideline.The guideline states "Place all imports at the top of file, never inside functions." Here the import is deliberately deferred until after environment variables are set. This is a valid integration-test pattern, but consider adding an inline comment explaining why to prevent future reviewers from "fixing" it. As per coding guidelines,
**/*.py: "Place all imports at the top of file, never inside functions."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/conftest.py` around lines 329 - 330, Add an inline comment next to the deferred import of lambda_handler explaining that the import is intentionally deferred until after environment variables are set for integration tests to avoid violating the "imports at top" guideline; locate the import line "from src.event_gate_lambda import lambda_handler" in tests/integration/conftest.py and insert a brief comment referencing that the import is deferred on purpose (e.g., to allow setting environment variables/mocking before module initialization) so future reviewers won't move it to module scope.
50-54: Class-level mutable state onMockJWTHandleris shared across all instances.Setting
private_key_pemandpublic_key_b64as class attributes and mutating them in_start_mock_jwt_server(lines 87–88) means any subsequent call with different keys would silently change the credentials for an already-running server. This is safe today because there's only one session-scoped mock server, but it's a fragile pattern. A minor defensive improvement would be to pass them via the server instance or a closure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/conftest.py` around lines 50 - 54, MockJWTHandler currently stores mutable keys as class attributes (private_key_pem, public_key_b64) which are shared across instances; instead, stop using class-level mutable state by passing the keys into the handler via the server instance or a closure created in _start_mock_jwt_server: attach the keys to the HTTPServer instance (e.g., server.private_key_pem/server.public_key_b64) and have MockJWTHandler read them from self.server, or create a handler factory/closure that binds the keys (using functools.partial or a nested class) so each server instance gets its own immutable key values when started.tests/integration/test_topics_endpoint.py (1)
171-199: Kafka consumer poll loop could benefit from a clearer timeout message.If the 10-second deadline passes without finding the event, the test falls through to
assert 1 == len(matched)which will fail with a generic assertion error (e.g.,assert 1 == 0). A small improvement would be to add a descriptive message.Proposed fix
matched = [m for m in messages if m["event_id"] == posted_event["event_id"]] - assert 1 == len(matched) + assert 1 == len(matched), f"Expected event {posted_event['event_id']} not found in Kafka within 10s" assert posted_event["tenant_id"] == matched[0]["tenant_id"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_topics_endpoint.py` around lines 171 - 199, In test_event_received_by_kafka update the failing assertion to include a descriptive message when the 10s poll deadline expires: when checking matched (variable matched) replace the bare assert 1 == len(matched) with an assertion that includes context (e.g., the expected event_id from posted_event, the actual len(matched), and any collected messages or elapsed timeout) so test_event_received_by_kafka provides a clear timeout/error message; keep the poll loop and consumer.close() logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@DEVELOPER.md`:
- Around line 133-139: Update the prose in DEVELOPER.md to accurately describe
that the pytest-cov command runs the unit tests in the tests/unit/ directory
(not “all tests in the tests directory”); specifically edit the sentence
describing the command shown (the pytest --cov=. -v tests/unit/
--cov-fail-under=80 --cov-report=html example) so it says it executes unit tests
under tests/unit/ and generates the coverage report with missing line details
and an 80% threshold.
In `@tests/integration/conftest.py`:
- Around line 246-253: The health-check loop using req_lib.get currently breaks
on success but silently yields the URL if all retries fail; modify the loop to
use a for/else so that if no attempt succeeds the else block raises an explicit
error (e.g., RuntimeError or AssertionError) with the URL and last exception
info, ensuring the fixture fails fast instead of yielding a dead server URL;
update the block that catches ConnectionError/OSError (the req_lib.get
health-check loop) to capture the last exception and include it in the raised
error message.
- Around line 334-345: The fixture's teardown only pops POSTGRES_SECRET_NAME and
POSTGRES_SECRET_REGION, leaving other env vars set earlier (LOG_LEVEL,
AWS_ENDPOINT_URL, AWS_DEFAULT_REGION, AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY,
CONF_DIR) to leak; update the cleanup block in tests/integration/conftest.py to
pop all those keys from os.environ (LOG_LEVEL, AWS_ENDPOINT_URL,
AWS_DEFAULT_REGION, AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY, CONF_DIR) in
addition to the existing POSTGRES_* keys, and ensure any created CONF_DIR/test
config files are removed as already handled (e.g., reference the same
test_config_file, access_config_dst, topic_schemas_dst and test_config_dir
symbols) so no env/config residue remains after the session.
In `@tests/integration/test_topics_endpoint.py`:
- Around line 201-212: The test test_event_received_by_eventbridge declares the
posted_event fixture but never uses it, so either remove the unused parameter
and adjust the test name to reflect a connectivity check (e.g., rename to
test_event_bus_reachable) or actually verify event delivery by wiring
posted_event into EventBridge: use the events client to create/connect an SQS
queue target (create_queue), add it as a target with put_targets on the default
bus, call put_events with the posted_event payload, then poll the SQS queue with
receive_message and assert the posted_event was received; update or remove the
posted_event parameter accordingly and clean up created resources.
---
Nitpick comments:
In `@tests/integration/conftest.py`:
- Around line 329-330: Add an inline comment next to the deferred import of
lambda_handler explaining that the import is intentionally deferred until after
environment variables are set for integration tests to avoid violating the
"imports at top" guideline; locate the import line "from src.event_gate_lambda
import lambda_handler" in tests/integration/conftest.py and insert a brief
comment referencing that the import is deferred on purpose (e.g., to allow
setting environment variables/mocking before module initialization) so future
reviewers won't move it to module scope.
- Around line 50-54: MockJWTHandler currently stores mutable keys as class
attributes (private_key_pem, public_key_b64) which are shared across instances;
instead, stop using class-level mutable state by passing the keys into the
handler via the server instance or a closure created in _start_mock_jwt_server:
attach the keys to the HTTPServer instance (e.g.,
server.private_key_pem/server.public_key_b64) and have MockJWTHandler read them
from self.server, or create a handler factory/closure that binds the keys (using
functools.partial or a nested class) so each server instance gets its own
immutable key values when started.
In `@tests/integration/test_topics_endpoint.py`:
- Around line 171-199: In test_event_received_by_kafka update the failing
assertion to include a descriptive message when the 10s poll deadline expires:
when checking matched (variable matched) replace the bare assert 1 ==
len(matched) with an assertion that includes context (e.g., the expected
event_id from posted_event, the actual len(matched), and any collected messages
or elapsed timeout) so test_event_received_by_kafka provides a clear
timeout/error message; keep the poll loop and consumer.close() logic unchanged.
Collaborator
oto-macenauer-absa
left a comment
oto-macenauer-absa
left a comment
There was a problem hiding this comment.
gj, this is very nice implementation
just please change the topic to runs if possible
| ) -> None: | ||
| """Test POST without JWT returns 401.""" | ||
| response = eventgate_client.post_event( | ||
| "public.cps.za.test", |
Collaborator
There was a problem hiding this comment.
Rather use public.cps.za.runs for the integration tests. It will have greater value for checking of the required functionality.
Collaborator
Author
There was a problem hiding this comment.
Please see the solution implemented in commit: b01e140.
requirements.txt
Outdated
| requests==2.32.5 | ||
| boto3==1.40.25 | ||
| confluent-kafka==2.12.1 | ||
| testcontainers==4.9.0 |
Collaborator
There was a problem hiding this comment.
The current version is 4.14. Was 4.9. selected on purpose? eg. compatibility issues?
Collaborator
Author
There was a problem hiding this comment.
That is a great catch. See the change in commit: b01e140.
There was a problem hiding this comment.
🧹 Nitpick comments (5)
tests/integration/conftest.py (4)
331-332: Deferred import is justified but violates the coding guideline.The coding guideline states "Place all imports at the top of file, never inside functions." Here, the deferred import is intentional because
src.event_gate_lambdaperforms module-level initialization that depends on the environment variables set above. Consider adding a brief inline comment explaining this to prevent future reviewers from "fixing" it.# Import the lambda handler (this triggers initialization). + # NOTE: Intentionally deferred — module-level init reads env vars set above. from src.event_gate_lambda import lambda_handlerAs per coding guidelines: "Place all imports at the top of file, never inside functions."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/conftest.py` around lines 331 - 332, Add a brief inline comment above the deferred import "from src.event_gate_lambda import lambda_handler" in conftest.py explaining that the import is intentionally deferred because src.event_gate_lambda performs module-level initialization that depends on environment variables set earlier in this function; this documents the exception to the "imports at top" guideline so future reviewers don't move it.
98-103: Pin the LocalStack image tag for reproducible CI builds.All other images are pinned to specific versions, but
localstack/localstack:latestis a floating tag. This can cause non-deterministic test failures when LocalStack publishes breaking changes. Pin to the same version used in thelocalstack_containerfixture or a known-good tag.CONTAINER_IMAGES: List[str] = [ "testcontainers/ryuk:0.8.1", "postgres:16", "confluentinc/cp-kafka:7.6.0", - "localstack/localstack:latest", + "localstack/localstack:4.1", ]Also update line 215 to match:
- container = LocalStackContainer("localstack/localstack:latest") + container = LocalStackContainer("localstack/localstack:4.1")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/conftest.py` around lines 98 - 103, CONTAINER_IMAGES contains a floating LocalStack tag; replace "localstack/localstack:latest" with the same pinned version used by the localstack_container fixture to make CI reproducible. Update the CONTAINER_IMAGES list (symbol CONTAINER_IMAGES) to use that specific tag and also adjust the localstack_container fixture (symbol localstack_container) to the same pinned tag so both definitions match.
172-192: Container leak if schema initialization fails beforeyield.If
psycopg2.connect()orcursor.execute(SCHEMA_SQL)raises, the generator never reachesyield, so the teardown code (container.stop()) never executes. Ryuk will eventually clean up, but atry/finallyis more robust.Proposed fix
`@pytest.fixture`(scope="session") def postgres_container() -> Generator[str, None, None]: """PostgreSQL container with initialized schema.""" logger.debug("Starting PostgreSQL container.") container = PostgresContainer("postgres:16", dbname="eventgate") - container.start() - dsn = _convert_dsn(container.get_connection_url()) - logger.debug("PostgreSQL started, initializing schema.") - - conn = psycopg2.connect(dsn) - conn.autocommit = True - with conn.cursor() as cursor: - cursor.execute(SCHEMA_SQL) - conn.close() - logger.debug("PostgreSQL schema initialized.") - - yield dsn - - container.stop() - logger.debug("PostgreSQL container stopped.") + try: + dsn = _convert_dsn(container.get_connection_url()) + logger.debug("PostgreSQL started, initializing schema.") + + conn = psycopg2.connect(dsn) + conn.autocommit = True + with conn.cursor() as cursor: + cursor.execute(SCHEMA_SQL) + conn.close() + logger.debug("PostgreSQL schema initialized.") + + yield dsn + finally: + container.stop() + logger.debug("PostgreSQL container stopped.")The same pattern applies to
kafka_containerandlocalstack_containerfixtures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/conftest.py` around lines 172 - 192, The fixture may leak containers if schema initialization (psycopg2.connect or cursor.execute(SCHEMA_SQL)) raises before yield; change the pattern in postgres_container (and similarly in kafka_container and localstack_container) to start the container first, then use try/finally: perform connection and schema initialization inside the try, ensure any DB connections/cursors are closed in that block, yield the dsn inside the try, and call container.stop() in the finally so container.stop() always runs even on initialization failure; reference postgres_container, container.start(), container.stop(), psycopg2.connect(), cursor.execute(SCHEMA_SQL), and yield when applying the change.
156-158: Unused variableimage_name(Ruff RUF059).Prefix with underscore to indicate it's intentionally unused.
- image_name, success, message = task.result() + _image_name, success, message = task.result()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/conftest.py` around lines 156 - 158, The loop unpacks task.result() into image_name, success, message but image_name is unused; rename it to _image_name (or prefix with an underscore) in the for-loop unpacking to satisfy Ruff RUF059 and indicate the value is intentionally unused—update the unpacking where as_completed(pull_tasks) is iterated and where task.result() is called.tests/integration/test_topics_endpoint.py (1)
213-227: Consider adding a brief retry or wait for the Postgres assertion.The lambda handler is invoked directly so writes should be synchronous, but if the write path ever becomes async (e.g., via a background flush or connection pool), this single-shot query would become flaky. A small polling loop (similar to the Kafka test) would make this more resilient to future changes.
That said, if the write is guaranteed synchronous today, this is fine as-is.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_topics_endpoint.py` around lines 213 - 227, The test_event_received_by_postgres single-shot SELECT in test_event_received_by_postgres can be flaky if writes become async; change it to poll for the row with a short retry loop (e.g., up to a few seconds) instead of a single fetch. Inside test_event_received_by_postgres, use posted_event["event_id"] and postgres_container to repeatedly open/use the psycopg2 connection and execute the SELECT in a loop with a small sleep (e.g., time.sleep(0.1)) and a timeout guard (e.g., time.time() + max_wait) and only assert once the row is returned or fail after timeout; add the required import for time at top of the test file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/integration/test_topics_endpoint.py`:
- Around line 201-211: The test was correctly renamed to
test_eventbridge_bus_reachable and the unused posted_event parameter removed,
but ensure there are no remaining references to the removed fixture: search for
and remove any usages or imports of posted_event (fixture, helper, or parameter)
and update any test names or markers that still reference the old name;
specifically check for leftover references in test_eventbridge_bus_reachable,
any conftest fixtures, and any calls to a posted_event helper so the suite no
longer expects that fixture.
---
Nitpick comments:
In `@tests/integration/conftest.py`:
- Around line 331-332: Add a brief inline comment above the deferred import
"from src.event_gate_lambda import lambda_handler" in conftest.py explaining
that the import is intentionally deferred because src.event_gate_lambda performs
module-level initialization that depends on environment variables set earlier in
this function; this documents the exception to the "imports at top" guideline so
future reviewers don't move it.
- Around line 98-103: CONTAINER_IMAGES contains a floating LocalStack tag;
replace "localstack/localstack:latest" with the same pinned version used by the
localstack_container fixture to make CI reproducible. Update the
CONTAINER_IMAGES list (symbol CONTAINER_IMAGES) to use that specific tag and
also adjust the localstack_container fixture (symbol localstack_container) to
the same pinned tag so both definitions match.
- Around line 172-192: The fixture may leak containers if schema initialization
(psycopg2.connect or cursor.execute(SCHEMA_SQL)) raises before yield; change the
pattern in postgres_container (and similarly in kafka_container and
localstack_container) to start the container first, then use try/finally:
perform connection and schema initialization inside the try, ensure any DB
connections/cursors are closed in that block, yield the dsn inside the try, and
call container.stop() in the finally so container.stop() always runs even on
initialization failure; reference postgres_container, container.start(),
container.stop(), psycopg2.connect(), cursor.execute(SCHEMA_SQL), and yield when
applying the change.
- Around line 156-158: The loop unpacks task.result() into image_name, success,
message but image_name is unused; rename it to _image_name (or prefix with an
underscore) in the for-loop unpacking to satisfy Ruff RUF059 and indicate the
value is intentionally unused—update the unpacking where
as_completed(pull_tasks) is iterated and where task.result() is called.
In `@tests/integration/test_topics_endpoint.py`:
- Around line 213-227: The test_event_received_by_postgres single-shot SELECT in
test_event_received_by_postgres can be flaky if writes become async; change it
to poll for the row with a short retry loop (e.g., up to a few seconds) instead
of a single fetch. Inside test_event_received_by_postgres, use
posted_event["event_id"] and postgres_container to repeatedly open/use the
psycopg2 connection and execute the SELECT in a loop with a small sleep (e.g.,
time.sleep(0.1)) and a timeout guard (e.g., time.time() + max_wait) and only
assert once the row is returned or fail after timeout; add the required import
for time at top of the test file.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/integration/test_topics_endpoint.py (1)
183-200: Kafka consumer silently swallows errors — consider logging or failing on fatal errors.When
msg.error()is truthy (line 190), the loop justcontinues. This hides broker-level errors (e.g., unknown topic, auth failures) that could cause the test to time out with a misleading "0 matched messages" assertion rather than surfacing the real problem.💡 Suggested improvement
+from confluent_kafka import KafkaError ... if msg.error(): + if msg.error().code() != KafkaError._PARTITION_EOF: + pytest.fail(f"Kafka consumer error: {msg.error()}") continue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_topics_endpoint.py` around lines 183 - 200, The test loop currently ignores broker errors by continuing when msg.error() is truthy; change this so any non-None msg.error() is surfaced: log or raise an AssertionError with the error details (e.g., include str(msg.error()) and any code) instead of silently continuing inside the while that calls consumer.poll(), so fatal broker/auth/topic errors fail the test immediately; keep the existing consumer.close() in the finally block.tests/integration/schemas/postgres_schema.py (1)
19-43: LGTM — consider adding primary keys for more realistic schema fidelity.The schema works for integration tests, but both tables lack primary keys and foreign key constraints. If the production schema has these, their absence could mask constraint-violation bugs in the writer code. Not blocking, but worth considering for higher-fidelity testing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/schemas/postgres_schema.py` around lines 19 - 43, The test schema SCHEMA_SQL defines public_cps_za_runs and public_cps_za_runs_jobs without primary/foreign keys; add realistic constraints to catch violations: add a primary key on public_cps_za_runs (e.g., event_id) and a primary key on public_cps_za_runs_jobs (e.g., composite (event_id, catalog_id) or a separate job_id), and add a foreign key on public_cps_za_runs_jobs.event_id referencing public_cps_za_runs.event_id (update NOT NULL as needed). Ensure these changes align with WriterPostgres._postgres_run_write expectations so tests reflect production constraints.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@conf/access.json`:
- Line 2: The access config entry "public.cps.za.runs" currently contains the
test principal "IntegrationTestUser" which must not be shipped to prod; remove
"IntegrationTestUser" from the array in conf/access.json and instead add it to a
test-only access overlay (eg. conf/access.test.json) or implement a deploy-time
filter that strips known test principals (like "IntegrationTestUser") from the
"public.cps.za.runs" principal list; update any deployment/build script (the
pipeline job that reads conf/access.json) to prefer the test overlay in test
environments or to remove test principals before packaging so prod never
contains "IntegrationTestUser".
---
Nitpick comments:
In `@tests/integration/schemas/postgres_schema.py`:
- Around line 19-43: The test schema SCHEMA_SQL defines public_cps_za_runs and
public_cps_za_runs_jobs without primary/foreign keys; add realistic constraints
to catch violations: add a primary key on public_cps_za_runs (e.g., event_id)
and a primary key on public_cps_za_runs_jobs (e.g., composite (event_id,
catalog_id) or a separate job_id), and add a foreign key on
public_cps_za_runs_jobs.event_id referencing public_cps_za_runs.event_id (update
NOT NULL as needed). Ensure these changes align with
WriterPostgres._postgres_run_write expectations so tests reflect production
constraints.
In `@tests/integration/test_topics_endpoint.py`:
- Around line 183-200: The test loop currently ignores broker errors by
continuing when msg.error() is truthy; change this so any non-None msg.error()
is surfaced: log or raise an AssertionError with the error details (e.g.,
include str(msg.error()) and any code) instead of silently continuing inside the
while that calls consumer.poll(), so fatal broker/auth/topic errors fail the
test immediately; keep the existing consumer.close() in the finally block.
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@DEVELOPER.md`:
- Around line 136-139: Update the descriptive text that follows the pytest
command to match the scoped command `pytest --cov=. -v tests/unit/
--cov-fail-under=80 --cov-report=html` by replacing the phrase "all tests in the
tests directory" with wording that specifies the unit test scope (e.g., "all
tests in the tests/unit directory" or "all unit tests in tests/unit/"); ensure
the description references the generated coverage report and the 80% threshold
unchanged so the explanation accurately reflects the `tests/unit/` scope.
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
2 participants
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.
Overview
This pull request is adding the integration testing feature to the project. It divides the unit tests and integration tests to separate folders and creates a better developer experience. This solution is creating a python containers and run the integration tests against them.
Release Notes
Related
Closes #63
Summary by CodeRabbit
New Features
Tests
CI / Chores
Documentation