Pytest Code Review

Pytest Code Review

Quick Reference

Review gates

Work in order. Do not assert pytest-specific problems until each applicable gate passes.

1. Scoped files — Pass when: You list every test_*.py and any conftest.py you will cite; no findings for files outside that list. 2. Async vs sync — Pass when: Per scoped file, you note whether it uses async def test_* / await; if yes, open references/async-testing.md before criticizing async usage. 3. Fixtures — Pass when: If shared setup matters, you name the conftest.py path(s) or state none; for yield fixtures, confirm cleanup exists before claiming resource leaks. 4. patch / mocks — Pass when: For any patch or mock critique, you give the import path where the symbol is used (call site), or mark N/A; open references/mocking.md when mocking is central to the review. 5. Findings — Pass when: Each finding includes a file path and line(s) or test node id, not a generic rule restatement.

Review Checklist

• [ ] Test functions are async def test_* for async code under test
• [ ] AsyncMock used for async dependencies, not Mock
• [ ] All async mocks and coroutines are awaited
• [ ] Fixtures in conftest.py for shared setup
• [ ] Fixture scope appropriate (function, class, module, session)
• [ ] Yield fixtures have proper cleanup in finally block
• [ ] @pytest.mark.parametrize for similar test cases
• [ ] No duplicated test logic across multiple test functions
• [ ] Mocks track calls properly (assert_called_once_with)
• [ ] patch() targets correct location (where used, not defined)
• [ ] No mocking of internals that should be tested
• [ ] Test isolation (no shared mutable state between tests)

When to Load References

• Reviewing async test functions → async-testing.md
• Reviewing fixtures or conftest.py → fixtures.md
• Reviewing similar test cases → parametrize.md
• Reviewing mocks and patches → mocking.md

Review Questions

1. Are all async functions tested with async def test_*? 2. Are fixtures properly scoped with appropriate cleanup? 3. Can similar test cases be parametrized to reduce duplication? 4. Are mocks tracking calls and used at the right locations?

Async Testing

Critical Anti-Patterns

1. Using Mock Instead of AsyncMock

Problem: Mock returns a regular Mock object, not a coroutine. Tests pass but don't actually test async behavior.

# BAD - Mock doesn't work with async
from unittest.mock import Mock

@pytest.mark.asyncio
async def test_fetch_data():
    mock_client = Mock()
    mock_client.get.return_value = {"data": "test"}

    # This won't work! mock_client.get() is not awaitable
    result = await fetch_data(mock_client)  # TypeError!

# GOOD - AsyncMock for async functions
from unittest.mock import AsyncMock

@pytest.mark.asyncio
async def test_fetch_data():
    mock_client = AsyncMock()
    mock_client.get.return_value = {"data": "test"}

    result = await fetch_data(mock_client)
    assert result == {"data": "test"}

2. Forgetting @pytest.mark.asyncio

Problem: Test function is not run as coroutine, async code never executes.

# BAD - missing decorator
async def test_process_data():
    result = await process_data()  # Never actually awaited!
    assert result == expected

# GOOD - proper async test
@pytest.mark.asyncio
async def test_process_data():
    result = await process_data()
    assert result == expected

3. Not Awaiting Async Mocks

Problem: Mock returns coroutine object instead of actual value.

# BAD - not awaiting AsyncMock
@pytest.mark.asyncio
async def test_service():
    mock_db = AsyncMock()
    mock_db.query.return_value = [{"id": 1}]

    service = UserService(mock_db)
    result = service.get_users()  # Returns coroutine, not list!
    assert len(result) == 1  # TypeError!

# GOOD - await AsyncMock
@pytest.mark.asyncio
async def test_service():
    mock_db = AsyncMock()
    mock_db.query.return_value = [{"id": 1}]

    service = UserService(mock_db)
    result = await service.get_users()
    assert len(result) == 1

4. Mixing Sync and Async in Tests

Problem: Calling sync blocking code in async test defeats purpose.

# BAD - mixing sync and async
@pytest.mark.asyncio
async def test_user_flow():
    user = create_user_sync()  # Blocking call!
    time.sleep(1)  # Blocks event loop!
    result = await process_user(user)
    assert result.processed

# GOOD - fully async
@pytest.mark.asyncio
async def test_user_flow():
    user = await create_user_async()
    await asyncio.sleep(1)
    result = await process_user(user)
    assert result.processed

5. Not Cleaning Up Async Resources

Problem: Background tasks, connections, or coroutines left running after test.

# BAD - no cleanup
@pytest.mark.asyncio
async def test_background_task():
    task = asyncio.create_task(long_running_operation())
    # Task still running after test ends!
    result = await some_other_operation()
    assert result

# GOOD - proper cleanup
@pytest.mark.asyncio
async def test_background_task():
    task = asyncio.create_task(long_running_operation())
    try:
        result = await some_other_operation()
        assert result
    finally:
        task.cancel()
        try:
            await task
        except asyncio.CancelledError:
            pass

6. Not Testing Concurrent Behavior

Problem: Tests run sequentially, missing race conditions and timing issues.

# BAD - sequential testing of concurrent code
@pytest.mark.asyncio
async def test_concurrent_updates():
    await update_counter()
    await update_counter()
    # Doesn't test actual concurrent access!
    assert get_counter() == 2

# GOOD - actually test concurrency
@pytest.mark.asyncio
async def test_concurrent_updates():
    results = await asyncio.gather(
        update_counter(),
        update_counter(),
        update_counter()
    )
    # Tests actual concurrent behavior
    assert get_counter() == 3

7. Using pytest-asyncio Without Configuration

Problem: Tests may not run in correct mode or fail silently.

# BAD - no configuration, ambiguous mode
# test_something.py
async def test_feature():  # Might not run as async!
    result = await process()
    assert result

# GOOD - explicit configuration
# pyproject.toml or pytest.ini
[tool.pytest.ini_options]
asyncio_mode = "auto"

# test_something.py
import pytest

@pytest.mark.asyncio
async def test_feature():
    result = await process()
    assert result

8. Not Testing Exception Paths in Async Code

Problem: Async exceptions behave differently, need explicit testing.

# BAD - not testing async exceptions
@pytest.mark.asyncio
async def test_error_handling():
    # Doesn't verify exception is properly raised
    result = await fetch_data_with_retry()
    assert result

# GOOD - test async exception handling
@pytest.mark.asyncio
async def test_error_handling():
    mock_client = AsyncMock()
    mock_client.get.side_effect = asyncio.TimeoutError()

    with pytest.raises(asyncio.TimeoutError):
        await fetch_data(mock_client)

Review Questions

1. Are all async test functions marked with @pytest.mark.asyncio? 2. Are AsyncMock objects used instead of Mock for async dependencies? 3. Are all coroutines and async mocks properly awaited? 4. Are async resources (tasks, connections) cleaned up after tests? 5. Do concurrent code tests actually run operations concurrently? 6. Is pytest-asyncio configured correctly in pyproject.toml or pytest.ini?

Fixtures

Critical Anti-Patterns

1. Duplicated Setup Across Tests

Problem: Same setup repeated in every test function instead of using fixtures.

# BAD - duplicated setup
@pytest.mark.asyncio
async def test_create_user():
    db = await create_db_connection()
    await db.setup_schema()
    result = await create_user(db, "Alice")
    await db.close()
    assert result.name == "Alice"

@pytest.mark.asyncio
async def test_delete_user():
    db = await create_db_connection()
    await db.setup_schema()
    result = await delete_user(db, 1)
    await db.close()
    assert result.success

# GOOD - fixture handles setup/teardown
@pytest.fixture
async def db():
    connection = await create_db_connection()
    await connection.setup_schema()
    yield connection
    await connection.close()

@pytest.mark.asyncio
async def test_create_user(db):
    result = await create_user(db, "Alice")
    assert result.name == "Alice"

@pytest.mark.asyncio
async def test_delete_user(db):
    result = await delete_user(db, 1)
    assert result.success

2. Missing Cleanup in Fixtures

Problem: Resources leak when tests fail or fixtures don't clean up.

# BAD - no cleanup
@pytest.fixture
async def temp_file():
    file_path = "/tmp/test_file.txt"
    async with aiofiles.open(file_path, "w") as f:
        await f.write("test data")
    return file_path
    # File never deleted!

# GOOD - cleanup with yield
@pytest.fixture
async def temp_file():
    file_path = "/tmp/test_file.txt"
    async with aiofiles.open(file_path, "w") as f:
        await f.write("test data")
    yield file_path
    # Cleanup always runs
    if os.path.exists(file_path):
        os.remove(file_path)

3. Wrong Fixture Scope

Problem: Expensive setup repeated unnecessarily or shared state causes test coupling.

# BAD - function scope for expensive operation
@pytest.fixture(scope="function")  # Runs for EVERY test!
def database_with_seed_data():
    db = create_database()
    seed_large_dataset(db)  # Takes 10 seconds!
    return db

# GOOD - module scope for expensive, read-only setup
@pytest.fixture(scope="module")
def database_with_seed_data():
    db = create_database()
    seed_large_dataset(db)
    yield db
    db.cleanup()

# BAD - session scope for mutable state
@pytest.fixture(scope="session")
def user_cache():
    return {}  # Shared across ALL tests - race conditions!

# GOOD - function scope for mutable state
@pytest.fixture(scope="function")
def user_cache():
    return {}  # Fresh cache per test

4. Not Using conftest.py

Problem: Fixtures duplicated across test files.

# BAD - fixture in test_users.py
@pytest.fixture
def db_session():
    session = create_session()
    yield session
    session.close()

# BAD - same fixture duplicated in test_posts.py
@pytest.fixture
def db_session():
    session = create_session()
    yield session
    session.close()

# GOOD - shared fixture in conftest.py
# conftest.py
@pytest.fixture
def db_session():
    session = create_session()
    yield session
    session.close()

# test_users.py and test_posts.py can both use db_session

5. Factory Fixtures Not Used for Variations

Problem: Creating multiple similar fixtures instead of one factory.

# BAD - separate fixture for each variation
@pytest.fixture
def user_alice():
    return User(name="Alice", role="admin")

@pytest.fixture
def user_bob():
    return User(name="Bob", role="user")

@pytest.fixture
def user_charlie():
    return User(name="Charlie", role="guest")

# GOOD - factory fixture
@pytest.fixture
def make_user():
    def _make_user(name: str, role: str = "user"):
        return User(name=name, role=role)
    return _make_user

def test_admin_access(make_user):
    admin = make_user("Alice", role="admin")
    assert admin.can_delete()

def test_user_access(make_user):
    user = make_user("Bob")
    assert not user.can_delete()

6. Fixture Dependencies Not Leveraged

Problem: Manually composing dependencies instead of using fixture chaining.

# BAD - manual composition
@pytest.fixture
def authenticated_client():
    app = create_app()
    client = TestClient(app)
    user = create_user()
    token = generate_token(user)
    client.headers["Authorization"] = f"Bearer {token}"
    return client

# GOOD - fixture chaining
@pytest.fixture
def app():
    return create_app()

@pytest.fixture
def client(app):
    return TestClient(app)

@pytest.fixture
def user():
    return create_user()

@pytest.fixture
def auth_token(user):
    return generate_token(user)

@pytest.fixture
def authenticated_client(client, auth_token):
    client.headers["Authorization"] = f"Bearer {auth_token}"
    return client

7. Autouse Fixtures Overused

Problem: Autouse fixtures run even when not needed, slowing tests.

# BAD - autouse for specific setup
@pytest.fixture(autouse=True)
def setup_database():
    # Runs for EVERY test, even ones that don't use database
    db = setup_test_db()
    yield
    db.teardown()

# GOOD - explicit fixture dependency
@pytest.fixture
def database():
    db = setup_test_db()
    yield db
    db.teardown()

def test_with_db(database):
    # Only runs when explicitly requested
    assert database.is_connected()

def test_without_db():
    # Doesn't pay database setup cost
    assert 1 + 1 == 2

8. Async Fixtures Without Proper Cleanup

Problem: Async cleanup not wrapped in try/finally.

# BAD - no try/finally in async fixture
@pytest.fixture
async def api_client():
    client = AsyncClient(base_url="http://test")
    yield client
    await client.close()  # Skipped if test fails!

# GOOD - try/finally ensures cleanup
@pytest.fixture
async def api_client():
    client = AsyncClient(base_url="http://test")
    try:
        yield client
    finally:
        await client.close()

9. Using Fixtures as Data Instead of Setup

Problem: Fixtures return data instead of managing resources.

# BAD - fixture just returns data
@pytest.fixture
def sample_users():
    return [
        {"name": "Alice", "role": "admin"},
        {"name": "Bob", "role": "user"}
    ]

# GOOD - use module-level constant
SAMPLE_USERS = [
    {"name": "Alice", "role": "admin"},
    {"name": "Bob", "role": "user"}
]

# ACCEPTABLE - fixture when setup/teardown needed
@pytest.fixture
async def sample_users(db):
    users = await db.create_users([
        {"name": "Alice", "role": "admin"},
        {"name": "Bob", "role": "user"}
    ])
    yield users
    await db.delete_users([u.id for u in users])

Review Questions

1. Are fixtures in conftest.py for cross-file reuse? 2. Do all fixtures with resources have proper yield + cleanup? 3. Is fixture scope appropriate (function/module/session)? 4. Are factory fixtures used for creating test data variations? 5. Are fixture dependencies chained instead of manually composed? 6. Are autouse fixtures limited to truly universal setup? 7. Do async fixtures wrap cleanup in try/finally blocks?

Mocking

Critical Anti-Patterns

1. Patching Where Defined Instead of Where Used

Problem: Mock doesn't affect the code under test because patch location is wrong.

# module_a.py
def external_api_call():
    return "real data"

# module_b.py
from module_a import external_api_call

def process_data():
    return external_api_call()

# BAD - patching where defined
from unittest.mock import patch

@patch("module_a.external_api_call")  # Wrong location!
def test_process_data(mock_api):
    mock_api.return_value = "mocked data"
    result = process_data()
    assert result == "mocked data"  # FAILS! Uses real function

# GOOD - patch where used
@patch("module_b.external_api_call")  # Patch in module_b namespace
def test_process_data(mock_api):
    mock_api.return_value = "mocked data"
    result = process_data()
    assert result == "mocked data"  # Works!

2. Not Verifying Mock Calls

Problem: Mock used but never verified, test doesn't validate behavior.

# BAD - mock not verified
@pytest.mark.asyncio
async def test_user_creation(mocker):
    mock_db = mocker.AsyncMock()
    mock_db.insert.return_value = {"id": 1}

    await create_user(mock_db, "Alice")
    # No verification! Did it call insert? With what args?

# GOOD - verify mock calls
@pytest.mark.asyncio
async def test_user_creation(mocker):
    mock_db = mocker.AsyncMock()
    mock_db.insert.return_value = {"id": 1}

    result = await create_user(mock_db, "Alice")

    mock_db.insert.assert_called_once_with({"name": "Alice"})
    assert result["id"] == 1

3. Over-Mocking Internal Implementation

Problem: Mocking internal details that should be tested, not mocked.

# BAD - mocking internal helper that should be tested
class UserService:
    def _validate_email(self, email):
        return "@" in email

    def create_user(self, email):
        if self._validate_email(email):
            return User(email=email)
        raise ValueError("Invalid email")

@patch.object(UserService, "_validate_email")
def test_create_user(mock_validate):
    mock_validate.return_value = True
    service = UserService()
    user = service.create_user("invalid")  # Should fail but doesn't!
    assert user.email == "invalid"

# GOOD - test the actual behavior
def test_create_user_valid():
    service = UserService()
    user = service.create_user("valid@example.com")
    assert user.email == "valid@example.com"

def test_create_user_invalid():
    service = UserService()
    with pytest.raises(ValueError, match="Invalid email"):
        service.create_user("invalid")

4. Using Mock Instead of AsyncMock for Async

Problem: Regular Mock doesn't work properly with async code.

# BAD - Mock for async function
from unittest.mock import Mock

@pytest.mark.asyncio
async def test_fetch_data():
    mock_client = Mock()
    mock_client.get = Mock(return_value={"data": "test"})

    result = await fetch_data(mock_client)  # TypeError: object Mock can't be used in 'await'

# GOOD - AsyncMock for async functions
from unittest.mock import AsyncMock

@pytest.mark.asyncio
async def test_fetch_data():
    mock_client = AsyncMock()
    mock_client.get.return_value = {"data": "test"}

    result = await fetch_data(mock_client)
    assert result == {"data": "test"}

5. Not Resetting Mocks Between Tests

Problem: Mock state leaks between tests, causing flaky failures.

# BAD - shared mock across tests
mock_api = Mock()

def test_first_call():
    mock_api.fetch.return_value = "data1"
    result = process(mock_api)
    assert result == "data1"

def test_second_call():
    # Mock still has state from test_first_call!
    mock_api.fetch.return_value = "data2"
    assert mock_api.fetch.call_count == 0  # FAILS! call_count is 1

# GOOD - fresh mock per test with fixture
@pytest.fixture
def mock_api():
    return Mock()

def test_first_call(mock_api):
    mock_api.fetch.return_value = "data1"
    result = process(mock_api)
    assert result == "data1"

def test_second_call(mock_api):
    mock_api.fetch.return_value = "data2"
    result = process(mock_api)
    assert mock_api.fetch.call_count == 1  # Works!

6. Not Using side_effect for Complex Behavior

Problem: Using return_value when mock needs to raise exceptions or vary responses.

# BAD - can't test retry logic with simple return_value
@pytest.mark.asyncio
async def test_retry_on_failure():
    mock_client = AsyncMock()
    mock_client.get.return_value = {"data": "test"}  # Always succeeds!

    result = await fetch_with_retry(mock_client)
    # Can't test retry behavior!

# GOOD - use side_effect for sequence of responses
@pytest.mark.asyncio
async def test_retry_on_failure():
    mock_client = AsyncMock()
    mock_client.get.side_effect = [
        asyncio.TimeoutError(),  # First call fails
        asyncio.TimeoutError(),  # Second call fails
        {"data": "test"}  # Third call succeeds
    ]

    result = await fetch_with_retry(mock_client, max_retries=3)
    assert result == {"data": "test"}
    assert mock_client.get.call_count == 3

# GOOD - use side_effect function for dynamic behavior
def test_dynamic_behavior():
    def side_effect_fn(user_id):
        if user_id == 1:
            return {"name": "Alice"}
        raise ValueError("User not found")

    mock_db = Mock()
    mock_db.get_user.side_effect = side_effect_fn

    assert mock_db.get_user(1) == {"name": "Alice"}
    with pytest.raises(ValueError):
        mock_db.get_user(2)

7. Not Using spec or spec_set

Problem: Mock accepts any attribute, allowing tests that pass but code that fails.

# BAD - mock without spec
def test_user_service():
    mock_db = Mock()
    service = UserService(mock_db)
    service.process()
    # Typo! Should be execute(), not exectue()
    mock_db.exectue.assert_called_once()  # Test passes! But code would fail

# GOOD - use spec to catch attribute errors
def test_user_service():
    mock_db = Mock(spec=Database)
    service = UserService(mock_db)
    service.process()
    # AttributeError: Mock object has no attribute 'exectue'
    mock_db.execute.assert_called_once()  # Forces correct spelling

8. Patching with Context Manager but Not Using It

Problem: Using patch as decorator when context manager is clearer for partial mocking.

# BAD - decorator for partial test mocking
@patch("module.external_call")
def test_process(mock_call):
    mock_call.return_value = "mocked"
    # First part of test uses mock
    result1 = process_with_external()
    assert result1 == "mocked"

    # Second part wants real call, but can't!
    result2 = process_with_external()  # Still mocked!

# GOOD - context manager for scoped mocking
def test_process():
    # First part uses mock
    with patch("module.external_call") as mock_call:
        mock_call.return_value = "mocked"
        result1 = process_with_external()
        assert result1 == "mocked"

    # Second part uses real function
    result2 = process_with_external()
    assert result2 != "mocked"

9. Not Checking Call Arguments Precisely

Problem: Using assert_called() instead of assert_called_with().

# BAD - only checks if called, not what arguments
@pytest.mark.asyncio
async def test_create_user():
    mock_db = AsyncMock()
    await create_user(mock_db, name="Alice", email="alice@example.com")
    mock_db.insert.assert_called()  # Called, but with what args?

# GOOD - verify exact arguments
@pytest.mark.asyncio
async def test_create_user():
    mock_db = AsyncMock()
    await create_user(mock_db, name="Alice", email="alice@example.com")
    mock_db.insert.assert_called_once_with(
        name="Alice",
        email="alice@example.com"
    )

# ALSO GOOD - use call_args for partial matching
@pytest.mark.asyncio
async def test_create_user():
    mock_db = AsyncMock()
    await create_user(mock_db, name="Alice", email="alice@example.com")
    call_args = mock_db.insert.call_args
    assert call_args.kwargs["name"] == "Alice"
    assert "email" in call_args.kwargs

10. Mocking Entire Objects Instead of Interfaces

Problem: Mocking concrete class when interface would be more accurate.

# BAD - mocking concrete class
from unittest.mock import Mock

class PostgresDatabase:
    def query(self, sql): ...
    def execute(self, sql): ...
    def internal_connection_pool(self): ...

def test_service():
    mock_db = Mock(spec=PostgresDatabase)
    # Test knows about PostgreSQL specifics!
    service = UserService(mock_db)

# GOOD - mock interface/protocol
from typing import Protocol

class Database(Protocol):
    async def query(self, sql: str) -> list: ...
    async def execute(self, sql: str) -> None: ...

@pytest.fixture
def mock_db():
    mock = AsyncMock(spec=Database)
    return mock

def test_service(mock_db):
    # Test only depends on interface
    service = UserService(mock_db)

11. Not Using pytest-mock Plugin

Problem: Using unittest.mock directly when pytest-mock provides better integration.

# BAD - manual patch cleanup
from unittest.mock import patch

def test_feature():
    patcher = patch("module.function")
    mock_fn = patcher.start()
    mock_fn.return_value = "test"

    result = use_function()

    patcher.stop()  # Easy to forget!
    assert result == "test"

# GOOD - using pytest-mock mocker fixture
def test_feature(mocker):
    mock_fn = mocker.patch("module.function")
    mock_fn.return_value = "test"

    result = use_function()
    # Automatic cleanup!
    assert result == "test"

Review Questions

1. Are patches applied where the function is used, not where it's defined? 2. Are mock calls verified with assert_called_once_with() or similar? 3. Are internal implementation details tested rather than mocked? 4. Is AsyncMock used for all async functions and methods? 5. Are mocks fresh for each test (via fixtures or setUp)? 6. Is side_effect used for exceptions or varying responses? 7. Do mocks use spec or spec_set to catch attribute errors? 8. Are call arguments verified precisely, not just call presence? 9. Is pytest-mock used instead of raw unittest.mock?

Parametrize

Critical Anti-Patterns

1. Duplicated Test Functions for Similar Cases

Problem: Copy-pasted test functions that differ only in input values.

# BAD - duplicated test logic
@pytest.mark.asyncio
async def test_validate_email_valid():
    result = validate_email("user@example.com")
    assert result.is_valid is True

@pytest.mark.asyncio
async def test_validate_email_valid_subdomain():
    result = validate_email("user@mail.example.com")
    assert result.is_valid is True

@pytest.mark.asyncio
async def test_validate_email_invalid_no_at():
    result = validate_email("userexample.com")
    assert result.is_valid is False

@pytest.mark.asyncio
async def test_validate_email_invalid_no_domain():
    result = validate_email("user@")
    assert result.is_valid is False

# GOOD - parametrized test
@pytest.mark.asyncio
@pytest.mark.parametrize("email,expected", [
    ("user@example.com", True),
    ("user@mail.example.com", True),
    ("userexample.com", False),
    ("user@", False),
])
async def test_validate_email(email, expected):
    result = validate_email(email)
    assert result.is_valid is expected

2. Unclear Parametrize Names

Problem: Using generic names like "input" and "output" instead of descriptive names.

# BAD - unclear parameter names
@pytest.mark.parametrize("input,output", [
    (10, 100),
    (5, 25),
    (0, 0),
])
def test_calculation(input, output):
    assert calculate(input) == output

# GOOD - descriptive parameter names
@pytest.mark.parametrize("radius,expected_area", [
    (10, 314.159),
    (5, 78.539),
    (0, 0),
])
def test_circle_area(radius, expected_area):
    assert calculate_area(radius) == pytest.approx(expected_area, rel=1e-3)

3. Not Using pytest.param for IDs

Problem: Test output shows cryptic parameter values instead of meaningful descriptions.

# BAD - unclear test IDs in output
@pytest.mark.parametrize("user_role,can_access", [
    ("admin", True),
    ("user", False),
    ("guest", False),
])
def test_access_control(user_role, can_access):
    assert check_access(user_role) == can_access
# Output: test_access_control[admin-True], test_access_control[user-False]

# GOOD - descriptive test IDs
@pytest.mark.parametrize("user_role,can_access", [
    pytest.param("admin", True, id="admin_has_access"),
    pytest.param("user", False, id="user_denied"),
    pytest.param("guest", False, id="guest_denied"),
])
def test_access_control(user_role, can_access):
    assert check_access(user_role) == can_access
# Output: test_access_control[admin_has_access], test_access_control[user_denied]

4. Not Combining Multiple Parametrize Decorators

Problem: Creating cartesian product manually instead of stacking decorators.

# BAD - manual combinations
@pytest.mark.parametrize("method,status,role", [
    ("GET", 200, "admin"),
    ("GET", 200, "user"),
    ("POST", 200, "admin"),
    ("POST", 403, "user"),
    ("DELETE", 200, "admin"),
    ("DELETE", 403, "user"),
])
def test_api_access(method, status, role):
    assert api_call(method, role).status_code == status

# GOOD - stacked parametrize for cartesian product
@pytest.mark.parametrize("method", ["GET", "POST", "DELETE"])
@pytest.mark.parametrize("role,expected_statuses", [
    ("admin", {"GET": 200, "POST": 200, "DELETE": 200}),
    ("user", {"GET": 200, "POST": 403, "DELETE": 403}),
])
def test_api_access(method, role, expected_statuses):
    assert api_call(method, role).status_code == expected_statuses[method]

# ALTERNATIVE - if all admins succeed and users fail writes
@pytest.mark.parametrize("method", ["GET", "POST", "DELETE"])
@pytest.mark.parametrize("role,can_write", [
    ("admin", True),
    ("user", False),
])
def test_api_access(method, role, can_write):
    response = api_call(method, role)
    if method in ["POST", "DELETE"] and not can_write:
        assert response.status_code == 403
    else:
        assert response.status_code == 200

5. Parametrizing Fixtures Instead of Tests

Problem: Complex parametrized fixtures when parametrized tests would be clearer.

# BAD - parametrized fixture is hard to read
@pytest.fixture(params=[
    {"name": "Alice", "role": "admin", "can_delete": True},
    {"name": "Bob", "role": "user", "can_delete": False},
])
def user(request):
    return User(**request.param)

def test_user_permissions(user):
    assert user.can_delete() == user.expected_can_delete

# GOOD - parametrize the test
@pytest.fixture
def make_user():
    def _make(name: str, role: str):
        return User(name=name, role=role)
    return _make

@pytest.mark.parametrize("name,role,can_delete", [
    ("Alice", "admin", True),
    ("Bob", "user", False),
])
def test_user_permissions(make_user, name, role, can_delete):
    user = make_user(name, role)
    assert user.can_delete() == can_delete

6. Not Marking Expected Failures

Problem: Including known failing cases without marking them.

# BAD - test fails on known edge case
@pytest.mark.parametrize("input,expected", [
    ("valid", True),
    ("also_valid", True),
    ("edge_case", True),  # This actually fails but is being worked on
])
def test_validator(input, expected):
    assert validate(input) == expected

# GOOD - mark expected failures
@pytest.mark.parametrize("input,expected", [
    ("valid", True),
    ("also_valid", True),
    pytest.param("edge_case", True, marks=pytest.mark.xfail(reason="Issue #123")),
])
def test_validator(input, expected):
    assert validate(input) == expected

7. Large Parametrize Tables in Test File

Problem: Test file cluttered with large data tables.

# BAD - 100 lines of test data inline
@pytest.mark.parametrize("input,expected", [
    ("case1", "result1"),
    ("case2", "result2"),
    # ... 100 more lines ...
])
def test_parser(input, expected):
    assert parse(input) == expected

# GOOD - externalize large datasets
# test_data/parser_cases.json
[
    {"input": "case1", "expected": "result1"},
    {"input": "case2", "expected": "result2"}
]

# test_parser.py
import json
from pathlib import Path

def load_test_cases():
    path = Path(__file__).parent / "test_data" / "parser_cases.json"
    with open(path) as f:
        cases = json.load(f)
    return [(c["input"], c["expected"]) for c in cases]

@pytest.mark.parametrize("input,expected", load_test_cases())
def test_parser(input, expected):
    assert parse(input) == expected

8. Not Using Indirect Parametrization

Problem: Creating expensive test data for every parameter combination.

# BAD - creating full database for each test
@pytest.mark.parametrize("user_id,expected_name", [
    (1, "Alice"),
    (2, "Bob"),
    (3, "Charlie"),
])
async def test_get_user(user_id, expected_name):
    db = await create_full_database()  # Expensive! Runs 3 times!
    user = await db.get_user(user_id)
    assert user.name == expected_name

# GOOD - indirect parametrization with fixture
@pytest.fixture
async def db_with_users():
    db = await create_full_database()
    yield db
    await db.cleanup()

@pytest.mark.parametrize("user_id,expected_name", [
    (1, "Alice"),
    (2, "Bob"),
    (3, "Charlie"),
])
async def test_get_user(db_with_users, user_id, expected_name):
    user = await db_with_users.get_user(user_id)
    assert user.name == expected_name

# EVEN BETTER - indirect parametrization
@pytest.fixture
async def user(request, db):
    user_id = request.param
    return await db.get_user(user_id)

@pytest.mark.parametrize("user,expected_name", [
    (1, "Alice"),
    (2, "Bob"),
    (3, "Charlie"),
], indirect=["user"])
async def test_user_name(user, expected_name):
    assert user.name == expected_name

9. Testing Multiple Assertions Instead of Separating

Problem: Multiple unrelated assertions in one parametrized test.

# BAD - multiple unrelated assertions
@pytest.mark.parametrize("user_data", [
    {"name": "Alice", "age": 30, "role": "admin"},
    {"name": "Bob", "age": 25, "role": "user"},
])
def test_user(user_data):
    user = User(**user_data)
    assert user.name == user_data["name"]
    assert user.age == user_data["age"]
    assert user.role == user_data["role"]
    assert user.is_valid()  # Unrelated to data validation
    assert len(user.name) > 0  # Different concern

# GOOD - separate tests for different concerns
@pytest.mark.parametrize("name,age,role", [
    ("Alice", 30, "admin"),
    ("Bob", 25, "user"),
])
def test_user_creation(name, age, role):
    user = User(name=name, age=age, role=role)
    assert user.name == name
    assert user.age == age
    assert user.role == role

@pytest.mark.parametrize("name", ["Alice", "Bob", ""])
def test_user_name_validation(name):
    if name:
        user = User(name=name, age=30, role="user")
        assert user.is_valid()
    else:
        with pytest.raises(ValueError):
            User(name=name, age=30, role="user")

Review Questions

1. Can duplicated test functions be combined with parametrize? 2. Do parametrized tests use descriptive parameter names? 3. Are test IDs meaningful using pytest.param(id="...")? 4. Should multiple parametrize decorators be stacked for combinations? 5. Are large test datasets externalized to separate files? 6. Is indirect parametrization used for expensive fixture setup?