Code Review for the Bank API Exercise#3
Code Review for the Bank API Exercise#3Jonesey13 wants to merge 5 commits intocorndeladmin:masterfrom
Conversation
- Added 5 unit tests covering add_funds() happy path, error on missing account, and rejection of zero/negative amounts - Fixed bug in Bank.add_funds(): amount <= 0 now raises ValueError
- test_account_creation: POST creates account, GET retrieves it, both return 200 and correct JSON body - test_get_account_returns_404_if_not_found: verifies 404 on missing account
- New BankReport class in bank_api/bank_report.py with get_balance(name) that sums transactions for the named account - 3 unit tests: zero balance, summing multiple transactions, account isolation - Tests use real Bank instance (to be mocked in StretchC)
- Import BankReport into app.py and wire it to the bank instance
- GET /accounts/<name> now returns {"name": ..., "balance": ...}
- Added integration test asserting balance reflects added funds
- Updated existing GET assertion to include balance: 0
- Replaced naive tests (using real Bank) with monkeypatched equivalents - get_account mocked via lambda; transactions replaced with controlled list - Isolation verified: breaking Bank.get_account fails test_bank.py but leaves test_bank_report.py fully green
Jonesey13
left a comment
Jonesey13
left a comment
There was a problem hiding this comment.
Looks great! All seems to work in my testing. My comments are mostly code quality comments rather than functional ones.
| if amount <= 0: | ||
| raise ValueError("Amount must be a positive integer") |
There was a problem hiding this comment.
This currently surfaces itself as a 500 error in the API but it's probably better suited as a 4XX error (i.e. a client error rather than a server error).
| assert post_response.status_code == 200 | ||
| assert post_response.get_json() == {"name": "Alice"} |
There was a problem hiding this comment.
Might be worth extracting "Alice" as a test constant here.
| assert get_response.status_code == 200 | ||
| assert get_response.get_json() == {"name": "Alice", "balance": 0} |
There was a problem hiding this comment.
It's worth discussing whether testing the precise structure of the JSON response is in scope for this test. For example you could instead write this as:
| assert get_response.status_code == 200 | |
| assert get_response.get_json() == {"name": "Alice", "balance": 0} | |
| assert get_response.status_code == 200 | |
| body_json = get_response.get_json() | |
| assert body_json["name"] == "Alice" | |
| assert body_json["balance"] == 0 |
This has 2 (potential) benefits:
- If the shape of the response changes/extends to more properties this test will not break (and arguably shouldn't break as an account is still created
- The test will fail on a specific attribute (rather than on the entire object)
| def test_add_funds_transaction_has_correct_amount(bank: Bank): | ||
| bank.create_account('Alice') | ||
| bank.add_funds('Alice', 500) | ||
|
|
||
| assert bank.transactions[0].amount == 500 |
There was a problem hiding this comment.
These tests could be written in a more Arrange/Act/Assert style e.g.
| def test_add_funds_transaction_has_correct_amount(bank: Bank): | |
| bank.create_account('Alice') | |
| bank.add_funds('Alice', 500) | |
| assert bank.transactions[0].amount == 500 | |
| def test_add_funds_transaction_has_correct_amount(bank: Bank): | |
| # Arrange | |
| test_account = 'Alice' | |
| test_amount = 500 | |
| bank.create_account(test_account) | |
| # Act | |
| bank.add_funds(test_account , test_amount) | |
| # Assert | |
| assert len(bank.transactions) == 1 | |
| created_transaction = bank.transactions[0] | |
| assert created_transaction.amount == test_amount | |
| assert created_transaction.account.name == test_account |
It's more verbose but arguably easier to understand (following the principle of self-documenting code)
| monkeypatch.setattr(bank, 'get_account', lambda name: account) | ||
| monkeypatch.setattr(bank, 'transactions', []) |
There was a problem hiding this comment.
(More the exercise's problem than yours but) Be aware that monkeypatch is fairly redundant here (vs editing the object properties directly) as a new bank is created for each test. If we were calling static methods on the Bank class then monkeypatch would be perfect for this task (as using monkeypatch in a test ensures any changes do not persist between tests).
2 participants
No description provided.