fix(test): address PR review feedback for Playwright E2E tests

devondragon · devondragon · commit 535c57de53a3 · 2026-01-26T11:47:28.000-07:00
Security improvements:
- Add IP whitelist to Test API (localhost only) to prevent accidental exposure
- Remove redundant @EnableWebSecurity annotation from TestApiSecurityConfig
- Remove /api/test/** from unprotectedURIs (handled by TestApiSecurityConfig)

Code quality fixes:
- Add HTTP error handling to TestApiClient with descriptive error messages
- Fix invalid Playwright selector 'visible=true' in UpdateUserPage
- Remove invalid clear() method calls (fill() clears automatically)
- Fix dialog handler in EventDetailsPage to use waitForEvent for reliability
- Add missing test user cleanup in delete-account tests
- Add missing assertion in change-password test
- Update Playwright version in package.json to match lock file (^1.58.0)

Cleanup:
- Remove unused imports from 10 page object files
- Remove unused variables and imports from 5 test files
- Add warning log when ROLE_USER not found in TestDataController
- Document foreign key constraint handling in deleteTestUser
- Document test DB credentials as acceptable for local-only profile
diff --git a/playwright/package.json b/playwright/package.json
@@ -14,7 +14,7 @@
     "codegen": "playwright codegen http://localhost:8080"
   },
   "devDependencies": {
-    "@playwright/test": "^1.40.0",
+    "@playwright/test": "^1.58.0",
     "@types/node": "^20.10.0",
     "typescript": "^5.3.0",
     "dotenv": "^16.3.0"
diff --git a/playwright/src/pages/AdminActionsPage.ts b/playwright/src/pages/AdminActionsPage.ts
@@ -1,4 +1,4 @@
-import { Page, Locator, expect } from '@playwright/test';
+import { Page, Locator } from '@playwright/test';
 import { BasePage } from './BasePage';
 
 /**
diff --git a/playwright/src/pages/BasePage.ts b/playwright/src/pages/BasePage.ts
@@ -1,4 +1,4 @@
-import { Page, Locator, expect } from '@playwright/test';
+import { Page } from '@playwright/test';
 
 /**
  * Base Page Object class providing common functionality for all pages.
diff --git a/playwright/src/pages/DeleteAccountPage.ts b/playwright/src/pages/DeleteAccountPage.ts
@@ -1,4 +1,4 @@
-import { Page, Locator, expect } from '@playwright/test';
+import { Page, Locator } from '@playwright/test';
 import { BasePage } from './BasePage';
 
 /**
diff --git a/playwright/src/pages/EventDetailsPage.ts b/playwright/src/pages/EventDetailsPage.ts
@@ -1,4 +1,4 @@
-import { Page, Locator, expect } from '@playwright/test';
+import { Page, Locator } from '@playwright/test';
 import { BasePage } from './BasePage';
 
 /**
@@ -76,11 +76,11 @@ export class EventDetailsPage extends BasePage {
    * Register for the event.
    */
   async register(): Promise<void> {
-    // Set up dialog handler for the alert
-    this.page.once('dialog', async (dialog) => {
-      await dialog.accept();
-    });
+    // Set up dialog handler for the alert and verify it appears
+    const dialogPromise = this.page.waitForEvent('dialog', { timeout: 5000 });
     await this.registerButton.click();
+    const dialog = await dialogPromise;
+    await dialog.accept();
     // Wait for page to reload
     await this.page.waitForLoadState('networkidle');
   }
@@ -89,11 +89,11 @@ export class EventDetailsPage extends BasePage {
    * Unregister from the event.
    */
   async unregister(): Promise<void> {
-    // Set up dialog handler for the alert
-    this.page.once('dialog', async (dialog) => {
-      await dialog.accept();
-    });
+    // Set up dialog handler for the alert and verify it appears
+    const dialogPromise = this.page.waitForEvent('dialog', { timeout: 5000 });
     await this.unregisterButton.click();
+    const dialog = await dialogPromise;
+    await dialog.accept();
     // Wait for page to reload
     await this.page.waitForLoadState('networkidle');
   }
diff --git a/playwright/src/pages/EventListPage.ts b/playwright/src/pages/EventListPage.ts
@@ -1,4 +1,4 @@
-import { Page, Locator, expect } from '@playwright/test';
+import { Page, Locator } from '@playwright/test';
 import { BasePage } from './BasePage';
 
 /**
@@ -82,9 +82,6 @@ export class EventListPage extends BasePage {
     const name = await card.locator('.card-title').textContent();
     const description = await card.locator('.card-text.text-muted').textContent();
 
-    // Get date and location from the card text
-    const cardBody = await card.locator('.card-body').textContent();
-
     return {
       name: name?.trim() || '',
       description: description?.trim() || '',
diff --git a/playwright/src/pages/ForgotPasswordPage.ts b/playwright/src/pages/ForgotPasswordPage.ts
@@ -1,4 +1,4 @@
-import { Page, Locator, expect } from '@playwright/test';
+import { Page, Locator } from '@playwright/test';
 import { BasePage } from './BasePage';
 
 /**
diff --git a/playwright/src/pages/LoginPage.ts b/playwright/src/pages/LoginPage.ts
@@ -1,4 +1,4 @@
-import { Page, Locator, expect } from '@playwright/test';
+import { Page, Locator } from '@playwright/test';
 import { BasePage } from './BasePage';
 
 /**
diff --git a/playwright/src/pages/RegisterPage.ts b/playwright/src/pages/RegisterPage.ts
@@ -1,4 +1,4 @@
-import { Page, Locator, expect } from '@playwright/test';
+import { Page, Locator } from '@playwright/test';
 import { BasePage } from './BasePage';
 
 /**
diff --git a/playwright/src/pages/UpdatePasswordPage.ts b/playwright/src/pages/UpdatePasswordPage.ts
@@ -1,4 +1,4 @@
-import { Page, Locator, expect } from '@playwright/test';
+import { Page, Locator } from '@playwright/test';
 import { BasePage } from './BasePage';
 
 /**
diff --git a/playwright/src/pages/UpdateUserPage.ts b/playwright/src/pages/UpdateUserPage.ts
@@ -1,4 +1,4 @@
-import { Page, Locator, expect } from '@playwright/test';
+import { Page, Locator } from '@playwright/test';
 import { BasePage } from './BasePage';
 
 /**
@@ -33,8 +33,8 @@ export class UpdateUserPage extends BasePage {
     this.firstNameError = page.locator('#firstNameError');
     this.lastNameError = page.locator('#lastNameError');
     // Use role-based selectors with exact name to get visible buttons/links
-    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();
   }
 
   /**
@@ -55,9 +55,7 @@ export class UpdateUserPage extends BasePage {
    * Fill in profile form.
    */
   async fillForm(firstName: string, lastName: string): Promise<void> {
-    await this.firstNameInput.clear();
     await this.firstNameInput.fill(firstName);
-    await this.lastNameInput.clear();
     await this.lastNameInput.fill(lastName);
   }
 
diff --git a/playwright/src/utils/test-api-client.ts b/playwright/src/utils/test-api-client.ts
@@ -129,6 +129,26 @@ export class TestApiClient {
     return this.context;
   }
 
+  /**
+   * Check response status and throw if not ok.
+   */
+  private async checkResponse(response: any, endpoint: string): Promise<any> {
+    if (!response.ok()) {
+      const statusText = response.statusText();
+      const status = response.status();
+      let errorBody = '';
+      try {
+        errorBody = JSON.stringify(await response.json());
+      } catch {
+        errorBody = await response.text().catch(() => 'Unable to read response body');
+      }
+      throw new Error(
+        `Test API request failed: ${endpoint} returned ${status} ${statusText}. Body: ${errorBody}`
+      );
+    }
+    return response.json();
+  }
+
   /**
    * Check if a user exists.
    */
@@ -137,7 +157,7 @@ export class TestApiClient {
     const response = await context.get(`/api/test/user/exists`, {
       params: { email },
     });
-    return response.json();
+    return this.checkResponse(response, '/api/test/user/exists');
   }
 
   /**
@@ -148,7 +168,7 @@ export class TestApiClient {
     const response = await context.get(`/api/test/user/enabled`, {
       params: { email },
     });
-    return response.json();
+    return this.checkResponse(response, '/api/test/user/enabled');
   }
 
   /**
@@ -159,7 +179,7 @@ export class TestApiClient {
     const response = await context.get(`/api/test/user/details`, {
       params: { email },
     });
-    return response.json();
+    return this.checkResponse(response, '/api/test/user/details');
   }
 
   /**
@@ -170,7 +190,7 @@ export class TestApiClient {
     const response = await context.get(`/api/test/user/verification-token`, {
       params: { email },
     });
-    return response.json();
+    return this.checkResponse(response, '/api/test/user/verification-token');
   }
 
   /**
@@ -181,7 +201,7 @@ export class TestApiClient {
     const response = await context.get(`/api/test/user/password-reset-token`, {
       params: { email },
     });
-    return response.json();
+    return this.checkResponse(response, '/api/test/user/password-reset-token');
   }
 
   /**
@@ -192,7 +212,7 @@ export class TestApiClient {
     const response = await context.post(`/api/test/user`, {
       data: userData,
     });
-    return response.json();
+    return this.checkResponse(response, '/api/test/user');
   }
 
   /**
@@ -203,7 +223,7 @@ export class TestApiClient {
     const response = await context.delete(`/api/test/user`, {
       params: { email },
     });
-    return response.json();
+    return this.checkResponse(response, '/api/test/user');
   }
 
   /**
@@ -214,7 +234,7 @@ export class TestApiClient {
     const response = await context.post(`/api/test/user/enable`, {
       params: { email },
     });
-    return response.json();
+    return this.checkResponse(response, '/api/test/user/enable');
   }
 
   /**
@@ -225,7 +245,7 @@ export class TestApiClient {
     const response = await context.post(`/api/test/user/unlock`, {
       params: { email },
     });
-    return response.json();
+    return this.checkResponse(response, '/api/test/user/unlock');
   }
 
   /**
@@ -237,7 +257,7 @@ export class TestApiClient {
     const response = await context.post(`/api/test/user/verification-token`, {
       params: { email },
     });
-    return response.json();
+    return this.checkResponse(response, '/api/test/user/verification-token');
   }
 
   /**
@@ -246,7 +266,7 @@ export class TestApiClient {
   async health(): Promise<HealthResponse> {
     const context = this.ensureContext();
     const response = await context.get(`/api/test/health`);
-    return response.json();
+    return this.checkResponse(response, '/api/test/health');
   }
 
   /**
diff --git a/playwright/tests/auth/password-reset.spec.ts b/playwright/tests/auth/password-reset.spec.ts
@@ -1,4 +1,4 @@
-import { test, expect, generateTestUser, generateTestPassword } from '../../src/fixtures';
+import { test, expect, generateTestUser } from '../../src/fixtures';
 
 test.describe('Password Reset', () => {
   test.describe('Request Reset', () => {
diff --git a/playwright/tests/auth/registration.spec.ts b/playwright/tests/auth/registration.spec.ts
@@ -1,4 +1,4 @@
-import { test, expect, generateTestUser, generateTestEmail } from '../../src/fixtures';
+import { test, expect, generateTestUser } from '../../src/fixtures';
 
 test.describe('Registration', () => {
   test.describe('Valid Registration', () => {
@@ -151,8 +151,6 @@ test.describe('Registration', () => {
       page,
       registerPage,
     }) => {
-      const user = generateTestUser('invalid-email');
-
       await registerPage.goto();
 
       // HTML5 email validation should prevent submission
@@ -190,6 +188,7 @@ test.describe('Registration', () => {
 
       // Password strength indicator should become visible
       const strengthVisible = await registerPage.isPasswordStrengthVisible();
+      expect(strengthVisible).toBe(true);
       // Note: This depends on JavaScript being enabled and working
     });
   });
diff --git a/playwright/tests/e2e/complete-user-journey.spec.ts b/playwright/tests/e2e/complete-user-journey.spec.ts
@@ -1,4 +1,4 @@
-import { test, expect, generateTestUser, generateTestEmail } from '../../src/fixtures';
+import { test, expect, generateTestUser } from '../../src/fixtures';
 
 /**
  * Complete End-to-End User Journey Test
diff --git a/playwright/tests/events/browse-events.spec.ts b/playwright/tests/events/browse-events.spec.ts
@@ -14,7 +14,6 @@ test.describe('Browse Events', () => {
       expect(page.url()).toContain('event');
 
       // Events should be visible (if any exist)
-      const eventCount = await eventListPage.getEventCount();
       // At least verify the page loads, event count depends on data
       await expect(eventListPage.eventCards.first()).toBeVisible().catch(() => {
         // No events is also valid
diff --git a/playwright/tests/profile/change-password.spec.ts b/playwright/tests/profile/change-password.spec.ts
@@ -98,6 +98,7 @@ test.describe('Change Password', () => {
       // Should show error or stay on page
       const isError = await updatePasswordPage.isErrorMessage() ||
                       await updatePasswordPage.hasCurrentPasswordError();
+      expect(isError).toBe(true);
 
       // Verify original password still works
       const details = await testApiClient.getUserDetails(user.email);
diff --git a/playwright/tests/profile/delete-account.spec.ts b/playwright/tests/profile/delete-account.spec.ts
@@ -10,7 +10,8 @@ test.describe('Delete Account', () => {
     }) => {
       // Create and login as a verified user
       const user = generateTestUser('delete-account');
-      // Don't add to cleanupEmails since we're deleting it
+      // Add to cleanupEmails in case deletion fails
+      cleanupEmails.push(user.email);
 
       await createAndLoginUser(page, testApiClient, user);
 
@@ -34,9 +35,12 @@ test.describe('Delete Account', () => {
       deleteAccountPage,
       loginPage,
       testApiClient,
+      cleanupEmails,
     }) => {
       // Create and login as a verified user
       const user = generateTestUser('delete-logout');
+      // Add to cleanupEmails in case deletion fails
+      cleanupEmails.push(user.email);
 
       await createAndLoginUser(page, testApiClient, user);
 
@@ -61,9 +65,12 @@ test.describe('Delete Account', () => {
       deleteAccountPage,
       loginPage,
       testApiClient,
+      cleanupEmails,
     }) => {
       // Create and login as a verified user
       const user = generateTestUser('delete-no-login');
+      // Add to cleanupEmails in case deletion fails
+      cleanupEmails.push(user.email);
       const password = user.password;
 
       await createAndLoginUser(page, testApiClient, user);
diff --git a/src/main/java/com/digitalsanctuary/spring/demo/test/api/TestDataController.java b/src/main/java/com/digitalsanctuary/spring/demo/test/api/TestDataController.java
@@ -197,6 +197,8 @@ public ResponseEntity<Map<String, Object>> createTestUser(@RequestBody CreateUse
         Role userRole = roleRepository.findByName("ROLE_USER");
         if (userRole != null) {
             user.setRoles(Collections.singletonList(userRole));
+        } else {
+            log.warn("Test API: ROLE_USER not found in database - user will be created without roles");
         }
 
         User savedUser = userRepository.save(user);
@@ -226,7 +228,9 @@ public ResponseEntity<Map<String, Object>> deleteTestUser(@RequestParam String e
             return ResponseEntity.status(HttpStatus.NOT_FOUND).body(response);
         }
 
-        // Delete related tokens first
+        // Delete related tokens first to avoid 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.
         VerificationToken verificationToken = verificationTokenRepository.findByUser(user);
         if (verificationToken != null) {
             verificationTokenRepository.delete(verificationToken);
diff --git a/src/main/java/com/digitalsanctuary/spring/demo/test/config/TestApiSecurityConfig.java b/src/main/java/com/digitalsanctuary/spring/demo/test/config/TestApiSecurityConfig.java
@@ -18,21 +18,29 @@
  */
 @Slf4j
 @Configuration
-@EnableWebSecurity
 @Profile("playwright-test")
 public class TestApiSecurityConfig {
 
     /**
      * Configure security for test API endpoints. This filter chain has higher priority (lower order number) than the
      * default security configuration, so it will be applied first for /api/test/** paths.
+     * <p>
+     * SECURITY: Restricts test API access to localhost only to prevent accidental exposure.
      */
     @Bean
     @Order(1)
     public SecurityFilterChain testApiSecurityFilterChain(HttpSecurity http) throws Exception {
-        log.info("Configuring Test API security - CSRF disabled for /api/test/**");
+        log.info("Configuring Test API security - CSRF disabled for /api/test/** (localhost only)");
 
         http.securityMatcher("/api/test/**")
-                .authorizeHttpRequests(authorize -> authorize.anyRequest().permitAll())
+                .authorizeHttpRequests(authorize -> authorize
+                        .requestMatchers(request -> {
+                            String remoteAddr = request.getRemoteAddr();
+                            return "127.0.0.1".equals(remoteAddr) ||
+                                   "0:0:0:0:0:0:0:1".equals(remoteAddr) ||
+                                   "localhost".equals(remoteAddr);
+                        }).permitAll()
+                        .anyRequest().denyAll())
                 .csrf(csrf -> csrf.disable());
 
         return http.build();
diff --git a/src/main/resources/application-playwright-test.yml b/src/main/resources/application-playwright-test.yml

Original file line number	Diff line number	Diff line change
`@@ -1,4 +1,4 @@`
`1`		`-import { Page, Locator, expect } from '@playwright/test';`
	`1`	`+import { Page, Locator } from '@playwright/test';`
`2`	`2`	`import { BasePage } from './BasePage';`
`3`	`3`
`4`	`4`	`/**`
Original file line number	Diff line number	Diff line change
`@@ -1,4 +1,4 @@`
`1`		`-import { Page, Locator, expect } from '@playwright/test';`
	`1`	`+import { Page } from '@playwright/test';`
`2`	`2`
`3`	`3`	`/**`
`4`	`4`	`* Base Page Object class providing common functionality for all pages.`