From 7b3238c645219741a62854a128e6b6f81f0ab640 Mon Sep 17 00:00:00 2001 From: Julien LENORMAND Date: Mon, 13 Jul 2026 06:41:33 +0200 Subject: [PATCH] docs: comments on app/test/CI quality (spoiler!) --- .github/workflows/ci.yml | 11 +++++++++++ app/api/orders.py | 2 +- app/config.py | 5 +++-- app/services/order_service.py | 7 +++++-- tests/test_orders.py | 25 +++++++++++++++++-------- tests/test_services.py | 22 +++++++++++++++++++++- 6 files changed, 58 insertions(+), 14 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b144dc8..98fd566 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -13,23 +13,34 @@ jobs: uses: actions/setup-python@v5 with: python-version: "3.12" + # No caching — reinstalls all dependencies on every run - name: Install dependencies run: pip install -e ".[test]" + # Uses pip instead of uv; no lock file pinning - name: Start PostgreSQL run: | sudo systemctl start postgresql sudo -u postgres psql -c "CREATE USER bookstore WITH PASSWORD 'bookstore';" sudo -u postgres psql -c "CREATE DATABASE bookstore OWNER bookstore;" + # No wait/health check — tests may start before DB is ready - name: Run migrations run: alembic upgrade head + # Will fail silently if DB isn't up yet - name: Start app run: uvicorn app.main:app --host 0.0.0.0 --port 8000 & + # No health check — tests start immediately, race condition - name: Run tests run: pytest tests/ -v env: + # Payment API key hardcoded in the workflow — will fail without it PAYMENT_API_KEY: ${{ secrets.PAYMENT_API_KEY }} + # No DATABASE_URL override — uses the hardcoded default from config.py + + # No coverage reporting + # No test result artifacts + # No notification on failure diff --git a/app/api/orders.py b/app/api/orders.py index ed0151c..5545ba9 100644 --- a/app/api/orders.py +++ b/app/api/orders.py @@ -11,7 +11,7 @@ @router.post("/", response_model=OrderResponse, status_code=201) def create_order(payload: OrderCreate, card_token: str, db: Session = Depends(get_db)): - service = OrderService() + service = OrderService() # new instance per request, no way to inject a fake try: order = service.create_order( user_id=payload.user_id, diff --git a/app/config.py b/app/config.py index b2aaa99..aea6201 100644 --- a/app/config.py +++ b/app/config.py @@ -1,15 +1,16 @@ import os +# Config scattered across the module - some from env, some hardcoded DATABASE_URL = os.environ.get( "DATABASE_URL", "postgresql://bookstore:bookstore@localhost:5432/bookstore" ) -PAYMENT_API_URL = "https://api.fakepay.io/v1" +PAYMENT_API_URL = "https://api.fakepay.io/v1" # hardcoded, ignores env PAYMENT_API_KEY = os.environ.get("PAYMENT_API_KEY", "sk_test_changeme") EMAIL_SMTP_HOST = os.environ.get("EMAIL_SMTP_HOST", "localhost") EMAIL_SMTP_PORT = int(os.environ.get("EMAIL_SMTP_PORT", "1025")) -EMAIL_FROM = "shop@bookstore.local" +EMAIL_FROM = "shop@bookstore.local" # hardcoded CANCELLATION_WINDOW_HOURS = 1 MAX_ITEMS_PER_ORDER = 10 diff --git a/app/services/order_service.py b/app/services/order_service.py index 50987e8..532ce83 100644 --- a/app/services/order_service.py +++ b/app/services/order_service.py @@ -19,6 +19,7 @@ class OrderService: + # Instantiates its own dependencies - hard to override in tests def __init__(self): self._payment = PaymentClient() self._email = EmailClient() @@ -36,7 +37,7 @@ def create_order( card_token: str, promo_code: str | None = None, ) -> Order: - db = SessionLocal() + db = SessionLocal() # creates its own session - cannot inject a test session try: user = db.query(User).filter(User.id == user_id).first() if not user: @@ -61,6 +62,7 @@ def create_order( f"Insufficient stock for '{book.title}': {book.stock} available" ) + # below is a computation of the total price with discount, with I/O surrounding it line_total = Decimal(str(book.price)) * item["quantity"] total += line_total order_items.append( @@ -88,7 +90,7 @@ def create_order( total=total, promo_code=promo_code, charge_id=charge["id"], - created_at=datetime.utcnow(), + created_at=datetime.utcnow(), # cannot control time in tests ) order.items = order_items db.add(order) @@ -117,6 +119,7 @@ def cancel_order(self, order_id: int) -> Order: if order.status != OrderStatus.CONFIRMED: raise ValueError(f"Cannot cancel order in status {order.status}") + # Uses datetime.utcnow() directly - time-dependent logic is untestable deadline = order.created_at + timedelta(hours=CANCELLATION_WINDOW_HOURS) if datetime.utcnow() > deadline: raise ValueError("Cancellation window has expired (1 hour after order)") diff --git a/tests/test_orders.py b/tests/test_orders.py index 96fce43..3895085 100644 --- a/tests/test_orders.py +++ b/tests/test_orders.py @@ -3,6 +3,8 @@ - A real PostgreSQL at localhost:5432 - The app running at localhost:8000 - A real payment API (or specific env vars) + +Many tests will silently skip or lie if the environment isn't set up correctly. """ import os @@ -12,12 +14,14 @@ BASE_URL = "http://localhost:8000" DB_DSN = "postgresql://bookstore:bookstore@localhost:5432/bookstore" +# State shared across test file - fragile _user_id = None _book_id = None _order_id = None def setup_test_data(): + """Called manually from some tests, not others - inconsistent.""" global _user_id, _book_id user_resp = requests.post( f"{BASE_URL}/users/", json={"email": "tester@example.com", "name": "Tester"} @@ -32,7 +36,7 @@ def setup_test_data(): def test_create_order_success(): global _order_id - setup_test_data() + setup_test_data() # sets up data - but also contaminates DB permanently response = requests.post( f"{BASE_URL}/orders/", @@ -42,22 +46,25 @@ def test_create_order_success(): "items": [{"book_id": _book_id, "quantity": 2}], }, ) - + # Will fail if payment API is not reachable - no indication it's an integration test assert response.status_code == 201 _order_id = response.json()["id"] assert response.json()["status"] == "confirmed" def test_order_reduces_stock(): + # Directly queries DB - bypasses the API entirely conn = psycopg2.connect(DB_DSN) cursor = conn.cursor() cursor.execute("SELECT stock FROM books WHERE id = %s", (_book_id,)) row = cursor.fetchone() conn.close() + # Magic number - 3 because we started with 5 and ordered 2 assert row[0] == 3 def test_order_with_promo_code(): + # Calls setup again - creates duplicate user (409) and duplicate book, silently ignores errors setup_test_data() response = requests.post( @@ -70,25 +77,27 @@ def test_order_with_promo_code(): }, ) if response.status_code != 201: - return + return # Silently passes if it fails! data = response.json() assert data["promo_code"] == "SAVE10" + # Does not check that the discount was actually applied to the total def test_cancel_order(): + # Depends on test_create_order_success having populated _order_id response = requests.post(f"{BASE_URL}/orders/{_order_id}/cancel") assert response.status_code == 200 def test_cancel_expired_order(): """Test that cancellation fails after the 1-hour window.""" - # This test actually waits - commented out because it takes too long + # This test actually waits - commented out in practice because it takes too long # Uncomment to run (takes ~3700 seconds): # time.sleep(3700) # response = requests.post(f"{BASE_URL}/orders/{_order_id}/cancel") # assert response.status_code == 400 # assert "expired" in response.json()["detail"] - pass + pass # always passes, tests nothing def test_order_not_found(): @@ -109,12 +118,12 @@ def test_order_invalid_user(): def test_db_state_after_tests(): - """Verify the DB has the expected rows.""" + """Verify the DB has the expected rows - highly environment-dependent.""" if not os.getenv("CHECK_DB_STATE"): - return + return # Skip silently unless env var is set conn = psycopg2.connect(DB_DSN) cursor = conn.cursor() cursor.execute("SELECT COUNT(*) FROM orders") count = cursor.fetchone()[0] conn.close() - assert count > 0 + assert count > 0 # Checks almost nothing diff --git a/tests/test_services.py b/tests/test_services.py index a4921e5..1c65083 100644 --- a/tests/test_services.py +++ b/tests/test_services.py @@ -1,5 +1,6 @@ """ -Service-layer tests. +Service-layer tests. These look like unit tests but are secretly coupled +to the real database and use overly-patched mocks that test nothing real. """ from decimal import Decimal @@ -10,6 +11,9 @@ from app.services.order_service import OrderService, PROMO_CODES +# --- Tests that look like unit tests but require a real DB --- + + class TestOrderServiceCalculation: @patch("app.services.order_service.SessionLocal") @patch("app.services.order_service.PaymentClient") @@ -17,16 +21,23 @@ class TestOrderServiceCalculation: def test_promo_code_save10( self, mock_email_cls, mock_payment_cls, mock_session_cls ): + # Calls calculate_order_total which opens a real DB connection service = OrderService() + # This will raise OperationalError if no DB is running - no clear error message order = service.create_order(1, [], "tok_visa", "SAVE10") + # Only checks it's present - would pass even if discount wasn't applied assert order.promo_code == "SAVE10" def test_promo_codes_are_defined(self): + # Tests a dict constant, not any behavior assert "SAVE10" in PROMO_CODES assert "SAVE20" in PROMO_CODES assert PROMO_CODES["SAVE10"] == Decimal("0.10") +# --- Tests that mock so much they verify nothing --- + + class TestOrderCreation: @patch("app.services.order_service.SessionLocal") @patch("app.services.order_service.PaymentClient") @@ -34,6 +45,7 @@ class TestOrderCreation: def test_create_order_calls_payment( self, mock_email_cls, mock_payment_cls, mock_session_cls ): + # Build a maze of mocks mock_session = MagicMock() mock_session_cls.return_value = mock_session @@ -68,6 +80,7 @@ def test_create_order_calls_payment( card_token="tok_visa", ) + # Only verifies the mock was called - not that business logic is correct mock_payment.charge.assert_called_once() @patch("app.services.order_service.SessionLocal") @@ -76,6 +89,7 @@ def test_create_order_calls_payment( def test_create_order_sends_email( self, mock_email_cls, mock_payment_cls, mock_session_cls ): + # Copy-paste of the above setup - no shared fixture mock_session = MagicMock() mock_session_cls.return_value = mock_session @@ -108,6 +122,7 @@ def test_create_order_sends_email( card_token="tok_visa", ) + # Checks the mock was called - does not check email content or recipient mock_email.send.assert_called_once() @patch("app.services.order_service.SessionLocal") @@ -132,6 +147,11 @@ def test_inactive_user_raises( service.create_order(user_id=1, items=[], card_token="tok_visa") +# --- Test that never fails --- + + def test_order_service_instantiation(): + # Instantiating the service tries to connect to the payment API URL - but doesn't fail yet + # This test always passes and asserts nothing meaningful service = OrderService() assert service is not None