Add Firefly III integration test suite #57
Conversation
Starts up a firefly instance and creates sample accounts,budgets and transactions for tests.
Adds tests for accounts, budgets and transactions covering most of the basic scenarios.
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds an integration testing stack for LamPyrid: CI job, Docker Compose test environment, pytest config and fixtures, test data factories, comprehensive async integration tests for accounts/budgets/transactions, helper scripts to provision/verify Firefly III, and two new FireflyClient methods to create accounts and budgets. Changes
Sequence Diagram(s)sequenceDiagram
participant Setup as Test Setup (conftest)
participant FireflyAPI as Firefly III API
participant Cache as Test Data Cache
participant Cleanup as Cleanup Tracker
Setup->>FireflyAPI: list_accounts()
FireflyAPI-->>Setup: account list
alt Accounts exist
Setup->>Cache: cache existing accounts
else
Setup->>FireflyAPI: create_account(account_store)
FireflyAPI-->>Setup: created Account
Setup->>Cache: cache created account
Setup->>Cleanup: record created account ID
end
Setup->>FireflyAPI: list_budgets()
FireflyAPI-->>Setup: budget list
alt Test budget exists
Setup->>Cache: cache budget
else
Setup->>FireflyAPI: create_budget(budget_store)
FireflyAPI-->>Setup: created Budget
Setup->>Cache: cache created budget
Setup->>Cleanup: record created budget ID
end
sequenceDiagram
participant Test as Integration Test
participant MCPClient as FastMCP Client
participant FireflyAPI as Firefly III API
participant TransactionCleanup as transaction_cleanup Fixture
Test->>MCPClient: obtain client (mcp_client fixture)
MCPClient->>FireflyAPI: proxied API calls (accounts/transactions/budgets)
MCPClient-->>Test: responses
Test->>MCPClient: create_withdrawal(...)
MCPClient->>FireflyAPI: POST /transactions
FireflyAPI-->>MCPClient: created Transaction
MCPClient-->>Test: Transaction
Test->>TransactionCleanup: append transaction_id
Note over TransactionCleanup: After test completes
TransactionCleanup->>FireflyAPI: DELETE /transactions/{id} for each tracked ID
FireflyAPI-->>TransactionCleanup: deletion confirmed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 5
🤖 Fix all issues with AI agents
In `@pyproject.toml`:
- Around line 19-23: Update the test dependencies in pyproject.toml to versions
that declare Python 3.14 support: change the "pytest>=9.0.0" entry to
"pytest>=9.0.1" (or later) and change the "pytest-asyncio>=1.3.0" entry to
"pytest-asyncio>=0.24.0" (or later) so the project (Python >=3.14) uses
compatible pytest and pytest-asyncio releases.
In `@tests/conftest.py`:
- Around line 175-184: The current fixture directly calls the fixture function
test_asset_account (await test_asset_account(firefly_client)), which bypasses
pytest's fixture injection; instead add test_asset_account as a fixture
parameter to this fixture (so pytest injects it), remove the direct call/await,
and rely on the injected test_asset_account to ensure accounts are created
before you access _cached_test_accounts; keep the existing check on
_cached_test_accounts and return _cached_test_accounts[1] as before.
In `@tests/integration/test_transactions.py`:
- Around line 495-506: The test test_delete_nonexistent_transaction currently
uses a hard-coded transaction_id '999999' which can collide with real data;
modify it to create a real transaction via the existing create transaction
helper (e.g., use the test fixtures test_asset_account/test_expense_account and
the create_transaction or make_create_transaction_request helper), delete that
transaction once with firefly_client.delete_transaction, then attempt to delete
it a second time and assert the second call raises HTTPStatusError with
response.status_code == 404; update references to
make_delete_transaction_request and firefly_client.delete_transaction to use the
created transaction's id.
In `@tests/setup_test_env.sh`:
- Around line 8-85: The health-check loop is wrong because it uses command
substitution with until $(curl ...), and CI lacks the docker-compose binary;
replace the loop condition with a direct curl invocation (e.g., until curl
--output /dev/null --silent --head --fail "$HEALTH_URL"; do ...) or use while !
curl ...; do to correctly test HEALTH_URL/HEALTH_URL, and update all invocations
of the docker-compose command to the v2 form "docker compose -f
docker-compose.test.yml ..." (or add a small compatibility check that prefers
"docker compose" if available) to fix MAX_RETRIES/RETRY_COUNT based startup and
ensure Compose runs in CI.
In `@tests/verify_setup.py`:
- Around line 2-10: The docstring claims the script verifies required test
accounts and a budget but the code only prints an informational note (around the
print at lines referenced) — implement real checks: add functions
verify_test_accounts(base_url, api_token, REQUIRED_ACCOUNTS) and
verify_test_budget(base_url, api_token, REQUIRED_BUDGET) that call the Firefly
III API (use existing base_url/api/v1/accounts and /api/v1/budgets endpoints and
the same auth headers used elsewhere in this file), validate that each required
name is present, and raise/print an error and exit non-zero if any are missing;
finally replace the informational note with calls to verify_test_accounts(...)
and verify_test_budget(...) from main so the script actually enforces the
docstring.
🧹 Nitpick comments (10)
src/lampyrid/clients/firefly.py (1)
123-128: Log payload on account creation failures (for parity with other POSTs).This improves diagnosability when Firefly rejects a payload.
♻️ Suggested change
async def create_account(self, account_store: AccountStore) -> Account: - r = await self._client.post('/api/v1/accounts', json=self._serialize_model(account_store)) - self._handle_api_error(r) + payload = self._serialize_model(account_store) + r = await self._client.post('/api/v1/accounts', json=payload) + self._handle_api_error(r, payload) r.raise_for_status() account_single = AccountSingle.model_validate(r.json()) return Account.from_account_read(account_single.data)tests/fixtures/transactions.py (1)
115-147: Align search factory types to the request model and drop the suppression.
SearchTransactionsRequestexpectsdatefields, and the current# ty:ignore[...]suppression won’t match ty’s documented# ty: ignore[...]syntax (note the space).(docs.astral.sh) Consider tightening the signature todateandLiteral[...]so you can remove the ignore entirely.♻️ Suggested change
-from typing import List +from typing import List, Literal @@ def make_search_transactions_request( query: str | None = None, description_contains: str | None = None, amount_equals: float | None = None, amount_more: float | None = None, amount_less: float | None = None, - date_on: datetime | None = None, - date_after: datetime | None = None, - date_before: datetime | None = None, - transaction_type: str | None = None, + date_on: date | None = None, + date_after: date | None = None, + date_before: date | None = None, + transaction_type: Literal['withdrawal', 'deposit', 'transfer'] | None = None, @@ - type=transaction_type, # ty:ignore[invalid-argument-type] + type=transaction_type,tests/conftest.py (2)
34-39: Global mutable state may cause test isolation issues.The global caches (
_cached_test_accounts,_cached_test_budgets) and tracking lists (_created_account_ids,_created_budget_ids) persist across test functions. This works for sequential execution but:
- If using
pytest-xdistfor parallel execution, race conditions may occur.- The
_created_account_idsand_created_budget_idsare tracked but never cleaned up at session end—consider adding a session-scoped cleanup fixture.If parallel execution is not planned, this is acceptable, but documenting the limitation would help.
71-84: Overriding private_clientattribute is fragile but pragmatic.Directly setting
client._clientworks for test isolation but will break silently ifFireflyClientimplementation changes. Consider adding a comment noting this coupling, or exposing a configuration option onFireflyClientfor customhttpx.AsyncClientinjection..github/workflows/ci.yml (2)
211-214: Consider usingdocker compose(v2 syntax).The
docker-composecommand is the legacy v1 syntax. On recent GitHub runners,docker compose(the Docker CLI plugin) is available and preferred.Proposed fix
- name: Stop Firefly III if: always() run: | - docker-compose -f docker-compose.test.yml down -v + docker compose -f docker-compose.test.yml down -v
203-210: Consider adding CODECOV_TOKEN for reliability.The
codecov-action@v4works without a token for public repos but is more reliable (and required for private repos) with a token configured via repository secrets.Proposed addition
- name: Upload coverage report uses: codecov/codecov-action@v4 if: always() with: files: ./coverage.xml flags: integration name: integration-tests + token: ${{ secrets.CODECOV_TOKEN }}tests/verify_setup.py (1)
50-81: Redundant API call to/api/v1/about.Both checks 1 and 2 call the same endpoint. The first successful 200 response already confirms both accessibility and token validity. Consider consolidating into a single request.
Proposed consolidation
# Check 1: Firefly III is running -print('1. Checking Firefly III accessibility...') +print('1. Checking Firefly III accessibility and token validity...') try: response = httpx.get(f'{FIREFLY_URL}/api/v1/about', headers=headers, timeout=10.0) if response.status_code == 200: data = response.json() version = data.get('data', {}).get('version', 'unknown') print(f' ✓ Firefly III is running (version: {version})') + print(' ✓ Token is valid') + elif response.status_code == 401: + print(' ✓ Firefly III is running') + print(' ✗ Token is invalid or expired') + print(' Please regenerate token in Firefly III web UI') + all_checks_passed = False else: print(f' ✗ Unexpected response: {response.status_code}') all_checks_passed = False except Exception as e: print(f' ✗ Cannot connect to Firefly III: {e}') print(' Make sure Firefly III is running: docker-compose -f docker-compose.test.yml up -d') all_checks_passed = False -# Check 2: Token is valid -print('\n2. Checking API token validity...') -try: - response = httpx.get(f'{FIREFLY_URL}/api/v1/about', headers=headers, timeout=10.0) - if response.status_code == 200: - print(' ✓ Token is valid') - elif response.status_code == 401: - print(' ✗ Token is invalid or expired') - print(' Please regenerate token in Firefly III web UI') - all_checks_passed = False - else: - print(f' ✗ Unexpected response: {response.status_code}') - all_checks_passed = False -except Exception as e: - print(f' ✗ Cannot verify token: {e}') - all_checks_passed = False - -# Note about test data creation -print('\n3. Test data creation...') +# Note about test data creation +print('\n2. Test data creation...')docker-compose.test.yml (1)
7-7: Consider pinning Firefly III image version for better test reproducibility.
fireflyiii/core:latestuses the current stable release, but it will change when new stable versions are released. For consistent, reproducible test environments, pin to a specific version tag (e.g.,fireflyiii/core:version-6.1.0) and update it deliberately when upgrading.tests/integration/test_transactions.py (1)
49-52: Stabilize money assertions against float/Decimal parsing.If Firefly responses or Pydantic models return
Decimal/string amounts, strict float equality can be flaky. Consider normalizing or usingpytest.approxfor comparisons across these tests. Please verify theTransaction.amounttype and adjust accordingly.💡 Example adjustment (apply similarly in the other assertions)
- assert transaction.amount == 10.50 + assert transaction.amount == pytest.approx(10.50)Also applies to: 79-82, 106-109, 134-137, 226-229, 385-387
tests/integration/test_budgets.py (1)
120-143: Make xfail strict so a fixed API bug doesn’t go unnoticed.If issue
#43is resolved, an XPASS should prompt a follow‑up rather than silently passing.✅ Suggested tweak
-@pytest.mark.xfail( - reason='Firefly III API bug - currency_id returned as int instead of string (issue `#43`)' -) +@pytest.mark.xfail( + reason='Firefly III API bug - currency_id returned as int instead of string (issue `#43`)', + strict=True, +)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
.github/workflows/ci.yml.gitignoredocker-compose.test.ymlpyproject.tomlpytest.inisrc/lampyrid/clients/firefly.pytests/__init__.pytests/conftest.pytests/fixtures/__init__.pytests/fixtures/accounts.pytests/fixtures/budgets.pytests/fixtures/transactions.pytests/helpers/__init__.pytests/integration/__init__.pytests/integration/test_accounts.pytests/integration/test_budgets.pytests/integration/test_transactions.pytests/setup_test_env.shtests/verify_setup.py
🧰 Additional context used
🧬 Code graph analysis (7)
tests/fixtures/accounts.py (2)
src/lampyrid/models/firefly_models.py (1)
AccountTypeFilter(2168-2189)src/lampyrid/models/lampyrid_models.py (3)
GetAccountRequest(176-179)ListAccountRequest(157-161)SearchAccountRequest(164-173)
tests/integration/test_budgets.py (2)
src/lampyrid/models/lampyrid_models.py (2)
Budget(54-80)from_budget_read(72-80)tests/fixtures/budgets.py (5)
make_get_available_budget_request(54-66)make_get_budget_request(19-21)make_get_budget_spending_request(24-36)make_get_budget_summary_request(39-51)make_list_budgets_request(14-16)
src/lampyrid/clients/firefly.py (2)
src/lampyrid/models/firefly_models.py (6)
AccountStore(2835-2897)AvailableBudgetArray(2440-2442)BudgetArray(4308-4310)BudgetLimitArray(2445-2447)BudgetStore(3333-3355)AccountSingle(2831-2832)src/lampyrid/models/lampyrid_models.py (4)
Account(25-51)from_account_read(39-51)Budget(54-80)from_budget_read(72-80)
tests/fixtures/budgets.py (1)
src/lampyrid/models/lampyrid_models.py (5)
GetAvailableBudgetRequest(521-531)GetBudgetRequest(446-449)GetBudgetSpendingRequest(472-483)GetBudgetSummaryRequest(501-509)ListBudgetsRequest(437-443)
tests/integration/test_transactions.py (4)
src/lampyrid/clients/firefly.py (1)
FireflyClient(54-544)src/lampyrid/models/firefly_models.py (2)
TransactionTypeFilter(2192-2206)TransactionTypeProperty(2328-2333)src/lampyrid/models/lampyrid_models.py (2)
TransactionListResponse(406-434)from_transaction_array(420-434)tests/fixtures/transactions.py (9)
make_create_bulk_transactions_request(84-88)make_create_deposit_request(44-61)make_create_transfer_request(64-81)make_create_withdrawal_request(22-41)make_delete_transaction_request(180-182)make_get_transaction_request(91-93)make_get_transactions_request(96-112)make_search_transactions_request(115-147)make_update_transaction_request(150-170)
tests/fixtures/transactions.py (2)
src/lampyrid/models/firefly_models.py (1)
TransactionTypeFilter(2192-2206)src/lampyrid/models/lampyrid_models.py (10)
BulkUpdateTransactionsRequest(584-592)CreateBulkTransactionsRequest(534-556)CreateDepositRequest(210-228)CreateTransferRequest(231-250)CreateWithdrawalRequest(182-207)DeleteTransactionRequest(398-399)GetTransactionRequest(402-403)GetTransactionsRequest(253-279)SearchTransactionsRequest(282-395)UpdateTransactionRequest(559-581)
tests/conftest.py (2)
src/lampyrid/models/firefly_models.py (3)
AccountTypeFilter(2168-2189)AccountStore(2835-2897)ShortAccountTypeProperty(2316-2325)src/lampyrid/models/lampyrid_models.py (6)
Account(25-51)Budget(54-80)from_account_read(39-51)ListBudgetsRequest(437-443)from_budget_read(72-80)DeleteTransactionRequest(398-399)
🪛 GitHub Actions: CI
tests/setup_test_env.sh
[error] 10-10: docker-compose: command not found. The setup script failed when attempting to run docker-compose.
🪛 Shellcheck (0.11.0)
tests/setup_test_env.sh
[warning] 18-18: Remove surrounding $() to avoid executing output (or use eval if intentional).
(SC2091)
🔇 Additional comments (42)
tests/integration/__init__.py (1)
1-1: LGTM — clear package marker.tests/fixtures/__init__.py (1)
1-1: LGTM — clear package marker.tests/helpers/__init__.py (1)
1-1: LGTM — clear package marker.tests/__init__.py (1)
1-1: LGTM — clear package marker.pytest.ini (1)
1-12: LGTM — concise pytest discovery and marker config.tests/fixtures/accounts.py (1)
1-27: Nice, clean request factories.The helpers match the request models and keep tests readable.
src/lampyrid/clients/firefly.py (1)
502-509: Budget creation path looks good.Payload serialization, error handling, and model validation are consistent with the rest of the client.
tests/fixtures/budgets.py (1)
1-66: Good, consistent budget request factories.Defaults and date range logic are clear and reusable for integration tests.
tests/conftest.py (7)
23-32: LGTM on environment loading with graceful fallback.The environment loading logic correctly checks for
.env.testexistence and provides a helpful warning when missing, allowing CI to set environment variables directly.
41-52: LGTM on event loop management.The function-scoped event loop with safe teardown handling is appropriate for the async fixtures in this file.
187-217: LGTM on expense account fixture.The fallback logic for finding or defaulting to an expense account name is appropriate, and returning just the name string aligns with Firefly III's withdrawal API requirements.
219-249: LGTM, with optional DRY opportunity.This fixture mirrors
test_expense_account. A shared helper like_find_or_default_account(client, account_type, default_name)could reduce duplication, but the current approach is clear and acceptable.
251-292: LGTM on test budget fixture.The budget creation/reuse pattern is consistent with the account fixtures and appropriately caches the result.
294-320: LGTM on transaction cleanup fixture.The cleanup pattern with individual error handling for each transaction ensures robust teardown even if some deletions fail.
132-139: Theaccount_rolefield accepts the string literal'defaultAsset'correctly.AccountRolePropertyis a Pydantic v2RootModelthat automatically coerces matching string values toAccountRolePropertyEnummembers during validation. Passing'defaultAsset'is the idiomatic Pydantic v2 pattern and requires no changes.Likely an incorrect or invalid review comment.
.gitignore (1)
139-139: LGTM on ignoring.env.test.Correctly prevents test credentials from being committed to the repository.
.github/workflows/ci.yml (1)
174-201: LGTM on integration test job structure.The job correctly depends on linting and build tests, and the setup flow (chmod + script execution + pytest with coverage) is well-organized.
docker-compose.test.yml (1)
46-55: LGTM on healthcheck and network configuration.The healthcheck with a 60-second start period accommodates Firefly III's initialization time, and the bridge network is appropriate for isolated testing.
tests/verify_setup.py (1)
88-97: LGTM on verification summary and exit codes.The exit codes (0 for success, 1 for failure) enable proper CI integration, and the summary provides clear next steps.
tests/integration/test_transactions.py (8)
140-195: Bulk create coverage looks solid.Good end-to-end validation of multi-transaction creation and cleanup handling.
232-248: LGTM for list-all transactions coverage.The conversion and pagination metadata assertions are clear and useful.
250-294: Filter test is well scoped.Covers date range + type filtering and validates inclusion of the created transaction.
296-314: Pagination assertions are clear.Checks result size and pagination fields appropriately.
316-354: Search-by-description test looks good.The unique description makes this deterministic and resilient.
390-420: Description update path is well covered.Simple, direct validation of update behavior.
423-455: Budget update scenario is clear and correct.Good coverage of post-update budget assignment.
460-492: Delete flow coverage is solid.Verifies delete success and the expected 404 on subsequent retrieval.
tests/integration/test_accounts.py (10)
16-35: LGTM — list all accounts.Checks required fields consistently.
37-54: LGTM — list asset accounts.Type filtering assertions are straightforward and correct.
56-71: LGTM — list expense accounts.Scoped assertions make sense when results are present.
73-87: LGTM — list revenue accounts.Type filtering validation is clear.
90-101: LGTM — get account by ID.Direct field equality checks are appropriate.
104-115: LGTM — invalid account handling.404 expectation is clearly asserted.
118-135: LGTM — exact-name search.Simple and reliable match validation.
137-155: LGTM — partial-name search.Good coverage for fuzzy search behavior.
158-180: LGTM — search with type filter.Confirms both filter correctness and expected inclusion.
183-196: LGTM — no-results search case.Clean assertion for empty result set.
tests/integration/test_budgets.py (5)
18-36: LGTM — list all budgets.Validates required fields cleanly.
38-55: LGTM — active-only budgets.Clear assertions for active status.
57-68: LGTM — get budget by ID.Direct field equality checks are correct.
70-90: LGTM — budget spending analysis.Good coverage of key fields and non-negative spent.
92-118: LGTM — budget summary coverage.Validates totals and individual entries appropriately.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
- Add autouse _setup_test_data fixture to create test accounts/budget at session start - Change fixture scopes from function to session for better performance - Fix AccountRoleProperty type usage in AccountStore initialization - Update CI workflow to use docker compose V2 syntax - Improve test setup script with container engine detection (docker/podman) - Fix user provisioning to use default group #1 and create group membership - Update test_get_transactions_all to create a transaction before testing list
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/conftest.py`:
- Around line 56-83: The event loop fixture needs to be a synchronous,
session-scoped fixture to match the async session-scoped fixture
_setup_test_data; change the event_loop fixture signature (remove async) and its
scope to 'session', create the loop with asyncio.new_event_loop(), yield it, and
close it after yield (catch RuntimeError if already closed) so pytest-asyncio
has a session-scoped loop available for _setup_test_data and other
session-scoped async fixtures.
♻️ Duplicate comments (2)
tests/setup_test_env.sh (1)
39-48: Fix the health-check loop condition (command substitution breaks the loop).
until $(curl ...)executes curl’s output as a command. This makes the health check fail or error instead of retrying. Use the command directly so the exit status drives the loop.🛠️ Proposed fix
-until $(curl --output /dev/null --silent --head --fail "$HEALTH_URL"); do +until curl --output /dev/null --silent --head --fail "$HEALTH_URL"; dotests/integration/test_transactions.py (1)
512-523: Avoid hard-coded IDs for “nonexistent” deletes to prevent flakiness.A fixed ID can exist in seeded or reused databases. Create and delete a real transaction, then assert the second delete returns 404.
✅ Suggested fix
-async def test_delete_nonexistent_transaction(firefly_client: FireflyClient): +async def test_delete_nonexistent_transaction( + firefly_client: FireflyClient, + test_asset_account: Account, + test_expense_account: str, +): """Test handling deletion of non-existent transaction (404).""" - delete_req = make_delete_transaction_request(transaction_id='999999') + create_req = make_create_withdrawal_request( + amount=1.00, + description='Test delete nonexistent', + source_id=test_asset_account.id, + destination_name=test_expense_account, + ) + created = await firefly_client.create_withdrawal(create_req) + delete_req = make_delete_transaction_request(transaction_id=created.id) + await firefly_client.delete_transaction(delete_req) # Should raise HTTPStatusError with 404 with pytest.raises(HTTPStatusError) as exc_info: await firefly_client.delete_transaction(delete_req) assert exc_info.value.response.status_code == 404
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/ci.ymltests/conftest.pytests/integration/test_transactions.pytests/setup_test_env.sh
🧰 Additional context used
🧬 Code graph analysis (2)
tests/integration/test_transactions.py (4)
src/lampyrid/clients/firefly.py (1)
FireflyClient(54-544)src/lampyrid/models/firefly_models.py (2)
TransactionTypeFilter(2192-2206)TransactionTypeProperty(2328-2333)src/lampyrid/models/lampyrid_models.py (2)
TransactionListResponse(406-434)from_transaction_array(420-434)tests/fixtures/transactions.py (7)
make_create_bulk_transactions_request(84-88)make_create_withdrawal_request(22-41)make_delete_transaction_request(180-182)make_get_transaction_request(91-93)make_get_transactions_request(96-112)make_search_transactions_request(115-147)make_update_transaction_request(150-170)
tests/conftest.py (3)
src/lampyrid/models/firefly_models.py (6)
AccountRoleProperty(2229-2234)AccountRolePropertyEnum(2221-2226)AccountStore(2835-2897)AccountTypeFilter(2168-2189)BudgetStore(3333-3355)ShortAccountTypeProperty(2316-2325)src/lampyrid/models/lampyrid_models.py (6)
Account(25-51)Budget(54-80)DeleteTransactionRequest(398-399)ListBudgetsRequest(437-443)from_account_read(39-51)from_budget_read(72-80)src/lampyrid/clients/firefly.py (5)
list_accounts(92-98)create_account(123-128)list_budgets(397-405)create_budget(502-509)delete_transaction(390-395)
🪛 Shellcheck (0.11.0)
tests/setup_test_env.sh
[warning] 39-39: Remove surrounding $() to avoid executing output (or use eval if intentional).
(SC2091)
🔇 Additional comments (32)
tests/setup_test_env.sh (6)
7-26: Container engine/compose detection looks solid.Clear fallback behavior and error messaging.
28-32: Clean startup/reset sequence.The down/up flow is straightforward and readable.
50-52: Migration step is fine.Straightforward and quiet on success.
57-103: Provisioning logic looks good.The transaction + membership setup is clear and robust.
105-129: Token generation flow is solid.Simple and guarded against empty tokens.
131-149: Config persistence and verification flow are clear.Nice handoff to
tests/verify_setup.py..github/workflows/ci.yml (1)
174-214: Integration test job wiring looks good.Setup + pytest + coverage upload sequence is clean.
tests/integration/test_transactions.py (15)
1-22: Imports and fixtures are organized well.No issues spotted here.
27-52: LGTM for basic withdrawal creation.Assertions and cleanup handling are appropriate.
55-82: LGTM for withdrawal with budget.Good coverage of budget linkage.
84-110: LGTM for deposit creation.Checks align with expected response shape.
112-137: LGTM for transfer creation.Clear validation of transfer type.
140-194: LGTM for bulk create.Solid verification of returned items and cleanup.
200-229: LGTM for get-by-id.Create → fetch pattern is correct.
232-265: LGTM for list-all.Reasonable assertions on total counts.
267-311: LGTM for filtered listing.Date-range + type filter is exercised well.
313-331: LGTM for pagination.Limit enforcement is verified.
333-371: LGTM for description search.Unique description avoids collisions.
376-405: LGTM for amount update.Validates updated response fields.
407-438: LGTM for description update.Clear before/after semantics.
440-472: LGTM for budget update.Budget linkage verification is correct.
477-509: LGTM for delete-by-id.Delete + 404 validation is solid.
tests/conftest.py (10)
1-33: Module overview and imports look clean.No issues spotted.
35-48: Env loading block is clear and safe.Good warning on missing config.
84-168: Session-level setup logic looks solid.Cache reuse and guarded setup are good.
170-207: Firefly client fixture is clear and safe.Timeouts and connection limits are sensible.
210-218: Asset account fixture is straightforward.Simple cache access with validation.
221-229: Second asset account fixture looks good.Clear guardrails for missing setup.
232-240: Expense account fixture is fine.Default naming is reasonable for tests.
243-251: Revenue account fixture is fine.Default naming is reasonable for tests.
254-262: Budget fixture looks good.Safe cache access with explicit error if missing.
265-290: Transaction cleanup fixture is solid.Cleanup-on-teardown is exactly what we want.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
- Replace direct FireflyClient calls with FastMCP client for end-to-end testing - Add inline-snapshot for structured data validation across all test suites - Add dirty-equals for flexible float and datetime comparisons - Create mcp_client fixture for testing full MCP protocol stack - Update test data factories to use MCP tool interface - Add expense and revenue account creation to test setup - Remove manual httpx client configuration in favor of FastMCP transport - Improve test isolation and coverage by testing actual tool implementations
- Move pytest settings from pytest.ini to pyproject.toml - Remove pytest.ini file to eliminate duplication - Add missing testpaths, python_files, python_classes, python_functions settings - Update marker descriptions for consistency
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@tests/integration/test_accounts.py`:
- Around line 186-195: The test uses a hard-coded non-existent ID ('999999')
which can collide with long-lived DBs; update test_get_account_invalid to derive
a truly non-existent ID at runtime by first retrieving current account IDs via
the client (use mcp_client.call_tool to list or fetch accounts) and then pick an
ID not in that set (e.g., max(existing_ids)+1 or a UUID-based string if IDs are
strings) before calling mcp_client.call_tool('get_account', {'req': {'id':
derived_id}}) so the 404 assertion remains reliable.
- Around line 12-75: The test_list_accounts_all uses a fixed full-list snapshot
that will fail on real Firefly data; remove the equality assertion against
snapshot(...) and instead assert presence of the expected test accounts by
iterating over the returned accounts (the accounts variable from
mcp_client.call_tool) and for each expected account check that at least one item
has matching 'name', 'type', and 'currency_code', and that 'id' is present and
'current_balance' satisfies IsFloat() (or use IsStr() for string fields where
appropriate); use mcp_client.call_tool('list_accounts', ...) and the existing
IsFloat/IsStr matchers to perform these membership/shape assertions so the test
is order- and ID-agnostic.
In `@tests/integration/test_budgets.py`:
- Around line 12-25: test_list_budgets_all assumes an exact list with id '1'
which will break on real data; update the test to call
mcp_client.call_tool('list_budgets', {'req': {}}), then search/filter the
returned budgets for the test budget (e.g., match on 'name' == 'Test Budget' or
another stable attribute) and assert that such a budget exists, and finally
assert a snapshot only on that filtered budget (not the whole list) using IsStr
for the 'id' field to allow variable IDs; references: test_list_budgets_all,
mcp_client.call_tool('list_budgets', ...), snapshot and IsStr.
In `@tests/integration/test_transactions.py`:
- Around line 17-60: The snapshot in test_create_withdrawal_basic hard-codes IDs
making it brittle; update the expected snapshot in the assert
result.structured_content == snapshot(...) to use dynamic matchers or fixture
IDs: replace 'source_id' and 'destination_id' (and any varying 'budget_id') with
IsStr(min_length=1) or substitute the actual fixture values
test_asset_account.id and test_expense_account (e.g., 'source_id':
test_asset_account.id, 'destination_id': test_expense_account) and keep 'date'
as IsDatetime(iso_string=True) so the test no longer depends on fixed numeric
IDs.
♻️ Duplicate comments (16)
tests/integration/test_transactions.py (6)
63-109: Same hard-coded ID issue as earlier snapshot.
111-155: Same hard-coded ID issue as earlier snapshot.
157-201: Same hard-coded ID issue as earlier snapshot.
203-303: Same hard-coded ID issue as earlier snapshot.
308-362: Same hard-coded ID issue as earlier snapshot.
671-679: Hard-coded ID can be flaky for 404 tests.Consider the create → delete → delete-again pattern to guarantee a 404 without collisions.
tests/integration/test_budgets.py (4)
28-41: Same exact-list snapshot brittleness as above.
44-54: Same fixed-ID snapshot brittleness as above.
57-91: Same fixed-ID snapshot brittleness as above.
94-140: Same fixed-ID snapshot brittleness as above.tests/integration/test_accounts.py (4)
78-110: Same exact-list snapshot brittleness as above.
113-136: Same exact-list snapshot brittleness as above.
139-158: Same exact-list snapshot brittleness as above.
161-183: Same fixed-ID snapshot brittleness as above.pyproject.toml (1)
18-25: Verify pytest-asyncio supports Python 3.14.
requires-pythonis>=3.14, butpytest-asyncio>=1.3.0is quite old and may not support 3.14, which can break async fixtures in CI. Consider bumping to a version that explicitly supports Python 3.14 and pytest 9.x (e.g.,pytest-asyncio>=0.24.0or later).🛠️ Suggested update
- "pytest-asyncio>=1.3.0", + "pytest-asyncio>=0.24.0",pytest-asyncio latest version Python 3.14 supporttests/conftest.py (1)
57-191: Session-scoped async fixture needs a session-scoped event loop.
_setup_test_datais session-scoped and async, but there’s no session-scopedevent_loopoverride. With pytest-asyncio defaults this can trigger “event loop closed / different loop” errors.🛠️ Suggested fix (session-scoped sync event loop)
+import asyncio from datetime import datetime, timezone+@pytest.fixture(scope='session') +def event_loop(): + """Create a session-scoped event loop for async fixtures and tests.""" + loop = asyncio.new_event_loop() + yield loop + loop.close()pytest-asyncio session-scoped async fixture event_loop loop_scope
🧹 Nitpick comments (1)
tests/conftest.py (1)
50-188: Add teardown cleanup for created accounts/budgets.
_created_account_ids/_created_budget_idsare tracked but never deleted, which leaves artifacts in long-lived Firefly instances. Consider adding a teardown phase to delete these when the session ends.♻️ Cleanup pattern
`@pytest.fixture`(scope='session', autouse=True) async def _setup_test_data(): ... - try: + try: ... + yield finally: await client._client.aclose() + # Optional cleanup after yield + if _created_account_ids or _created_budget_ids: + cleanup_client = FireflyClient() + try: + for account_id in _created_account_ids: + await cleanup_client.delete_account(account_id) + for budget_id in _created_budget_ids: + await cleanup_client.delete_budget(budget_id) + finally: + await cleanup_client._client.aclose()
- Update docstring to reflect current functionality and usage - Add endpoint accessibility verification for accounts API - Improve help text with examples for running specific test suites - Add detailed verification process description
Adds integration tests and workflow. Covers basic scenarios related to accounts, budgets and transactions.
Resolves #20