feat(test): add Playwright E2E test framework

[devondragon]

Summary

This PR adds a comprehensive Playwright-based E2E test suite with Page Object Model architecture for testing user flows in the Spring Demo Application.

• 81 tests covering all major user workflows
• Page Object Model architecture for maintainability
• Test API endpoints for backend data manipulation
• Automatic test cleanup and isolation

Test Coverage

Category	Tests	Description
Registration	9	User signup, validation, terms acceptance
Login	8	Authentication, error handling, navigation
Email Verification	6	Token verification, edge cases
Password Reset	7	Request, complete, invalid tokens
Profile Updates	9	Name changes, validation
Password Change	7	Current password verification, validation
Account Deletion	7	Confirmation, post-deletion behavior
Access Control	14	Protected pages, public pages, session management
Events	10	Browse, register, unregister, my-events
E2E Journeys	4	Complete user lifecycle, password reset flow

Framework Features

• Page Object Model: BasePage, LoginPage, RegisterPage, DeleteAccountPage, etc.
• Test Fixtures: Common setup/teardown, user generation, cleanup
• Test API Client: Backend data manipulation for test scenarios
• Playwright Config: Auto-start server, parallel execution, retries

New Files

Playwright Framework (playwright/)

• playwright.config.ts - Configuration with webServer auto-start
• src/pages/*.ts - Page Object classes
• src/fixtures/ - Test fixtures and helpers
• src/utils/test-api-client.ts - API client for test data
• tests/**/*.spec.ts - Test specifications

Backend Support

• AdminController.java - Admin page with @PreAuthorize for access control tests
• TestDataController.java - Test API endpoints (enabled only in test profile)
• TestApiSecurityConfig.java - Security config for test API
• application-playwright-test.yml - Test profile configuration

Test Plan

• All 81 Playwright tests passing locally
• CI pipeline runs tests successfully
• Manual smoke test of key user flows

Running Tests

cd playwright
npm install
npx playwright test

Or with the Gradle task:

./gradlew bootRun --args='--spring.profiles.active=playwright-test'
# In another terminal:
cd playwright && npx playwright test

[claude]

PR Review: Playwright E2E Test Framework

This is an excellent addition to the testing infrastructure! The PR adds a comprehensive Playwright-based E2E test suite with 81 tests covering all major user workflows. Here's my detailed feedback:

✅ Strengths

Architecture & Design

• Well-structured Page Object Model: Clean separation of concerns with BasePage and specialized page objects
• Smart test fixtures: The fixture system with automatic cleanup is elegant and reduces boilerplate
• Test API design: The TestDataController and TestApiClient provide a clean way to manipulate test data without going through the UI

Code Quality

• Type safety: Excellent use of TypeScript with comprehensive interfaces for API responses
• Maintainability: Page objects encapsulate selectors and interactions, making tests resilient to UI changes
• Test isolation: Each test generates unique users and cleans up after itself

Coverage

• Comprehensive: 81 tests covering registration, login, password reset, profile management, events, and access control
• Edge cases: Tests include validation failures, expired tokens, and invalid inputs

🔒 Security Concerns (CRITICAL)

1. Test API Exposure Risk (HIGH PRIORITY)

Location: TestDataController.java:42, TestApiSecurityConfig.java:22

Issue: The test API endpoints at /api/test/** have CSRF disabled and no authentication. While protected by the @Profile("playwright-test") annotation, this creates several risks:

• Profile misconfiguration: If playwright-test profile is accidentally activated in production, these endpoints would be exposed
• No authentication: Endpoints allow complete database manipulation without any credentials
• No audit trail: Operations bypass normal security and logging mechanisms

Recommendation:

// Add IP whitelist restriction
@Bean
@Order(1)
public SecurityFilterChain testApiSecurityFilterChain(HttpSecurity http) throws Exception {
    http.securityMatcher("/api/test/**")
        .authorizeHttpRequests(authorize -> authorize
            .requestMatchers(request -> {
                String remoteAddr = request.getRemoteAddr();
                return "127.0.0.1".equals(remoteAddr) || "localhost".equals(remoteAddr);
            }).permitAll()
            .anyRequest().denyAll()
        )
        .csrf(csrf -> csrf.disable());
    return http.build();
}

Additionally, consider:

• Adding a startup check that logs a warning if playwright-test profile is active
• Adding request validation to ensure test API calls include a secret token from environment variable
• Implementing rate limiting even for test endpoints

2. Dependency Version Concern

Location: build.gradle:42

Issue: The framework dependency was changed to a SNAPSHOT version:

implementation 'com.digitalsanctuary:ds-spring-user-framework:4.0.3-SNAPSHOT'

Impact: SNAPSHOT dependencies can change without notice, leading to:

• Non-reproducible builds
• Unexpected breaking changes
• Build failures if snapshot repository becomes unavailable

Recommendation: Either:

• Use a stable release version (4.0.3 when available)
• Document why the SNAPSHOT is needed and when it will be updated
• If testing unreleased framework features, add a comment explaining the dependency

3. Configuration Security

Location: application-playwright-test.yml:27

Issue: The configuration adds /api/test/** to unprotectedURIs, which is redundant with TestApiSecurityConfig but creates confusion about security boundaries.

Recommendation: Remove from unprotectedURIs since TestApiSecurityConfig handles it. Keep security configuration centralized.

🐛 Potential Bugs

1. Token Deletion Race Condition

Location: TestDataController.java:229-238

Issue: When deleting a user, tokens are deleted but there's a potential race condition if tokens are being accessed during deletion:

VerificationToken verificationToken = verificationTokenRepository.findByUser(user);
if (verificationToken != null) {
    verificationTokenRepository.delete(verificationToken);
}

Recommendation: Consider using cascade delete on the User entity, or add proper locking:

@Transactional
public ResponseEntity<Map<String, Object>> deleteTestUser(@RequestParam String email) {
    User user = userRepository.findByEmail(email);
    if (user == null) {
        // handle not found
    }
    
    // Use deleteAll with query to handle multiple tokens if any
    verificationTokenRepository.deleteByUser(user);
    passwordResetTokenRepository.deleteByUser(user);
    userRepository.delete(user);
    // ...
}

2. Hardcoded Timeout in BasePage

Location: BasePage.ts:88

Issue: await this.page.waitForTimeout(200); uses a fixed delay instead of waiting for actual conditions.

Recommendation:

async logout(): Promise<void> {
    const accountDropdown = this.page.locator('#accountDropdown');
    if (await accountDropdown.count() > 0) {
        await accountDropdown.first().click();
        // Wait for dropdown menu to be visible instead of arbitrary timeout
        await this.page.locator('.dropdown-menu').waitFor({ state: 'visible', timeout: 2000 });
    }
    // ...
}

3. Missing Error Handling in Test API Client

Location: test-api-client.ts:135-141

Issue: API calls don't check response status codes. Failed requests return error JSON but tests may not detect them:

async userExists(email: string): Promise<UserExistsResponse> {
    const response = await context.get(`/api/test/user/exists`, {
        params: { email },
    });
    return response.json(); // What if response.status is 500?
}

Recommendation:

async userExists(email: string): Promise<UserExistsResponse> {
    const response = await context.get(`/api/test/user/exists`, {
        params: { email },
    });
    if (!response.ok()) {
        throw new Error(`Test API request failed: ${response.status()} ${response.statusText()}`);
    }
    return response.json();
}

⚡ Performance Considerations

1. Parallel Test Execution

Location: playwright.config.ts:33

Issue: On CI, workers are set to 1, which will make test runs very slow:

workers: process.env.CI ? 1 : undefined,

Recommendation: Consider using a higher number even on CI (e.g., 2-4) since tests appear to be properly isolated with unique emails. If there are database concurrency concerns, that's a separate issue to address.

2. WebServer Reuse

Location: playwright.config.ts:120

Good practice: reuseExistingServer: !process.env.CI prevents unnecessary server restarts during development. Consider adding a health check before reusing:

webServer: {
    // ...
    reuseExistingServer: !process.env.CI,
    // Add this:
    healthCheckUrl: 'http://localhost:8080/api/test/health',
}

3. Database Connection Pooling

Location: application-playwright-test.yml:13-16

Recommendation: Add connection pool settings for better test performance:

spring:
    datasource:
        hikari:
            maximum-pool-size: 5
            minimum-idle: 2

📝 Code Quality Suggestions

1. Magic Strings

Location: Multiple test files

Issue: URLs and selectors are sometimes hardcoded in tests instead of using page objects.

Example from registration.spec.ts:23:

expect(page.url()).toContain('registration-complete');

Recommendation: Add constants or methods to page objects:

// In RegisterPage
readonly REGISTRATION_COMPLETE_PATH = '/user/registration-complete';

async isOnRegistrationComplete(): Promise<boolean> {
    return this.page.url().includes(this.REGISTRATION_COMPLETE_PATH);
}

2. Test Data Builders

Issue: Test user generation is basic and doesn't support custom attributes easily.

Recommendation: Implement a builder pattern:

class TestUserBuilder {
    private user: Partial<TestUser> = {
        password: generateTestPassword(),
        firstName: 'Test',
        lastName: 'User',
    };
    
    withEmail(prefix: string): this {
        this.user.email = generateTestEmail(prefix);
        return this;
    }
    
    withName(firstName: string, lastName: string): this {
        this.user.firstName = firstName;
        this.user.lastName = lastName;
        return this;
    }
    
    build(): TestUser {
        if (!this.user.email) {
            this.user.email = generateTestEmail('default');
        }
        return this.user as TestUser;
    }
}

3. Add JSDoc Comments

Location: Page object methods

Many methods lack documentation. Example:

/**
 * Register a new user and wait for the registration to complete.
 * @param firstName - User's first name
 * @param lastName - User's last name  
 * @param email - User's email address
 * @param password - User's password
 * @throws {Error} If registration fails or times out
 */
async registerAndWait(/* ... */): Promise<void> {
    // ...
}

🔍 Test Coverage Gaps

While the 81 tests are comprehensive, consider adding:

1. Concurrency tests: Multiple users registering simultaneously with the same email
2. Session management: Test session timeout, concurrent logins from different devices
3. XSS/injection tests: Verify that user inputs are properly sanitized in display
4. File upload tests: If the app has file upload features
5. Accessibility tests: Add axe-core integration for a11y testing
6. Performance tests: Add basic performance assertions (e.g., page load < 2s)

📚 Documentation

Missing Documentation

1. README: Add playwright/README.md with:

   • Prerequisites (Node.js version, database setup)
   • Environment variable configuration
   • Common troubleshooting scenarios
   • How to run specific test suites

2. CI/CD Integration: Document how to integrate these tests into GitHub Actions or other CI pipelines

3. Debugging Guide: Add examples of debugging failing tests with Playwright Inspector

✨ Minor Issues

1. Line 118 in playwright.config.ts: Profile should be just playwright-test not local,playwright-test to avoid unintended config mixing:

command: 'cd .. && ./gradlew bootRun --args="--spring.profiles.active=playwright-test"',

2. CLAUDE.md update: Consider updating CLAUDE.md to document the Playwright test structure and conventions

3. Build.gradle tasks: The Gradle tasks are well done, but consider adding:

tasks.register('playwrightTestHeaded', Exec) {
    description = 'Run Playwright tests in headed mode for debugging'
    workingDir 'playwright'
    commandLine 'npx', 'playwright', 'test', '--headed'
    dependsOn 'playwrightBrowsers'
}

🎯 Recommendations Summary

Must Fix (Before Merge)

1. ✅ Add IP whitelist or token authentication to Test API endpoints
2. ✅ Change framework dependency from SNAPSHOT to stable version or document why needed
3. ✅ Fix playwright.config.ts profile to use only playwright-test

Should Fix (High Priority)

1. Add error handling to Test API Client methods
2. Replace waitForTimeout with condition-based waits
3. Add health check to webServer config
4. Fix token deletion to use proper cascade or repository methods

Nice to Have

1. Add Playwright README with setup and debugging instructions
2. Implement test data builder pattern
3. Add JSDoc comments to page objects
4. Update CLAUDE.md with Playwright conventions
5. Consider increasing CI workers for faster test runs
6. Add accessibility testing with axe-core

🎉 Conclusion

This is high-quality work that significantly improves the project's test coverage. The Page Object Model architecture is solid, the test API is a clever solution for test data management, and the coverage is comprehensive. With the security concerns addressed (particularly around the test API), this will be a valuable addition to the codebase.

The main concern is ensuring the test API can never be accidentally exposed in production. Once that's addressed, this is ready to merge.

Approval Status: ⚠️ Approve with Changes (address security concerns first)

Great work on this comprehensive test suite! 🚀

In general, the problem is that CSRF protection is completely disabled for the /api/test/** endpoints. The fix is to stop calling csrf.disable() and instead leave CSRF enabled, optionally configuring it to ignore specific non-browser-safe endpoints when necessary. For test-only APIs that might be hit by browser-based tools (like Playwright, which does behave like a browser), the safest approach is to keep CSRF protection on and have the tests supply CSRF tokens, or to limit CSRF ignoring to clearly non-mutating or non-browser-facing endpoints.

For this specific file, the minimal change that removes the vulnerability while preserving overall behavior is:

• Remove the .csrf(csrf -> csrf.disable()) call from the HttpSecurity configuration.
• Do not add any custom ignoring, since this is a small test API and the comment already indicates security sensitivity. Leaving CSRF enabled means:
  • Any state-changing endpoints under /api/test/** will now require CSRF tokens when invoked from a browser.
  • Non-browser clients can still call them if they send the appropriate CSRF header/cookie when needed.

Concretely:

• In src/main/java/com/digitalsanctuary/spring/demo/test/config/TestApiSecurityConfig.java, within testApiSecurityFilterChain, delete the .csrf(csrf -> csrf.disable()); call and terminate the previous configuration chain with a semicolon. No new imports or methods are required.

---

[claude]

Pull Request Review: Playwright E2E Test Framework

Thank you for this comprehensive addition! This PR adds a robust Playwright-based E2E testing framework with 81 tests. Here's my detailed review:

Strengths

Architecture & Design

• Excellent Page Object Model implementation with clean separation of concerns
• Well-organized test structure grouped by feature areas
• Smart test fixtures with reusable setup/teardown and automatic cleanup
• Pragmatic Test API design for E2E testing without email dependency

Code Quality

• Full TypeScript implementation with proper types
• Test isolation with unique user data per test
• Comprehensive coverage: 81 tests across registration, login, password flows, profile updates, events, and access control

Security

• Test API properly restricted to playwright-test profile only (TestDataController.java:42, TestApiSecurityConfig.java:22)
• CSRF disabled only for /api/test/** endpoints in test profile
• Clear warnings against production use

Critical Issues

1. SECURITY: Production Deployment Protection

Location: TestDataController.java, TestApiSecurityConfig.java, application-playwright-test.yml

Issue: While @Profile annotations prevent the test API from loading in production, there's no failsafe if the profile is accidentally enabled.

Recommendation: Add an application startup check to abort if test profile is active in production. Create a TestProfileGuard component that checks environment variables and fails startup if ENVIRONMENT=production.

2. Security: Overly Broad URI Exceptions

Location: application-playwright-test.yml:27

Issue: unprotectedURIs adds /api/test/** to the global unprotected list, bypassing security at multiple layers.

Recommendation: Remove /api/test/** from YAML config - TestApiSecurityConfig already handles this with securityMatcher, which is more explicit and maintainable.

3. Missing Error Handling in Test API Client

Location: test-api-client.ts - all methods

Issue: API calls don't check response status codes before parsing JSON.

Recommendation: Add response.ok() validation and throw descriptive errors for non-200 responses.

4. Parallel Execution Concerns

Location: playwright.config.ts:33

Issue: Tests run in parallel locally but share the same database, potentially causing race conditions.

Current mitigation: Unique email generation provides some isolation.

Recommendation: Document limitations or consider database transaction rollback support.

Minor Issues

1. Logging sensitivity (TestDataController.java:218, 254): Use DEBUG instead of INFO level for user emails
2. Magic numbers (playwright.config.ts:80, 84): Extract timeout values to named constants
3. Test cleanup: Add manual cleanup script for crashed test runs
4. Mobile testing: Document which tests are mobile-compatible
5. Test coverage: Consider adding tests for session timeout, rate limiting, lockout, OAuth flows

Documentation Suggestions

1. Add playwright/README.md with setup, troubleshooting, and architecture
2. Document test API endpoints
3. Add E2E test section to main README

Recommendations Summary

Must Fix Before Merge:

1. Add production environment guard for test profile
2. Add response status validation in TestApiClient

Should Fix:

3. Remove /api/test/** from YAML unprotectedURIs
4. Add Playwright README with setup guide
5. Document parallel execution limitations

Nice to Have:

6. Extract timeout constants
7. Add lockout/rate limiting tests
8. Change logging to DEBUG level

Final Verdict

This is high-quality work that significantly improves test coverage. The Page Object Model architecture is well-designed and maintainable.

Primary concern: Security configuration needs safeguards against accidental production deployment.

Recommendation: Approve with minor changes requested

Great job on this comprehensive testing framework!

[Copilot]

Pull request overview

This PR adds a comprehensive Playwright-based E2E test framework to the Spring Demo Application, providing 81 tests covering major user workflows including registration, login, email verification, password reset, profile management, account deletion, access control, and event management.

Changes:

• Added Playwright test framework with Page Object Model architecture, test fixtures, and a Test API client for database manipulation
• Created backend Test API controller and security configuration (active only in playwright-test profile) to support test scenarios
• Fixed several template issues including CSRF meta tag null checks, login error message display, and script tag path resolution
• Updated build configuration to include Gradle tasks for running Playwright tests

Reviewed changes

Copilot reviewed 39 out of 42 changed files in this pull request and generated 31 comments.

Show a summary per file

File	Description
src/main/java/com/digitalsanctuary/spring/demo/test/config/TestApiSecurityConfig.java	Security configuration that disables CSRF and authentication for /api/test/** endpoints when playwright-test profile is active
src/main/java/com/digitalsanctuary/spring/demo/test/api/TestDataController.java	REST controller providing test data manipulation endpoints for E2E tests
src/main/java/com/digitalsanctuary/spring/demo/controller/AdminController.java	Admin controller with role-based access control for testing authorization
src/main/resources/application-playwright-test.yml	Test profile configuration disabling emails and enabling test API endpoints
src/main/resources/application.yml	Updated unprotected URIs to include /js/utils/* path
src/main/resources/templates/user/login.html	Added error message display for form login failures
src/main/resources/templates/user/forgot-password-change.html	Fixed script tag to use Thymeleaf URL expression
src/main/resources/templates/layout.html	Added null checks for CSRF meta tags to prevent errors when CSRF is disabled
playwright/playwright.config.ts	Playwright configuration with webServer auto-start and multi-browser testing
playwright/tsconfig.json	TypeScript configuration with path aliases for cleaner imports
playwright/package.json	Node.js dependencies including Playwright 1.40+
playwright/src/pages/*.ts	Page Object Model classes for all application pages
playwright/src/fixtures/test-fixtures.ts	Test fixtures for common setup/teardown and user generation
playwright/src/utils/test-api-client.ts	Client for interacting with Test API endpoints
playwright/tests/**/*.spec.ts	81 test specifications organized by feature area
build.gradle	Added Gradle tasks for Playwright installation and test execution
.gitignore	Added Playwright-specific ignore patterns

Files not reviewed (1)

• playwright/package-lock.json: Language not supported

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

If the userRole is null (ROLE_USER doesn't exist in the database), the user will be created without any roles. This could lead to unexpected test failures or authentication issues. Consider either throwing an exception when the role is not found, or logging a warning at minimum, to make these issues more visible during test execution.

[Copilot]

The selector 'visible=true' is not a valid Playwright locator filter. Playwright uses the 'visible' state option in waitFor() or isVisible() methods, but not as a chained locator filter. This will likely cause runtime errors. Instead, use the filter method with hasText or simply call first() on visible elements, or use locator.filter() with proper options.

Suggested change

	this.changePasswordLink = page.getByRole('link', { name: 'Change Password' }).locator('visible=true').first();
	this.deleteAccountLink = page.getByRole('link', { name: 'Delete Account' }).locator('visible=true').first();
	this.changePasswordLink = page.getByRole('link', { name: 'Change Password' }).first();
	this.deleteAccountLink = page.getByRole('link', { name: 'Delete Account' }).first();

[Copilot]

The clear() method is not a standard Playwright Locator method. Use fill('') to clear the field before filling, or simply call fill() directly which will clear and then fill the input. Calling clear() will cause a runtime error.

Suggested change

	await this.firstNameInput.clear();
	await this.firstNameInput.fill(firstName);
	await this.lastNameInput.clear();
	await this.firstNameInput.fill(firstName);

[Copilot]

The user created in this test is not added to cleanupEmails, but if the test fails before the deletion actually happens (e.g., the deleteAccountAndWait() fails), the user will remain in the database and could cause issues with subsequent test runs. Consider adding the user to cleanupEmails before the test runs, and let the cleanup fixture handle removal even if the deletion was successful (the cleanup will just be a no-op in that case).

[Copilot]

The user created for this test is also not added to cleanupEmails. If the test fails before account deletion, this user will be left in the database. The same issue exists in the 'should not allow login after account deletion' test at line 39. Add these users to cleanupEmails to ensure proper cleanup on test failure.

[Copilot]

Unused variable user.

Suggested change

const user = generateTestUser('invalid-email');

[Copilot]

Unused variable strengthVisible.

Suggested change

	const strengthVisible = await registerPage.isPasswordStrengthVisible();
	const strengthVisible = await registerPage.isPasswordStrengthVisible();
	expect(strengthVisible).toBe(true);

[Copilot]

Unused import generateTestEmail.

Suggested change

	import { test, expect, generateTestUser, generateTestEmail } from '../../src/fixtures';
	import { test, expect, generateTestUser } from '../../src/fixtures';

[Copilot]

Unused import loginUser.

[Copilot]

Unused variable createdUser.

[claude]

Pull Request Review: Playwright E2E Test Framework

Overview

This PR adds a comprehensive Playwright E2E test suite with 81 tests covering all major user workflows. The implementation demonstrates strong architectural principles with Page Object Model, test fixtures, and a dedicated test API. This is a substantial and well-structured addition to the project.

✅ Strengths

Architecture & Design

• Excellent Page Object Model implementation: Clean separation of concerns with BasePage providing common functionality and specific page classes extending it
• Well-designed test fixtures: The fixture pattern in test-fixtures.ts provides reusable test setup with automatic cleanup
• Smart Test API approach: The TestDataController elegantly solves the email verification problem by allowing direct database manipulation during tests
• Strong type safety: TypeScript interfaces for all API responses ensure type-safe test code

Code Quality

• Comprehensive test coverage: 81 tests across 10 categories covering registration, login, password reset, profile updates, events, and access control
• Proper cleanup handling: The cleanupEmails fixture and cleanupUser methods ensure tests don't leave orphaned data
• Good error handling: Test API client includes proper error checking with informative error messages
• Clear documentation: Good inline comments and JSDoc throughout

Security

• Profile-based activation: @Profile("playwright-test") ensures test endpoints are never active in production
• IP whitelist restriction: TestApiSecurityConfig properly restricts test API to localhost only (127.0.0.1, ::1)
• Explicit warnings: Multiple comments warning that test configuration should never be in production

🔴 Critical Issues

1. Security: Test Profile Could Be Accidentally Enabled in Production

Location: TestDataController.java:42, TestApiSecurityConfig.java:21, application-playwright-test.yml

Issue: While @Profile("playwright-test") provides protection, there's a risk that this profile could be accidentally activated in production through environment variables or misconfiguration.

Recommendation:

• Add a startup bean that explicitly fails if playwright-test profile is active AND any production indicator is detected:

@Component
@Profile("playwright-test")
public class TestProfileGuard implements ApplicationListener<ApplicationReadyEvent> {
    @Override
    public void onApplicationEvent(ApplicationReadyEvent event) {
        String env = System.getenv("ENVIRONMENT");
        if ("production".equalsIgnoreCase(env) || "prod".equalsIgnoreCase(env)) {
            throw new IllegalStateException(
                "SECURITY: playwright-test profile MUST NOT be active in production!");
        }
    }
}

2. Hardcoded Database Credentials in Config File

Location: application-playwright-test.yml:15-18

Issue:

password: springuser
url: jdbc:mariadb://localhost:3306/springuser?createDatabaseIfNotExist=true
username: springuser

While the comment acknowledges this is for local development, hardcoded credentials in committed files are a security anti-pattern that can lead to credential reuse and scanning by automated tools.

Recommendation:

• Use environment variables even for test profile:

password: ${DB_PASSWORD:springuser}
username: ${DB_USERNAME:springuser}

3. Incomplete User Deletion in Test API

Location: TestDataController.java:232-233

Issue: The comment acknowledges event registrations aren't deleted:

// Note: Event registrations and other related entities are not deleted.
// If the user has event registrations, this may fail with foreign key constraint violation.

This will cause test failures when tests create users who register for events.

Recommendation: Properly cascade delete or manually delete related entities:

// Delete event registrations first
eventRegistrationRepository.deleteByUser(user);

// Then delete verification and password reset tokens
VerificationToken verificationToken = verificationTokenRepository.findByUser(user);
// ... rest of deletion

⚠️ Major Issues

4. Playwright Config Combines Multiple Profiles

Location: playwright.config.ts:118

command: 'cd .. && ./gradlew bootRun --args="--spring.profiles.active=local,playwright-test"'

Issue: Combining local and playwright-test profiles can lead to configuration conflicts and unexpected behavior. The playwright-test profile already includes local database configuration.

Recommendation: Remove the local profile and ensure playwright-test is self-contained:

command: 'cd .. && ./gradlew bootRun --args="--spring.profiles.active=playwright-test"'

5. Race Conditions in Parallel Test Execution

Location: playwright.config.ts:24, database shared state

Issue: Tests run in parallel (fullyParallel: true) but share the same database. Tests that create users with predictable names or query global state (like event lists) may have race conditions.

Current mitigation: Unique email generation with timestamp + random string is good, but events are not isolated.

Recommendation: Either:

• Use database transactions with rollback for each test (requires test infrastructure changes)
• Or ensure all test data is properly isolated with unique identifiers
• Document that certain test categories should not run in parallel

6. Hard Wait with waitForTimeout

Location: BasePage.ts:88

await this.page.waitForTimeout(200);

Issue: Hard-coded timeouts are brittle and can cause flakiness. Playwright discourages this pattern.

Recommendation: Use proper waiting strategies:

// Wait for dropdown menu to be visible instead
await this.page.locator('.dropdown-menu').waitFor({ state: 'visible' });

7. Missing Admin Template

Location: AdminController.java:28

The controller returns "admin/actions" but I don't see this template in the changed files. This will cause 404 errors in access control tests.

Recommendation: Ensure src/main/resources/templates/admin/actions.html exists or is added to the PR.

💡 Suggestions for Improvement

8. Framework Dependency Upgrade

Location: build.gradle:42

implementation 'com.digitalsanctuary:ds-spring-user-framework:4.0.2'
// Changed to:
implementation 'com.digitalsanctuary:ds-spring-user-framework:4.0.3'

Suggestion: Document what changes in 4.0.3 are required for this PR. If this is just an incidental upgrade, consider separating it into its own commit.

9. Test API Should Return 401/403 for Non-Localhost

Location: TestApiSecurityConfig.java:43

Currently non-localhost requests are denied with denyAll(), but the HTTP status code isn't explicit.

Suggestion: Add explicit error handling to return 403 Forbidden with a clear message:

.anyRequest().denyAll()

Consider adding an access denied handler that returns JSON with a clear message about localhost-only access.

10. Password Strength Validation Test is Weak

Location: playwright/tests/auth/registration.spec.ts:180-193

test('should show password strength indicator', async ({
  registerPage,
}) => {
  await registerPage.goto();
  await registerPage.passwordInput.fill('Test@123');
  const strengthVisible = await registerPage.isPasswordStrengthVisible();
  expect(strengthVisible).toBe(true);
});

Issue: This test only checks visibility, not the actual strength indication or whether weak passwords are blocked.

Suggestion: Add tests for different password strengths:

test('should show weak password as red', async ({ registerPage }) => {
  await registerPage.goto();
  await registerPage.passwordInput.fill('weak');
  expect(await registerPage.getPasswordStrength()).toBe('weak');
});

11. Incomplete Error Checking in Registration Test

Location: playwright/tests/auth/registration.spec.ts:97-99

const hasError = await registerPage.hasGlobalError() ||
                 await registerPage.hasExistingAccountError() ||
                 url.includes('error');

Suggestion: This is too permissive. Check for the specific error message about existing account:

expect(await registerPage.getErrorMessage()).toContain('already exists');

12. BasePage.isLoggedIn() is Fragile

Location: BasePage.ts:71-76

The method uses multiple selector strategies which makes it brittle to UI changes.

Suggestion: Use a data-testid attribute for reliable testing:

async isLoggedIn(): Promise<boolean> {
  return (await this.page.locator('[data-testid="user-menu"]').count()) > 0;
}

13. Missing Gradle Task Dependencies

Location: build.gradle:127-166

The Gradle tasks are well-structured, but playwrightTest should probably depend on bootJar or at least classes to ensure the application is built before tests run.

Suggestion:

tasks.register('playwrightTest', Exec) {
    // ... existing config
    dependsOn 'classes', 'playwrightBrowsers'
}

14. Overly Broad Unprotected URIs

Location: application-playwright-test.yml:29

The unprotected URIs list is extremely long and includes wildcards like /js/*. While necessary for functionality, this reduces defense in depth.

Suggestion: Consider using Spring Security's built-in static resource handling:

spring:
  security:
    resources:
      chain:
        enabled: true

📊 Test Coverage Analysis

Excellent coverage of user-facing workflows:

• ✅ Registration, login, logout flows
• ✅ Email verification (simulated via Test API)
• ✅ Password reset complete flow
• ✅ Profile updates and password changes
• ✅ Account deletion with confirmation
• ✅ Access control for protected pages
• ✅ Event browsing and registration

Missing coverage (consider adding):

• ❌ OAuth2/OIDC login flows (Google, Facebook)
• ❌ Session timeout behavior
• ❌ Account lockout after failed login attempts
• ❌ CSRF protection validation
• ❌ File upload scenarios (if applicable)
• ❌ API endpoint tests (currently only UI tests)

🎯 Performance Considerations

1. Startup time: Starting Spring Boot for every test run (120s timeout) will be slow. Consider:

   • Using reuseExistingServer: true for local development (already done ✅)
   • Docker-compose based testing for faster CI runs

2. Database cleanup: Individual DELETE operations in cleanup. Consider:

   • Batch deletion for faster cleanup
   • Or use test database with full wipe between runs

3. Parallel execution: Config runs tests in parallel but limits workers to 1 on CI:

   workers: process.env.CI ? 1 : undefined,

   This is conservative. Consider allowing parallel execution with proper database isolation.

🔒 Additional Security Observations

1. CSRF disabled for test API: Necessary for test API but properly scoped to /api/test/** only ✅

2. No authentication on test API: Mitigated by localhost restriction, but consider:

   • Adding a secret token in environment variables for extra protection
   • Or using HTTP Basic Auth with test credentials

3. Logging test user operations: Good use of log.info for user creation/deletion for audit trail ✅

📝 Documentation

Strengths:

• README-like documentation in PR description
• Good inline comments
• Clear JSDoc on TypeScript classes

Suggestions:

• Add a playwright/README.md with:
  • How to run tests locally
  • How to add new tests
  • How to debug failed tests
  • Explanation of test fixtures pattern
• Document the Test API endpoints (OpenAPI/Swagger would be nice but maybe overkill for test-only API)

🎉 Summary

This is a high-quality PR that adds significant value to the project. The Playwright test suite is well-architected with proper Page Object Model, fixtures, and test API support. The code demonstrates good testing practices and strong TypeScript skills.

Recommendation: ✅ Approve with required changes

Must fix before merge:

1. ❗ Fix incomplete user deletion in TestDataController (Issue Create the Event Entity and Repository and CRUD API #3)
2. ❗ Remove 'local' profile from Playwright config (Issue Create the front end for the Events #4)
3. ❗ Ensure admin/actions.html template exists (Issue Issue 2 validate readme instructions to setup a working application #7)
4. ❗ Add cascade deletion for event registrations or handle FK violations gracefully

Should fix (but not blocking):

• Replace hardcoded database credentials with environment variables
• Add production profile guard
• Replace waitForTimeout with proper waits
• Fix test data isolation concerns

Nice to have:

• Improve test assertions specificity
• Add data-testid attributes for reliable selectors
• Document Test API in separate README
• Add OAuth2 test coverage

Great work on this comprehensive test suite! The architectural decisions are sound and this will significantly improve the project's quality and confidence in changes.

[claude]

Pull Request Review: Playwright E2E Test Framework

This is an impressive and comprehensive test suite implementation! The PR adds 81 E2E tests with proper Page Object Model architecture. Here's my detailed review:

---

🎯 Strengths

Architecture & Design

• Excellent Page Object Model implementation: Clean separation of concerns with BasePage and specific page classes
• Well-structured fixtures: Smart use of Playwright fixtures for test setup, cleanup, and dependency injection
• Comprehensive test coverage: 81 tests covering all major user workflows from registration to account deletion
• Good test isolation: Proper use of unique email generation and automatic cleanup mechanisms

Code Quality

• TypeScript best practices: Proper typing throughout the codebase with well-defined interfaces
• Reusable helpers: Good abstraction with generateTestUser(), loginUser(), and registerAndVerifyUser()
• Clear documentation: Comments explain the purpose and behavior of each component

---

🔒 Security Concerns

CRITICAL: Test API Security ⚠️

While the test API is well-designed for its purpose, there are important security considerations:

1. IP-based access control may not be sufficient (TestApiSecurityConfig.java:37-42)

   • The localhost check uses request.getRemoteAddr() which can be spoofed with X-Forwarded-For headers
   • Recommendation: Add a warning in documentation that this profile should NEVER be used in production
   • Consider adding an additional check to fail fast if deployed with wrong profile

2. Profile isolation needs hardening

   @Profile("playwright-test")

   • While @Profile annotation helps, consider adding a runtime check on startup
   • Recommendation: Add an @PostConstruct method that validates the environment isn't production

3. Hardcoded credentials in config (application-playwright-test.yml:15-18)

   • The comment acknowledges this, but it's still risky
   • Recommendation: Use environment variables even for test profiles, or add validation that database is localhost

MEDIUM: Password exposure in logs

• TestDataController.java:174 logs user creation with email, which is fine
• Ensure that password hashing is always applied and raw passwords never appear in logs (currently appears safe)

---

🐛 Potential Bugs & Issues

1. Transactional Annotation May Cause Issues (TestDataController.java:218-250)

The deleteTestUser method has a comment about foreign key constraints:

// Note: Event registrations and other related entities are not deleted.
// If the user has event registrations, this may fail with foreign key constraint violation.

Issues:

• Tests that create event registrations will fail during cleanup
• This could lead to flaky tests or database pollution

Recommendation:

@DeleteMapping("/user")
@Transactional
public ResponseEntity<Map<String, Object>> deleteTestUser(@RequestParam String email) {
    // Add cascade delete for all related entities
    // Or provide a query param to force delete with cascade
}

2. Race Condition in Singleton TestApiClient (test-api-client.ts:340-348)

let clientInstance: TestApiClient | null = null;

export async function getTestApiClient(): Promise<TestApiClient> {
  if (!clientInstance) {
    clientInstance = new TestApiClient();
    await clientInstance.init();
  }
  return clientInstance;
}

Issue: In parallel test execution, multiple tests might try to create the singleton simultaneously

Recommendation: The current fixture-based approach (test-fixtures.ts:84-89) is better and should be used exclusively. Consider removing the singleton pattern to avoid confusion.

3. Hard-coded Timeout in BasePage (BasePage.ts:88)

await this.page.waitForTimeout(200);

Issue: Hard-coded timeouts are brittle and can cause flaky tests

Recommendation:

// Wait for dropdown to be visible instead
await this.page.locator('.dropdown-menu').waitFor({ state: 'visible' });

4. Gradle Task Missing Error Handling (build.gradle:132-168)

The playwrightTest task doesn't check if npm/npx are installed

Recommendation:

tasks.register('playwrightTest', Exec) {
    description = 'Run Playwright E2E tests'
    group = 'verification'
    workingDir 'playwright'
    commandLine 'npx', 'playwright', 'test'
    dependsOn 'playwrightBrowsers'
    
    // Add error handling
    doFirst {
        if (!file('playwright/node_modules').exists()) {
            throw new GradleException('Playwright dependencies not installed. Run playwrightInstall first.')
        }
    }
}

---

🚀 Performance Considerations

1. Playwright Config - Worker Settings (playwright.config.ts:33)

workers: process.env.CI ? 1 : undefined,

Good: Sequential execution in CI prevents resource conflicts
Concern: Local development might spawn too many workers

Recommendation: Set a reasonable cap for local development:

workers: process.env.CI ? 1 : Math.min(4, os.cpus().length),

2. Database Connection Reuse

The tests use a shared local database which is good, but consider:

• Adding connection pool configuration for test profile
• Documenting database cleanup procedures between test runs

3. Test API Performance

Multiple sequential API calls in fixtures could slow tests. Consider:

// Instead of multiple sequential calls:
await testApiClient.createUser(...);
await testApiClient.enableUser(...);
await testApiClient.getDetails(...);

// Consider a single endpoint:
await testApiClient.createEnabledUserWithDetails(...);

---

🧪 Test Coverage Observations

Great Coverage:

• ✅ User registration, login, verification
• ✅ Password reset flow
• ✅ Profile updates and deletion
• ✅ Access control and authorization
• ✅ Event management flows

Potential Gaps:

1. Session management edge cases

   • Concurrent login from multiple browsers
   • Session expiration during activity

2. XSS/Injection testing

   • Consider adding tests with malicious input (e.g., <script>alert('xss')</script> in names)
   • SQL injection patterns in email fields

3. Rate limiting

   • No tests for failed login attempt limits (though framework may handle this)

4. Browser compatibility

   • Tests run on 5 browsers (great!), but consider adding viewport size variations

---

📝 Code Style & Best Practices

Minor Issues:

1. Inconsistent Error Handling (test-api-client.ts:135-150)

   • Good error handling, but consider extracting error formatting to a helper

2. Magic Strings (BasePage.ts:47-54)

   await this.page.waitForSelector('.alert-success', { timeout });

   Recommendation: Extract to constants:

   private static readonly SELECTORS = {
     SUCCESS_ALERT: '.alert-success',
     ERROR_ALERT: '.alert-danger',
   };

3. Missing JSDoc for Public Methods

   • Most methods have good comments, but some are missing
   • Example: generateTestPassword() could explain the requirements

---

🎨 Suggestions for Improvement

1. Add CI/CD Integration Examples

The PR description mentions CI pipeline, but doesn't show GitHub Actions workflow. Consider adding:

# .github/workflows/playwright.yml
name: Playwright Tests
on: [push, pull_request]
jobs:
  test:
    runs-on: ubuntu-latest
    services:
      mariadb:
        image: mariadb:latest
        # ... database config
    steps:
      - uses: actions/checkout@v3
      - name: Run Playwright tests
        run: ./gradlew playwrightTest

2. Add Test Reporting to CI

tasks.register('playwrightTestCI', Exec) {
    commandLine 'npx', 'playwright', 'test', '--reporter=github,html'
}

3. Consider Adding Visual Regression Testing

Playwright supports screenshot comparison - could be valuable for UI stability

4. Add Accessibility Testing

Consider using @axe-core/playwright for automated accessibility checks

---

✅ Final Verdict

APPROVE with minor recommendations

This is excellent work that significantly improves the project's test coverage and quality. The implementation follows best practices and demonstrates deep understanding of E2E testing patterns.

Before Merging:

1. ✅ Address the critical security concerns around test API deployment
2. ✅ Fix the user deletion cascade issue (or document the limitation)
3. ✅ Remove hard-coded timeout in BasePage.ts
4. ⚠️ Consider adding CI workflow configuration

Post-Merge Enhancements:

• Add visual regression tests
• Expand coverage for edge cases (XSS, concurrent sessions)
• Add performance benchmarking

---

Great work on this comprehensive test suite! 🎉

The framework version bump to 4.0.3 is also noted - ensure that's intentional and documented.