Skip to content

PRO-365: e2e_tests - Add response analysis page tests#1397

Open
georgia-i-ai wants to merge 4 commits into
mainfrom
grajamanoharan/pro-365-e2e-response-analysis-page
Open

PRO-365: e2e_tests - Add response analysis page tests#1397
georgia-i-ai wants to merge 4 commits into
mainfrom
grajamanoharan/pro-365-e2e-response-analysis-page

Conversation

@georgia-i-ai

Copy link
Copy Markdown
Contributor

Context

Adds E2E test coverage for the response analysis page (question detail view). The page has several interactive filtering
mechanisms — demographic filters, multichoice answer filters, theme filters, flagged toggle — none of which had automated tests.

Changes proposed in this pull request

New file e2e_tests/tests/response-analysis.e2e.ts with:

  • testFilter helper — reusable function that clicks a filter, verifies the count displayed on the filter button is unchanged after selection, checks the response list shows the expected number of results, verifies other filters in the same group drop to zero, then resets and confirms the full list is restored. Supports OR (same-group) and AND (cross-group) operators.
  • getFilterButton helper — resolves filter buttons either by accessible name (demographic filters, theme filters) or by a scoped label selector (multichoice answer filters, where button names like "No" would otherwise match "No answer").
  • Demographic filter tests — checks each nation and age group filter individually, two filters in the same group (OR), and two filters across groups (AND).
  • Multichoice answer filter tests — checks each answer option filter; uses span[class~="sm:hidden"] for the count since the multichoice buttons have a different DOM structure to demographic filter buttons.
  • Theme filter tests — checks each theme filter using the ThemesTable's td:nth-child(2) for the count column, scoped to the table element to avoid matching theme chip buttons on response cards.
  • Flagged toggle test — flags the first response, enables the flagged-only toggle, asserts exactly one response is shown, then cleans up.
  • Respondent navigation test — clicks the respondent button on the first response and asserts navigation to the respondent detail page.

Guidance to review

The selector choices are non-obvious in places:

  • containerSelector: 'section:has-text("Multiple Choice Answers")' — scopes multichoice filter buttons away from demographic filters in the aside.
  • containerSelector: 'section:has-text("Theme analysis") table' — scopes to the table specifically to exclude theme chip buttons on response cards, which share the same name.
  • labelSelector: 'h3' on multichoice filters — matches buttons by their inner heading text rather than accessible name, which avoids "No" matching "No answer".
  • getByRole('switch', ...) for the flagged toggle — the melt-ui Switch component renders role="switch" via melt-ui.

Notes

The testFilter helper function got quite complex in order to handle all of the different ways in which I wanted to use it. Open to ideas on how to simplify. Also happy to move any of the local helpers to helpers.ts if it is thought they would be useful elsehwere.

▎ 🤖 PR description mostly generated with Claude Code (https://claude.ai/claude-code)

@linear

linear Bot commented Jun 5, 2026

Copy link
Copy Markdown

PRO-365

@github-actions

github-actions Bot commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Claude finished @georgia-i-ai's task in 1m 18s —— View job


PR Review

  • Gather context
  • Review changed files
  • Post feedback

Good coverage of a previously untested page. The testFilter helper is clever and the selector choices are well-documented.

One bug:

🔴 DO: The respondent URL regex /\/respondent\/\d+/ won't match — Respondent uses UUIDs as PKs (via UUIDPrimaryKeyModel). [e2e_tests/tests/response-analysis.e2e.ts:214] — Fix this →

// line 214 — \d+ won't match a UUID
await page.waitForURL(/\/respondent\/\d+/);
// should be:
await page.waitForURL(/\/respondent\/[0-9a-f-]+/);

Compare with respondent-detail.e2e.ts:53 which correctly uses /\/consultations\/.*\/respondent\/.*/.

⚪️ NIT: The sleep(1000) calls on lines 84 and 112 are fragile — waitForLoadState('networkidle') should already cover debounce; worth removing and seeing if tests pass without them.

@georgia-i-ai

Copy link
Copy Markdown
Contributor Author

Will look at the issue raised by Claude but it seemed to be matching fine (and tests pass).

@Nazehs Nazehs left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM! just some minor comments and suggestions

Comment thread e2e_tests/tests/users.e2e.ts Outdated
});
});

test.beforeEach(async ({ page }) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggestion: Can we extract this to make it reusable similar to the below which I believe response-analysis.e2e.ts, respondent-detail.e2e.ts, and questions.e2e.ts could benefits from

  export async function goToFirstQuestion(page: Page, consultationId: string, questionId: string) {
    await page.goto(`/consultations/${consultationId}`);
    await page.waitForLoadState("networkidle");
    await page.getByTestId(`question-link-${questionId}`).click();
    await page.waitForURL(/\/questions\//);
    await page.waitForLoadState("networkidle");
  }

@georgia-i-ai georgia-i-ai Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure good idea - I haven't looked to much at the other tests so keep for suggestions of reusable code to move into helpers.ts

  • move setup into helper function

Comment thread e2e_tests/tests/response-analysis.e2e.ts Outdated
@georgia-i-ai georgia-i-ai force-pushed the grajamanoharan/pro-365-e2e-response-analysis-page branch from c99c465 to 4a082f6 Compare June 11, 2026 15:51
…ts include checking that filters work as expected, that selecting themes filters responses and flagged filters work as expected, and that clicking on a respondent goes to details page.

   PRO-365
@georgia-i-ai georgia-i-ai force-pushed the grajamanoharan/pro-365-e2e-response-analysis-page branch from 4a082f6 to 53e525c Compare June 11, 2026 15:52
@github-actions

github-actions Bot commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Claude finished @georgia-i-ai's task in 2m 3s —— View job


PR Review

  • Gather context
  • Review changed files
  • Post feedback

Good coverage overall, and the review feedback from Nazehs has been addressed. Two issues remain:

🔴 DO: \/respondent\/\d+ on line 214 still won't match the UUID-based URL — Respondent uses UUIDPrimaryKeyModel so the path is /consultations/{uuid}/respondent/{uuid}. The previous review flagged this; the commit message says it was "fixed" but line 214 is unchanged. Use the same pattern as respondent-detail.e2e.ts:56: [e2e_tests/tests/response-analysis.e2e.ts:214] — Fix this →

// line 214 — won't match UUID
await page.waitForURL(/\/respondent\/\d+/);
// should match respondent-detail.e2e.ts:56 pattern
await expect(page).toHaveURL(/\/consultations\/.*\/respondent\/.*/);

⚪️ NIT: The sleep(1000) calls (lines 88, 116) are fragile. waitForLoadState('networkidle') precedes them both — worth removing and confirming tests still pass.

@georgia-i-ai

georgia-i-ai commented Jun 11, 2026

Copy link
Copy Markdown
Contributor Author

⚪️ NIT: The sleep(1000) calls (lines 88, 116) are fragile. waitForLoadState('networkidle') precedes them both — worth removing and confirming tests still pass.

This seems to be needed even despite the wait for network call - without it the response count is incorrect

@github-actions

github-actions Bot commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Claude finished @georgia-i-ai's task in 50s —— View job


PR Review

  • Gather context
  • Review changed files
  • Post feedback

The UUID regex fix is now in place on line 214. Looks good overall.

⚪️ NIT: getFilterButton uses ^${filter}$ (exact match) for the labelSelector path but only ^${filter} (no end anchor) for the getByRole path (line 35). Harmless with current fixture data but worth aligning.


@georgia-i-ai georgia-i-ai requested a review from Nazehs June 11, 2026 16:04
@georgia-i-ai

Copy link
Copy Markdown
Contributor Author

All comments addressed from @Nazehs and Claude and rebased on main - ready for re-review.

/**
* Goes to a specific question in a consultation
*/
export async function goToQuestion(page: Page, consultationId: string, questionID: string) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

NITPICK: Could you consider naming this maybe goToConsultationQuestion or something to increase clarity and context

});

test.afterAll(async () => {
await deleteFixtureData(testData);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggestion: There is a CleanupManager class now that is in main could you rebase with main and make use of it for the clean up?

Comment on lines +13 to +15
function sleep() {
return setTimeout(1000);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is not async and therefore not working as intended?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Something like this should work:
const sleep = duration => new Promise(resolve => setTimeout(resolve, duration));

await page.waitForLoadState('networkidle');

// Check filter still shows correct count after click (should be same as before)
await sleep(); // Wait for any debounce to finish and count to update

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we not use something like waitForFunction instead of our own sleep?

return setTimeout(1000);
}

const freeTextResponses = (page: Page) =>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we name this as an action like getFreeTextResponses?

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.

3 participants