Testing and Code Review Practices¶
Section Overview
Comprehensive testing strategies and code review processes that ensure reliability, maintain quality, and promote knowledge sharing across the development team.
Unit Testing Best Practices¶
Test Organization and Structure¶
Core Principle: Unit tests must be organized systematically to maintain clarity, scalability, and ease of maintenance, following a consistent project-wide structure.
Key Guidelines:
- Mirror the production code structure in test organization
- Group related tests logically by feature or module
- Maintain clear separation between different types of tests
- Follow consistent file naming patterns across the project
Why This Matters
Well-organized tests improve maintainability, make it easier to locate specific tests, and help ensure comprehensive coverage. Systematic organization reduces cognitive load during development and troubleshooting.
Standard Directory Structure:
src/
├── user/
│ ├── authentication.py
│ ├── profile.py
│ └── settings.py
tests/
├── unit/
│ ├── user/
│ │ ├── test_authentication.py
│ │ ├── test_profile.py
│ │ └── test_settings.py
├── integration/
├── e2e/
└── fixtures/
Test Naming Conventions¶
Pattern: test_[unit]_[scenario]_[expected_result]
Examples:
# tests/unit/user/test_authentication.py
def test_user_login_with_valid_credentials_should_succeed():
user = create_test_user()
result = authenticate_user(user.email, user.password)
assert result.is_authenticated
def test_user_login_with_invalid_password_should_fail():
user = create_test_user()
result = authenticate_user(user.email, "wrong_password")
assert not result.is_authenticated
def test_user_login_with_nonexistent_email_should_raise_error():
with pytest.raises(UserNotFoundError):
authenticate_user("nonexistent@example.com", "password")
// tests/unit/user/authentication.test.js
describe('User Authentication', () => {
test('should authenticate successfully with valid credentials', () => {
const user = createTestUser();
const result = authenticateUser(user.email, user.password);
expect(result.isAuthenticated).toBe(true);
});
test('should reject login with invalid password', () => {
const user = createTestUser();
const result = authenticateUser(user.email, 'wrongPassword');
expect(result.isAuthenticated).toBe(false);
});
test('should throw error when user does not exist', () => {
expect(() => {
authenticateUser('nonexistent@example.com', 'password');
}).toThrow(UserNotFoundError);
});
});
// src/test/java/com/company/user/AuthenticationTest.java
public class AuthenticationTest {
@Test
public void shouldAuthenticateSuccessfullyWithValidCredentials() {
User user = createTestUser();
AuthResult result = authenticateUser(user.getEmail(), user.getPassword());
assertTrue(result.isAuthenticated());
}
@Test
public void shouldRejectLoginWithInvalidPassword() {
User user = createTestUser();
AuthResult result = authenticateUser(user.getEmail(), "wrongPassword");
assertFalse(result.isAuthenticated());
}
@Test(expected = UserNotFoundException.class)
public void shouldThrowExceptionWhenUserDoesNotExist() {
authenticateUser("nonexistent@example.com", "password");
}
}
Naming Guidelines
- Use complete words, avoid abbreviations
- Names should read as specifications
- Include edge cases and conditions
- Follow language-specific conventions (snake_case vs camelCase)
Mocking and Stubbing Strategies¶
Core Principle: Tests must isolate the unit under test from its dependencies through effective use of mocks, stubs, and test doubles.
When to Use Mocks:
| Scenario | Approach |
|---|---|
| External APIs | Mock to avoid network calls |
| Database Operations | Use in-memory or test instance |
| File System | Mock file operations |
| Time-dependent Code | Mock time functions |
| Third-party Services | Stub expected responses |
Implementation Examples:
# Using pytest-mock
def test_user_service_sends_welcome_email(mocker):
# Arrange: Mock the email service
mock_email = mocker.patch('services.EmailService')
mock_email.send_welcome.return_value = True
user_service = UserService(mock_email)
# Act
user = user_service.create_user("test@example.com", "password")
# Assert
mock_email.send_welcome.assert_called_once_with(
"test@example.com",
user_name=user.name
)
def test_payment_processing_handles_timeout(mocker):
# Mock gateway to simulate timeout
mock_gateway = mocker.Mock()
mock_gateway.process.side_effect = TimeoutError("Timeout")
payment_service = PaymentService(mock_gateway)
# Verify proper error handling
with pytest.raises(PaymentProcessingError) as exc:
payment_service.charge(amount=100, card="1234")
assert "Timeout" in str(exc.value)
// Using Jest
test('should send welcome email on user creation', async () => {
const mockEmailService = {
sendWelcome: jest.fn().mockResolvedValue(true)
};
const userService = new UserService(mockEmailService);
const user = await userService.createUser(
'test@example.com',
'password'
);
expect(mockEmailService.sendWelcome).toHaveBeenCalledWith(
'test@example.com',
expect.objectContaining({ userName: user.name })
);
});
test('should handle payment gateway timeout', async () => {
const mockGateway = {
process: jest.fn().mockRejectedValue(
new Error('Gateway timeout')
)
};
const paymentService = new PaymentService(mockGateway);
await expect(
paymentService.charge({ amount: 100, card: '1234' })
).rejects.toThrow('Gateway timeout');
});
// Using Mockito
@Test
public void shouldSendWelcomeEmailOnUserCreation() {
EmailService mockEmail = mock(EmailService.class);
when(mockEmail.sendWelcome(anyString())).thenReturn(true);
UserService userService = new UserService(mockEmail);
User user = userService.createUser("test@example.com", "password");
verify(mockEmail).sendWelcome(
eq("test@example.com"),
eq(user.getName())
);
}
@Test(expected = PaymentProcessingException.class)
public void shouldHandlePaymentGatewayTimeout() {
PaymentGateway mockGateway = mock(PaymentGateway.class);
when(mockGateway.process(any())).thenThrow(
new TimeoutException("Gateway timeout")
);
PaymentService paymentService = new PaymentService(mockGateway);
paymentService.charge(100, "1234");
}
Common Mocking Pitfalls
- Over-mocking everything reduces test value
- Mocking implementation details creates brittle tests
- Mocks that don't reflect real behavior mask bugs
- Excessive setup complexity indicates poor design
Integration and End-to-End Testing¶
Test Suite Organization¶
Hierarchical Structure:
tests/
├── unit/ # Fast, isolated tests
│ ├── services/
│ ├── models/
│ └── utils/
├── integration/ # Tests with real dependencies
│ ├── auth/
│ │ ├── login/
│ │ ├── registration/
│ │ └── password_reset/
│ ├── payments/
│ │ ├── processing/
│ │ └── refunds/
│ └── api/
├── e2e/ # Complete user workflows
│ ├── workflows/
│ └── scenarios/
└── fixtures/ # Shared test data
Testing with Realistic Environments¶
Environment Setup:
# docker-compose.test.yml
version: '3.8'
services:
app:
build:
context: .
dockerfile: Dockerfile.test
environment:
- NODE_ENV=test
- DB_HOST=test-db
- REDIS_HOST=test-redis
depends_on:
test-db:
condition: service_healthy
test-redis:
condition: service_healthy
test-db:
image: postgres:14
environment:
- POSTGRES_DB=testdb
- POSTGRES_USER=test
- POSTGRES_PASSWORD=testpass
healthcheck:
test: ["CMD-SHELL", "pg_isready -U test"]
interval: 5s
timeout: 5s
retries: 5
tmpfs:
- /var/lib/postgresql/data
test-redis:
image: redis:7-alpine
healthcheck:
test: ["CMD", "redis-cli", "ping"]
interval: 5s
timeout: 5s
retries: 5
# tests/fixtures/data_manager.py
class TestDataManager:
def __init__(self, database):
self.db = database
self.created_records = []
def seed_base_data(self):
"""Load foundational test data"""
self.db.load_fixtures('base_data.json')
def create_test_user(self, **kwargs):
"""Create a test user with sensible defaults"""
defaults = {
'email': f'test-{uuid4()}@example.com',
'username': f'testuser_{uuid4().hex[:8]}',
'password': 'Test123!@#',
'is_active': True
}
user_data = {**defaults, **kwargs}
user = self.db.users.create(**user_data)
self.created_records.append(('users', user.id))
return user
def create_test_order(self, user_id=None, **kwargs):
"""Create a test order"""
if user_id is None:
user = self.create_test_user()
user_id = user.id
defaults = {
'user_id': user_id,
'status': 'pending',
'total_amount': 100.00
}
order_data = {**defaults, **kwargs}
order = self.db.orders.create(**order_data)
self.created_records.append(('orders', order.id))
return order
def cleanup(self):
"""Clean up all created test data"""
for table, record_id in reversed(self.created_records):
self.db[table].delete(record_id)
self.created_records.clear()
Handling External Dependencies¶
Service Virtualization Approach:
| Dependency | Strategy | Implementation |
|---|---|---|
| Payment Gateways | Mock responses | Custom mock class |
| Email Services | Stub responses | Test-specific implementation |
| Third-party APIs | Record/replay | VCR.py, nock |
| Databases | Containerized | Docker |
| Message Queues | In-memory | Fake implementation |
Mock Payment Gateway Example:
# tests/mocks/payment_gateway.py
class MockPaymentGateway:
"""Simulates payment gateway for testing"""
def __init__(self):
self.transactions = []
self.scenarios = {
'success': {'status': 'approved', 'transaction_id': 'mock-123'},
'insufficient_funds': {'status': 'declined', 'error': 'INS_FUND_001'},
'card_declined': {'status': 'declined', 'error': 'CARD_DEC_001'},
'service_error': {'status': 'error', 'error': 'SVC_ERR_001'}
}
def process_payment(self, amount, card):
"""Process payment with configurable scenarios"""
if amount > 10000:
response = self.scenarios['service_error']
elif not self._is_valid_card(card):
response = self.scenarios['card_declined']
elif card.get('balance', float('inf')) < amount:
response = self.scenarios['insufficient_funds']
else:
response = self.scenarios['success']
self.transactions.append({
'amount': amount,
'card': card,
'response': response,
'timestamp': datetime.now()
})
return response
def _is_valid_card(self, card):
required_fields = ['number', 'expiry', 'cvv']
return all(field in card for field in required_fields)
def get_transaction_history(self):
return self.transactions
def reset(self):
self.transactions.clear()
Retry Pattern:
from functools import wraps
import time
def retry(max_attempts=3, delay=1, backoff=2, exceptions=(Exception,)):
"""Retry decorator with exponential backoff"""
def decorator(func):
@wraps(func)
def wrapper(*args, **kwargs):
attempt = 0
current_delay = delay
while attempt < max_attempts:
try:
return func(*args, **kwargs)
except exceptions as e:
attempt += 1
if attempt == max_attempts:
raise
time.sleep(current_delay)
current_delay *= backoff
return None
return wrapper
return decorator
# Usage
@retry(max_attempts=3, delay=1, exceptions=(ConnectionError, TimeoutError))
def test_external_service():
service = ExternalService()
response = service.make_request()
assert response.status_code == 200
Testing External Dependencies
- Use containerized versions of external services
- Mock services should match real API contracts
- Record and replay real responses (VCR pattern)
- Test error scenarios: timeouts, rate limits, service unavailable
- Document all external service requirements
Test Coverage Guidelines¶
Coverage Standards¶
Minimum Coverage by Component:
| Component Type | Minimum | Target |
|---|---|---|
| Critical Business Logic | 95% | 100% |
| API Endpoints | 90% | 95% |
| Service Layer | 85% | 90% |
| UI Components | 80% | 90% |
| Utility Functions | 70% | 85% |
| Configuration | 60% | 75% |
Require 100% Coverage For:
- Financial calculations and transactions
- Authentication and authorization logic
- Data validation and sanitization
- Payment processing
- User data handling
- Security-critical functions
- Regulatory compliance code
Coverage Configuration¶
{
"jest": {
"collectCoverageFrom": [
"src/**/*.{js,jsx,ts,tsx}",
"!src/**/*.d.ts",
"!src/**/*.test.{js,jsx,ts,tsx}"
],
"coverageThreshold": {
"global": {
"branches": 80,
"functions": 80,
"lines": 80,
"statements": 80
},
"./src/critical/": {
"branches": 100,
"functions": 100,
"lines": 100,
"statements": 100
}
},
"coverageReporters": ["text", "html", "lcov"]
}
}
<!-- pom.xml -->
<plugin>
<groupId>org.jacoco</groupId>
<artifactId>jacoco-maven-plugin</artifactId>
<version>0.8.8</version>
<executions>
<execution>
<goals>
<goal>prepare-agent</goal>
</goals>
</execution>
<execution>
<id>report</id>
<phase>test</phase>
<goals>
<goal>report</goal>
</goals>
</execution>
</executions>
</plugin>
Coverage Reporting in CI/CD¶
name: Test Coverage
on:
push:
branches: [ main, develop ]
pull_request:
branches: [ main, develop ]
jobs:
coverage:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
- name: Setup Node.js
uses: actions/setup-node@v3
with:
node-version: '18'
cache: 'npm'
- name: Install dependencies
run: npm ci
- name: Run tests with coverage
run: npm run test:coverage
- name: Upload to Codecov
uses: codecov/codecov-action@v3
with:
files: ./coverage/lcov.info
fail_ci_if_error: true
- name: Archive coverage reports
uses: actions/upload-artifact@v3
with:
name: coverage-report
path: coverage/
retention-days: 30
Coverage Best Practices
- Focus on meaningful coverage, not just percentages
- Test behavior, not implementation details
- Identify and prioritize critical paths
- Review coverage reports regularly
- Track coverage trends over time
Code Review Process¶
Pull Request Template¶
## Description
What changed and why?
---
## Type of Change
- [ ] Bug fix
- [ ] New feature
- [ ] Breaking change
- [ ] Documentation update
- [ ] Configuration change
- [ ] Code refactoring
---
## Testing
- [ ] Unit tests added/updated
- [ ] Integration tests added/updated
- [ ] All tests passing
- [ ] Manual testing completed
---
## Code Quality Checklist
- [ ] Code follows style guidelines
- [ ] Self-review completed
- [ ] Comments added for complex logic
- [ ] No debugging code left in
- [ ] No sensitive data exposed
---
## Security Considerations
- [ ] Input validation implemented
- [ ] Authentication/authorization checked
- [ ] Security implications considered
- [ ] No hardcoded credentials
- [ ] Sensitive data handling reviewed
---
## Performance Impact
- [ ] Performance implications assessed
- [ ] Database queries optimized
- [ ] No obvious performance issues
---
## Documentation
- [ ] README updated (if needed)
- [ ] API documentation updated (if needed)
- [ ] Code comments added
- [ ] CHANGELOG updated
---
## Related Issues
Closes #123, Relates to #456
---
## Additional Notes
Any other information for reviewers?
Review Classifications¶
Mandatory Reviews (2+ Approvals Required):
Critical Changes
- Financial and payment processing logic
- Authentication and authorization changes
- Database schema modifications
- API contract changes
- Security-related code
- Infrastructure configuration
- Dependency version updates
Standard Reviews (1 Approval Required):
Normal Changes
- Business logic modifications
- New features
- Bug fixes
- Performance improvements
- Code refactoring
Optional Reviews:
- Documentation-only updates
- Test additions without code changes
- Minor UI copy changes
- Development tooling updates
Review Checklists¶
Architecture and Design:
- [ ] Appropriate design patterns used (not over-engineered)
- [ ] SOLID principles followed
- [ ] Clear separation of concerns
- [ ] Minimal coupling between components
- [ ] Design scales with future requirements
- [ ] Code is maintainable and readable
Code Quality:
- [ ] Logical organization and structure
- [ ] Appropriate modularization
- [ ] No code duplication
- [ ] Naming conventions followed
- [ ] Complex logic is documented
- [ ] Functions are reasonably sized
Error Handling:
- [ ] Proper exception handling
- [ ] Meaningful error messages
- [ ] Edge cases considered
- [ ] Graceful degradation implemented
- [ ] Error recovery is appropriate
Security:
- [ ] All user inputs validated
- [ ] SQL injection prevention in place
- [ ] XSS prevention implemented
- [ ] CSRF protection configured
- [ ] Sensitive data properly handled
- [ ] Authentication/authorization verified
- [ ] No hardcoded credentials
Feedback Guidelines¶
Structure for Feedback:
- Observation - What you found
- Impact - Why it matters
- Suggestion - How to improve
Example Feedback:
## Suggestion: Improve Error Handling
The current code catches all exceptions generically:
try:
result = process_payment(order)
except Exception as e:
return error_response()
This makes debugging difficult and masks the actual error from monitoring.
Consider handling specific exception types:
try:
result = process_payment(order)
except PaymentGatewayError as e:
logger.error(f"Payment gateway error: {e}")
return error_response("Payment processing failed", status=503)
except InsufficientFundsError as e:
logger.warning(f"Insufficient funds for order {order.id}")
return error_response("Insufficient funds", status=402)
except Exception as e:
logger.exception(f"Unexpected error: {e}")
return error_response("Internal error", status=500)
Benefits: Better error visibility, appropriate HTTP status codes, easier debugging.
Code Review Best Practices
- Read the code thoroughly, don't skim
- Ask questions if something is unclear
- Acknowledge good work, not just problems
- Be respectful and professional
- Focus on the code, not the person
- Block PRs only for serious issues
Continuous Integration and Deployment¶
CI/CD Pipeline Configuration¶
GitHub Actions Example:
name: CI/CD Pipeline
on:
push:
branches: [ main, develop ]
pull_request:
branches: [ main, develop ]
jobs:
test:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
- name: Setup Node.js
uses: actions/setup-node@v3
with:
node-version: '18'
cache: 'npm'
- name: Install dependencies
run: npm ci
- name: Run linting
run: npm run lint
- name: Run type checking
run: npm run type-check
- name: Run unit tests
run: npm run test:unit
- name: Run integration tests
run: npm run test:integration
- name: Run coverage
run: npm run test:coverage
- name: Security scanning
uses: snyk/actions/node@master
env:
SNYK_TOKEN: ${{ secrets.SNYK_TOKEN }}
deploy-staging:
needs: test
if: github.ref == 'refs/heads/main'
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
- name: Deploy to staging
run: ./scripts/deploy.sh staging
deploy-production:
needs: deploy-staging
if: github.ref == 'refs/heads/main'
runs-on: ubuntu-latest
environment: production
steps:
- uses: actions/checkout@v3
- name: Deploy to production
run: ./scripts/deploy.sh production
Jenkins Pipeline Example:
pipeline {
agent any
environment {
NODE_ENV = 'test'
}
stages {
stage('Build') {
steps {
sh 'npm ci'
sh 'npm run build'
}
}
stage('Test') {
parallel {
stage('Lint') {
steps {
sh 'npm run lint'
}
}
stage('Unit Tests') {
steps {
sh 'npm run test:unit'
}
}
stage('Integration Tests') {
steps {
sh 'npm run test:integration'
}
}
}
}
stage('Security Scan') {
steps {
sh 'npm audit'
sh 'snyk test'
}
}
stage('Deploy Staging') {
when { branch 'main' }
steps {
sh './scripts/deploy.sh staging'
sh 'npm run test:smoke'
}
}
stage('Deploy Production') {
when { branch 'main' }
input {
message "Deploy to production?"
ok "Yes"
}
steps {
sh './scripts/deploy.sh production'
}
}
}
post {
always {
junit '**/test-results/*.xml'
publishHTML(target: [
reportDir: 'coverage',
reportFiles: 'index.html',
reportName: 'Coverage Report'
])
}
failure {
sh './scripts/rollback.sh'
}
}
}
Deployment Strategies¶
Blue-Green Deployment¶
Maintains two identical production environments for zero-downtime deployments and quick rollbacks.
apiVersion: apps/v1
kind: Deployment
metadata:
name: app-blue
spec:
replicas: 3
selector:
matchLabels:
app: myapp
version: blue
template:
metadata:
labels:
app: myapp
version: blue
spec:
containers:
- name: myapp
image: myapp:1.0
---
apiVersion: apps/v1
kind: Deployment
metadata:
name: app-green
spec:
replicas: 3
selector:
matchLabels:
app: myapp
version: green
template:
metadata:
labels:
app: myapp
version: green
spec:
containers:
- name: myapp
image: myapp:2.0
Canary Deployment¶
Gradually rolls out changes to a small subset of users before full deployment.
def deploy_canary(new_version, initial_percent=5):
"""Canary deployment with monitoring"""
try:
deploy_version(new_version, percent=initial_percent)
thresholds = {
'error_rate': 1.0,
'latency_p95': 500,
'cpu_usage': 85,
'memory_usage': 90
}
stages = [25, 50, 75, 100]
for target_percent in stages:
if monitor_deployment(thresholds):
deploy_version(new_version, percent=target_percent)
else:
rollback_deployment()
raise Exception("Deployment metrics exceeded thresholds")
return True
except Exception as e:
logging.error(f"Canary deployment failed: {str(e)}")
rollback_deployment()
return False
Rollback and Disaster Recovery¶
Automated Rollback Triggers:
| Trigger | Threshold | Action |
|---|---|---|
| Error rate | Exceeds 1% | Automatic rollback |
| Response time | Exceeds 500ms | Automatic rollback |
| CPU usage | Exceeds 85% | Alert and manual review |
| Memory usage | Exceeds 90% | Automatic rollback |
Rollback Script:
#!/bin/bash
PREVIOUS_VERSION=$(cat ./previous_version)
DEPLOYMENT_NAME="myapp"
NAMESPACE="production"
MAX_RETRIES=3
check_health() {
local endpoint=$1
local retry_count=0
while [ $retry_count -lt $MAX_RETRIES ]; do
if curl -f "http://${endpoint}/health" > /dev/null 2>&1; then
return 0
fi
retry_count=$((retry_count + 1))
sleep 5
done
return 1
}
perform_rollback() {
echo "Starting rollback to version ${PREVIOUS_VERSION}"
incident_start=$(date +%s)
kubectl rollout undo deployment/${DEPLOYMENT_NAME} -n ${NAMESPACE}
if ! kubectl rollout status deployment/${DEPLOYMENT_NAME} -n ${NAMESPACE} --timeout=300s; then
echo "Rollback failed to complete"
exit 1
fi
if ! check_health "${DEPLOYMENT_NAME}.${NAMESPACE}"; then
echo "Health check failed after rollback"
exit 1
fi
incident_end=$(date +%s)
recovery_time=$((incident_end - incident_start))
echo "Rollback completed in ${recovery_time} seconds"
update_monitoring_system
generate_incident_report ${incident_start} ${incident_end}
}
perform_rollback
Disaster Recovery Procedures:
Critical Recovery Elements
- Automated health checks on critical services
- Automated alerts for system anomalies
- Regular database backup verification
- Define and document Recovery Time Objectives (RTO)
- Implement automated recovery procedures
- Regular disaster recovery drills
- Regular database backups with transaction logs
- Point-in-time recovery capabilities
- Detailed incident response procedures
- System dependencies and recovery order documentation
Deployment Checklist:
- All tests passing
- Code review completed and approved
- Security scan completed
- Database migrations reviewed (if applicable)
- Rollback plan documented
- On-call support scheduled
- Monitoring and alerting configured
- Backup verification completed
Last updated: October 2025