Skip to content

Conversation

@bhollis
Copy link
Contributor

@bhollis bhollis commented Jul 14, 2025

I'd like to have a testing suite that uses the actual browser. I asked Claude Code to set up Playwright which is the du jour browser testing tool. Then I asked it to set things up to use mock data (the same profile data exports we use for tests) instead of needing a real Bungie.net account, and then to write some basic tests by exploring the website on its own and seeing what the behavior should be.

It's truly terrible at this - it can't consistently remember how to run the tests, and it'll fix one test and break another. It also wrote what IMO is just some really awful tests.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR sets up a Playwright-based end-to-end test suite with mock Bungie API data and integrates an e2eMode feature flag to return test fixtures instead of real API calls.

  • Extracted mock profile and vendor data into a dedicated test-profile.ts module.
  • Added conditional branches ($featureFlags.e2eMode) across APIs and state management to serve mock data in E2E runs.
  • Introduced Playwright configuration, scripts, and a comprehensive set of E2E tests under e2e/, plus a shared InventoryHelpers class.

Reviewed Changes

Copilot reviewed 23 out of 25 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/testing/test-utils.ts Removed inline getTestProfile/getTestVendors in favor of new helper module
src/testing/test-profile.ts New module exporting getTestProfile and getTestVendors
src/app/vendors/d2-vendors.test.ts Updated imports to use testing/test-profile
src/app/loadout-analyzer/analysis.test.ts Updated imports to use testing/test-profile
src/app/dim-api/dim-api.ts Added mock getDimApiProfile path under e2eMode flag
src/app/bungie-api/destiny2-api.ts Added mock branches for linked accounts, profile, and vendors under e2eMode
src/app/accounts/reducer.ts Changed state initialization to bypass login/developer checks in e2eMode
src/app/accounts/bungie-account.ts Added mock Bungie account in e2eMode
playwright.config.ts New Playwright test configuration
package.json Added Playwright dependency and E2E scripts
config/feature-flags.ts Introduced e2eMode flag based on E2E_MOCK_DATA
config/webpack.ts Adjusted Babel loader exclusions and copied manifest cache for E2E
eslint.config.js Expanded test file patterns to include destiny2-api.ts
e2e/helpers/inventory-helpers.ts Added shared helper methods for inventory E2E tests
Files not reviewed (1)
  • pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (2)

config/webpack.ts:309

  • Dropping the /testing/ exclusion will allow test-only files to be bundled—consider re-adding /testing/ to the exclude array to keep test utilities out of production builds.
          exclude: [/\.test\.ts$/],

eslint.config.js:443

  • Including destiny2-api.ts in the test file patterns disables import restrictions on a production file—adjust this pattern so only actual test files are affected.
    files: ['**/*.test.ts', '**/destiny2-api.ts'],

Comment on lines +348 to +355
/**
* Navigate to the inventory page and wait for it to load
*/
async navigateToInventory(): Promise<void> {
await this.page.goto('/');
await this.waitForInventoryLoad();
}

Copy link

Copilot AI Jul 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a second definition of navigateToInventory starting here; remove or consolidate it with the earlier method to avoid duplicate identifiers.

Suggested change
/**
* Navigate to the inventory page and wait for it to load
*/
async navigateToInventory(): Promise<void> {
await this.page.goto('/');
await this.waitForInventoryLoad();
}

Copilot uses AI. Check for mistakes.
/**
* Verify that character stats are displayed with expected values
*/
async verifyCharacterStats(expectedStats: { [stat: string]: string }): Promise<void> {
Copy link

Copilot AI Jul 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The class already defines verifyCharacterStats() without parameters above; having two methods with the same name causes a duplicate identifier error—merge or rename one.

Suggested change
async verifyCharacterStats(expectedStats: { [stat: string]: string }): Promise<void> {
async verifyCharacterStatsWithExpectedValues(expectedStats: { [stat: string]: string }): Promise<void> {

Copilot uses AI. Check for mistakes.
await page.goto('/');

// Wait for the app to load successfully - be more flexible about timing
await page.waitForTimeout(5000);
Copy link

Copilot AI Jul 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using a fixed timeout can lead to flaky tests; replace waitForTimeout(5000) with a wait on a specific element or network idle for more reliable synchronization.

Suggested change
await page.waitForTimeout(5000);
await page.waitForSelector('header', { state: 'visible', timeout: 15000 });

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant