Skip to content

test: fix flaky combobox test#625

Open
mrTuomoK wants to merge 1 commit into
mainfrom
LINK-2581-fix-flaky-tests
Open

test: fix flaky combobox test#625
mrTuomoK wants to merge 1 commit into
mainfrom
LINK-2581-fix-flaky-tests

Conversation

@mrTuomoK

@mrTuomoK mrTuomoK commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Refs: LINK-2581

Description ✨

Hopefully now fixes the combobox test which failed often in CI.

Issues 🐛

Closes 🙅‍♀️

LINK-2581:

Summary by CodeRabbit

  • Tests
    • Improved test reliability by ensuring React Testing Library cleanup runs after each test.
    • Updated the selected-keyword UI assertion to wait for the option button to appear asynchronously (with a longer timeout), while preserving the existing verification.

@mrTuomoK mrTuomoK requested a review from a team as a code owner July 10, 2026 05:15
@coderabbitai

coderabbitai Bot commented Jul 10, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The classification section test now cleans up after each test and uses an explicit 5000ms timeout when locating the selected keyword button.

Changes

Classification selection test

Layer / File(s) Summary
Selection test stability
src/domain/event/formSections/classificationSection/__tests__/ClassificationSection.test.tsx
Adds cleanup() to per-test teardown and extends the selected keyword button lookup with an explicit 5000ms timeout while preserving the accessible-name match.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Possibly related PRs

Suggested reviewers: jorilindell, tjgofore

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description has a brief summary and linked issue, but it omits several template sections like Related, Testing, Screenshots, and Additional notes. Add the missing template sections, especially Related and Testing, and include automated/manual test details; add screenshots or notes if applicable.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise and accurately reflects the main change: fixing a flaky combobox test.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch LINK-2581-fix-flaky-tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
src/domain/event/formSections/classificationSection/__tests__/ClassificationSection.test.tsx (1)

266-277: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Prefer findByRole with timeout over waitFor + getByRole.

findByRole already polls internally and accepts { timeout: 5000 } as a third argument. The current waitFor(() => expect(getByRole(...)).toBeInTheDocument()) pattern is functionally equivalent but less idiomatic and produces less helpful failure messages. The simpler form also matches the existing usage of findByRole in the findElement helper at line 121.

♻️ Proposed refactor
-  await waitFor(
-    () =>
-      expect(
-        screen.getByRole('button', {
-          name: new RegExp(
-            `1 valittu vaihtoehto.*${getValue(keyword?.name?.fi, '')}`,
-            'i'
-          ),
-        })
-      ).toBeInTheDocument(),
-    { timeout: 5000 }
-  );
+  await screen.findByRole(
+    'button',
+    {
+      name: new RegExp(
+        `1 valittu vaihtoehto.*${getValue(keyword?.name?.fi, '')}`,
+        'i'
+      ),
+    },
+    { timeout: 5000 }
+  );
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@src/domain/event/formSections/classificationSection/__tests__/ClassificationSection.test.tsx`
around lines 266 - 277, Replace the waitFor/getByRole assertion in the
ClassificationSection test with screen.findByRole, passing the existing button
name matcher and { timeout: 5000 } options, and assert the awaited result is in
the document. Follow the existing findElement helper’s findByRole usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In
`@src/domain/event/formSections/classificationSection/__tests__/ClassificationSection.test.tsx`:
- Around line 266-277: Replace the waitFor/getByRole assertion in the
ClassificationSection test with screen.findByRole, passing the existing button
name matcher and { timeout: 5000 } options, and assert the awaited result is in
the document. Follow the existing findElement helper’s findByRole usage.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 68600cf4-bf6e-42f5-b3c3-65607d47fb98

📥 Commits

Reviewing files that changed from the base of the PR and between 69074b2 and 0903fe1.

📒 Files selected for processing (1)
  • src/domain/event/formSections/classificationSection/__tests__/ClassificationSection.test.tsx

@mrTuomoK mrTuomoK force-pushed the LINK-2581-fix-flaky-tests branch from 0903fe1 to 889a400 Compare July 10, 2026 05:21
@azure-pipelines

Copy link
Copy Markdown

LINKEDCOMPONENTS-UI branch is deployed to platta: https://linkedcomponents-ui-pr625.dev.hel.ninja 🚀🚀🚀

@azure-pipelines

Copy link
Copy Markdown

e2e tests result is success for https://linkedcomponents-ui-pr625.dev.hel.ninja 😆🎉🎉🎉

@mrTuomoK mrTuomoK force-pushed the LINK-2581-fix-flaky-tests branch from 889a400 to a906df9 Compare July 10, 2026 07:12

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
src/domain/event/formSections/classificationSection/__tests__/ClassificationSection.test.tsx (1)

268-277: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Timeout increase is appropriate, but note other findByRole calls lack explicit timeouts.

The explicit { timeout: 5000 } on this assertion is a reasonable mitigation for CI flakiness where the selected keyword button may take longer to render. However, the findElement helper (lines 123, 127, 131) also uses findByRole / findByRole with the default 1000ms timeout for option and checkbox lookups. If the CI slowness is systemic, those calls could become future flakiness sources. Consider applying a consistent timeout to all async queries if flakiness recurs.

As per library context, @testing-library/react 16.2.0 defaults findBy* queries to 1000ms.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@src/domain/event/formSections/classificationSection/__tests__/ClassificationSection.test.tsx`
around lines 268 - 277, Use a consistent extended timeout for the async role
lookups in the findElement helper, including its option and checkbox findByRole
calls, matching the 5000ms timeout used by the selected keyword assertion. Keep
the timeout centralized or reusable where practical so future CI slowness does
not leave these queries at the default 1000ms.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In
`@src/domain/event/formSections/classificationSection/__tests__/ClassificationSection.test.tsx`:
- Around line 268-277: Use a consistent extended timeout for the async role
lookups in the findElement helper, including its option and checkbox findByRole
calls, matching the 5000ms timeout used by the selected keyword assertion. Keep
the timeout centralized or reusable where practical so future CI slowness does
not leave these queries at the default 1000ms.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e52adab3-ebea-4069-90ad-4de81d74215a

📥 Commits

Reviewing files that changed from the base of the PR and between 0903fe1 and a906df9.

📒 Files selected for processing (1)
  • src/domain/event/formSections/classificationSection/__tests__/ClassificationSection.test.tsx

@azure-pipelines

Copy link
Copy Markdown

LINKEDCOMPONENTS-UI branch is deployed to platta: https://linkedcomponents-ui-pr625.dev.hel.ninja 🚀🚀🚀

@azure-pipelines

Copy link
Copy Markdown

e2e tests result is success for https://linkedcomponents-ui-pr625.dev.hel.ninja 😆🎉🎉🎉

@sonarqubecloud

Copy link
Copy Markdown

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.

2 participants