docs: comments on app/test/CI quality (spoiler!)

.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

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,

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

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)")

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

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,30 +11,41 @@
 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")
     @patch("app.services.order_service.EmailClient")
     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")
     @patch("app.services.order_service.EmailClient")
     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