MPT-19910: add e2e tests for currency endpoints#275
Merged
albertsola merged 2 commits intomainfrom Apr 2, 2026
Merged
Conversation
albertsola
changed the title
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdded Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e/exchange/currencies/conftest.py`:
- Around line 42-45: The teardown currently catches all MPTAPIError when calling
currencies_service.delete(currency.id) which hides real failures; change the
handler to only suppress expected not-found errors by checking the error's
status/code (e.g., error.status == 404 or error.code == "not_found") and
silently ignore those, but for any other MPTAPIError re-raise or log and raise
so real teardown failures surface; apply the same change to the other similar
block that calls currencies_service.delete.
In `@tests/e2e/exchange/currencies/test_async_currencies.py`:
- Around line 29-36: The test_update_currency may send an empty file because
logo_fd was already consumed by the initial create call; before calling
async_currencies_service.update in test_update_currency, reset the file-like
object's cursor (e.g., call logo_fd.seek(0)) so the uploaded file is read from
the start when async_currencies_service.update(...) is invoked; update
references: test_update_currency, async_currencies_service.update, and
async_created_currency.create.
In `@tests/e2e/exchange/currencies/test_sync_currencies.py`:
- Around line 29-33: The test test_update_currency uses logo_fd previously
consumed during creation, so reset the file pointer before reuse: ensure logo_fd
is rewound (e.g., call logo_fd.seek(0) or equivalent) right before invoking
currencies_service.update(created_currency.id, update_data, file=logo_fd) so the
upload reads the file from the beginning; check logo_fd supports seek to avoid
flaky EOF uploads.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 461d270a-eb76-4c9f-982b-6e0345f9febf
📒 Files selected for processing (6)
e2e_config.test.jsontests/e2e/exchange/__init__.pytests/e2e/exchange/currencies/__init__.pytests/e2e/exchange/currencies/conftest.pytests/e2e/exchange/currencies/test_async_currencies.pytests/e2e/exchange/currencies/test_sync_currencies.py
albertsola
force-pushed
the
MPT-19910/add-currency-endpoints-e2e-tests
branch
2 times, most recently
from
bb0d8b0 to
45d37c8
Compare
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
albertsola
force-pushed
the
MPT-19910/add-currency-endpoints-e2e-tests
branch
from
45d37c8 to
6fdb67a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/e2e/exchange/currencies/conftest.py (2)
40-43:⚠️ Potential issue | 🟠 MajorTeardown still swallows all API errors (duplicate of previous review).
The teardown continues to catch all
MPTAPIErrorexceptions without discriminating. Only 404 (resource already deleted) should be suppressed; other failures (permissions, network issues, etc.) should surface to prevent resource leaks and mask real problems.🛡️ Proposed fix to check error status
try: currencies_service.delete(currency.id) except MPTAPIError as error: - print(f"TEARDOWN - Unable to delete currency {currency.id}: {error.title}") # noqa: WPS421 + if error.status != 404: + raise🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/exchange/currencies/conftest.py` around lines 40 - 43, The teardown currently swallows all MPTAPIError exceptions in the call to currencies_service.delete(currency.id); modify the except block for MPTAPIError to inspect the HTTP status on the exception (e.g., error.status or error.status_code) and only suppress/log and continue when it equals 404 (resource already gone), otherwise re-raise the exception so permission/network/errors surface and don't get masked.
52-55:⚠️ Potential issue | 🟠 MajorAsync teardown has the same error suppression issue (duplicate).
Same as the sync version: all
MPTAPIErrorexceptions are caught without checking the status. Only 404 should be ignored; other errors must surface.🛡️ Proposed fix to check error status
try: await async_currencies_service.delete(currency.id) except MPTAPIError as error: - print(f"TEARDOWN - Unable to delete currency {currency.id}: {error.title}") # noqa: WPS421 + if error.status != 404: + raise🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/exchange/currencies/conftest.py` around lines 52 - 55, The async teardown is suppressing all MPTAPIError exceptions—update the exception handler around async_currencies_service.delete(currency.id) so it only ignores the error when the MPTAPIError indicates a 404 (e.g., check error.status == 404 or equivalent property on MPTAPIError) and re-raise or let other errors surface; keep the existing log for the 404 case but do not swallow non-404 errors.
🧹 Nitpick comments (1)
tests/e2e/exchange/currencies/conftest.py (1)
23-31: Currency code generation is functional but has minor collision risk.The digit-to-alpha translation for generating unique currency codes works correctly. Since
short_uuidis hex (0-9a-f), the first three characters could theoretically collide (e.g., "abc" → "ABC", "afc" → "AFC" differ by only one char). For sequential E2E tests this is likely acceptable, but be aware of potential collisions if tests run concurrently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/exchange/currencies/conftest.py` around lines 23 - 31, The currency code generation in fixture currency_data uses only the first three chars of short_uuid and a digit->alpha map (digit_to_alpha) which can lead to collisions in concurrent runs; update currency_data to produce a more unique code by expanding the input (e.g., use the first 4–6 characters of short_uuid or append a short random/timestamp suffix) and keep the same translation logic (digit_to_alpha) and .upper() call so existing downstream expectations remain valid; ensure the resulting code still meets any length/format constraints expected by the system.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e/exchange/currencies/conftest.py`:
- Line 43: Remove the inline "# noqa: WPS421" from the print statement in
tests/e2e/exchange/currencies/conftest.py (the print(f"TEARDOWN - Unable to
delete currency {currency.id}: {error.title}") line) and add a per-file-ignores
entry in pyproject.toml under [tool.flake8] to suppress WPS421 for this file (or
for the broader tests/e2e/**/conftest.py pattern if preferred); update the
per-file-ignores list to include the path and WPS421 so the inline suppression
can be deleted.
---
Duplicate comments:
In `@tests/e2e/exchange/currencies/conftest.py`:
- Around line 40-43: The teardown currently swallows all MPTAPIError exceptions
in the call to currencies_service.delete(currency.id); modify the except block
for MPTAPIError to inspect the HTTP status on the exception (e.g., error.status
or error.status_code) and only suppress/log and continue when it equals 404
(resource already gone), otherwise re-raise the exception so
permission/network/errors surface and don't get masked.
- Around line 52-55: The async teardown is suppressing all MPTAPIError
exceptions—update the exception handler around
async_currencies_service.delete(currency.id) so it only ignores the error when
the MPTAPIError indicates a 404 (e.g., check error.status == 404 or equivalent
property on MPTAPIError) and re-raise or let other errors surface; keep the
existing log for the 404 case but do not swallow non-404 errors.
---
Nitpick comments:
In `@tests/e2e/exchange/currencies/conftest.py`:
- Around line 23-31: The currency code generation in fixture currency_data uses
only the first three chars of short_uuid and a digit->alpha map (digit_to_alpha)
which can lead to collisions in concurrent runs; update currency_data to produce
a more unique code by expanding the input (e.g., use the first 4–6 characters of
short_uuid or append a short random/timestamp suffix) and keep the same
translation logic (digit_to_alpha) and .upper() call so existing downstream
expectations remain valid; ensure the resulting code still meets any
length/format constraints expected by the system.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: e744d9b9-f101-4864-b678-bf5a0a182f4b
📒 Files selected for processing (1)
tests/e2e/exchange/currencies/conftest.py
albertsola
changed the title
|
robcsegal
approved these changes
Apr 2, 2026
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.
Summary
Adds end-to-end tests for the
CurrenciesServiceandAsyncCurrenciesServicecovering all operations exposed by the service.Changes
tests/e2e/exchange/__init__.py— new module inittests/e2e/exchange/currencies/__init__.py— new module inittests/e2e/exchange/currencies/conftest.py— fixtures for service, currency ID, currency data, and created currency (with icon upload + teardown)tests/e2e/exchange/currencies/test_sync_currencies.py— sync e2e teststests/e2e/exchange/currencies/test_async_currencies.py— async e2e testse2e_config.test.json— addedexchange.currency.idseeded valueTests covered
Closes MPT-19910