-
Notifications
You must be signed in to change notification settings - Fork 27
Code Review for the Bank API Exercise #3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
6310a2d
e4a3703
c24f409
1a4bcdd
56a9d0a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| from bank_api.bank import Bank | ||
|
|
||
|
|
||
| class BankReport: | ||
| def __init__(self, bank: Bank): | ||
| self.bank = bank | ||
|
|
||
| def get_balance(self, name: str) -> int: | ||
| account = self.bank.get_account(name) | ||
| return sum(t.amount for t in self.bank.transactions if t.account == account) |
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -16,8 +16,28 @@ def client(): | |||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| def test_account_creation(client: FlaskClient): | ||||||||||||||
| # Use the client to make requests to the Flask app. | ||||||||||||||
| # response = client.get('/example/route') | ||||||||||||||
| # Or use client.post to make a POST request | ||||||||||||||
| # https://flask.palletsprojects.com/en/1.1.x/testing/ | ||||||||||||||
| pass | ||||||||||||||
| post_response = client.post('/accounts/Alice') | ||||||||||||||
|
|
||||||||||||||
| assert post_response.status_code == 200 | ||||||||||||||
| assert post_response.get_json() == {"name": "Alice"} | ||||||||||||||
|
Comment on lines
+21
to
+22
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might be worth extracting |
||||||||||||||
|
|
||||||||||||||
| get_response = client.get('/accounts/Alice') | ||||||||||||||
|
|
||||||||||||||
| assert get_response.status_code == 200 | ||||||||||||||
| assert get_response.get_json() == {"name": "Alice", "balance": 0} | ||||||||||||||
|
Comment on lines
+26
to
+27
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
Suggested change
This has 2 (potential) benefits:
|
||||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| def test_get_account_returns_404_if_not_found(client: FlaskClient): | ||||||||||||||
| response = client.get('/accounts/Nobody') | ||||||||||||||
|
|
||||||||||||||
| assert response.status_code == 404 | ||||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| def test_get_account_includes_balance(client: FlaskClient): | ||||||||||||||
| client.post('/accounts/Alice') | ||||||||||||||
| client.post('/money', json={'name': 'Alice', 'amount': 1000}) | ||||||||||||||
|
|
||||||||||||||
| response = client.get('/accounts/Alice') | ||||||||||||||
|
|
||||||||||||||
| assert response.status_code == 200 | ||||||||||||||
| assert response.get_json()['balance'] == 1000 | ||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -32,5 +32,33 @@ def test_get_account_raises_error_if_no_account_matches(bank: Bank): | |||||||||||||||||||||||||||||||||||||||
| with pytest.raises(ValueError): | ||||||||||||||||||||||||||||||||||||||||
| bank.get_account('Name 2') | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| # TODO: Add unit tests for bank.add_funds() | ||||||||||||||||||||||||||||||||||||||||
| # --- add_funds() tests --- | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| def test_add_funds_creates_transaction(bank: Bank): | ||||||||||||||||||||||||||||||||||||||||
| bank.create_account('Alice') | ||||||||||||||||||||||||||||||||||||||||
| bank.add_funds('Alice', 1000) | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| assert len(bank.transactions) == 1 | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| 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 | ||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+43
to
+47
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These tests could be written in a more Arrange/Act/Assert style e.g.
Suggested change
It's more verbose but arguably easier to understand (following the principle of self-documenting code) |
||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| def test_add_funds_raises_error_if_account_not_found(bank: Bank): | ||||||||||||||||||||||||||||||||||||||||
| with pytest.raises(ValueError): | ||||||||||||||||||||||||||||||||||||||||
| bank.add_funds('Nobody', 100) | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| def test_add_funds_raises_error_if_amount_is_negative(bank: Bank): | ||||||||||||||||||||||||||||||||||||||||
| bank.create_account('Alice') | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| with pytest.raises(ValueError): | ||||||||||||||||||||||||||||||||||||||||
| bank.add_funds('Alice', -50) | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| def test_add_funds_raises_error_if_amount_is_zero(bank: Bank): | ||||||||||||||||||||||||||||||||||||||||
| bank.create_account('Alice') | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| with pytest.raises(ValueError): | ||||||||||||||||||||||||||||||||||||||||
| bank.add_funds('Alice', 0) | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| """Unit tests for bank_report.py""" | ||
|
|
||
| from datetime import datetime | ||
|
|
||
| import pytest | ||
|
|
||
| from bank_api.bank import Account, Bank, Transaction | ||
| from bank_api.bank_report import BankReport | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def bank() -> Bank: | ||
| return Bank() | ||
|
|
||
|
|
||
| def test_balance_is_zero_with_no_transactions(bank: Bank, monkeypatch): | ||
| bank_report = BankReport(bank) | ||
| account = Account('Alice') | ||
| monkeypatch.setattr(bank, 'get_account', lambda name: account) | ||
| monkeypatch.setattr(bank, 'transactions', []) | ||
|
Comment on lines
+19
to
+20
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (More the exercise's problem than yours but) Be aware that |
||
|
|
||
| assert bank_report.get_balance('Alice') == 0 | ||
|
|
||
|
|
||
| def test_balance_sums_transactions(bank: Bank, monkeypatch): | ||
| bank_report = BankReport(bank) | ||
| account = Account('Alice') | ||
| monkeypatch.setattr(bank, 'get_account', lambda name: account) | ||
| monkeypatch.setattr(bank, 'transactions', [ | ||
| Transaction(account, datetime.now(), 500), | ||
| Transaction(account, datetime.now(), 300), | ||
| ]) | ||
|
|
||
| assert bank_report.get_balance('Alice') == 800 | ||
|
|
||
|
|
||
| def test_balance_ignores_other_accounts_transactions(bank: Bank, monkeypatch): | ||
| bank_report = BankReport(bank) | ||
| alice = Account('Alice') | ||
| bob = Account('Bob') | ||
| monkeypatch.setattr(bank, 'get_account', lambda name: alice) | ||
| monkeypatch.setattr(bank, 'transactions', [ | ||
| Transaction(alice, datetime.now(), 1000), | ||
| Transaction(bob, datetime.now(), 200), | ||
| ]) | ||
|
|
||
| assert bank_report.get_balance('Alice') == 1000 | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).