chore: turn off no await in loop rule - FE-7119 #7299
Conversation
…it instead of promise
DipperTheDan
force-pushed
the
FE-7119_review-skipped-pw-accessibility-tests
branch
from
a857a20 to
8a7dbee
Compare
DipperTheDan
force-pushed
the
FE-7119_review-skipped-pw-accessibility-tests
branch
from
8a7dbee to
3dcac7f
Compare
nuria1110
left a comment
nuria1110
left a comment
There was a problem hiding this comment.
Just a couple comments but changes look good to me! Glad we can have more accessibility tests enabled.
| // We have two options here. We can either omit the colour contrast check as the mock component | ||
| // used in the test has a disabled tile select, which is causing this failure. | ||
| // Or we can remove the disabled tile select from the mock component. | ||
| test("should pass accessibility tests for MultiSelect example", async ({ |
There was a problem hiding this comment.
comment: Since the disabled tile is not relevant to what is being tested here I think we could remove it from the mock component, it also helps to make sure the rest of the elements pass colour contrast too. Same with the WithCustomSpacing test.
| await page.keyboard.press(`Tab`); | ||
| } | ||
|
|
||
| await Promise.all(promises); | ||
| }; | ||
|
|
||
| export const continuePressingSHIFTTAB = async (page: Page, count: number) => { | ||
| const promises = []; | ||
|
|
||
| for (let i = 0; i < count; i++) { | ||
| promises.push(page.keyboard.press(`Shift+Tab`)); | ||
| await page.keyboard.press(`Shift+Tab`); | ||
| } | ||
|
|
||
| await Promise.all(promises); | ||
| }; |
There was a problem hiding this comment.
praise: nice job bringing this in-line with the preferred (modern) approach 👍
|
|
||
| // TODO: Skipped due to flaky focus behaviour. To review in FE-6428 | ||
| test.skip("should check accessibility with header children", async ({ | ||
| // This test now seems to pass. |
There was a problem hiding this comment.
question: do we need this comment?
src/components/drawer/drawer.pw.tsx
Outdated
| ["3s", "15s"].forEach((animationDuration) => { | ||
| // TODO: Skipped due to flaky focus behaviour. To review in FE-6428 | ||
| test.skip(`should pass accessibility tests when animation time is set to ${animationDuration}`, async ({ | ||
| // It looks like these are now working as expected. |
There was a problem hiding this comment.
question: same here
src/components/menu/menu.pw.tsx
Outdated
|
|
||
| // Test skipped because of issue FE-5731 | ||
| test.skip(`should pass accessibility tests for Menu with icon`, async ({ | ||
| // Looks like this now passes. |
There was a problem hiding this comment.
question: same here
src/components/menu/menu.pw.tsx
Outdated
|
|
||
| (["left", "right"] as MenuFullscreenProps["startPosition"][]).forEach( | ||
| (side) => { | ||
| // TODO: Skipped due to flaky focus behaviour. To review in FE-6428 |
There was a problem hiding this comment.
suggestion: remove the TODO if it's working
| }); | ||
|
|
||
| ["top", "bottom", "left", "right"].forEach((tooltipPosition) => { | ||
| // TODO: Skipped due to flaky focus behaviour. To review in FE-6428 |
There was a problem hiding this comment.
suggestion: same here
DipperTheDan
added
Pending Review
Pending QA
No QA Required
and removed
DO NOT MERGE
labels
|
🎉 This PR is included in version 153.7.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
6 participants
Proposed behaviour
no-await-in-loopin the eslint configcontinuePressingTabandcontinuePressingShiftTabhelpers to reduce flakiness.Current behaviour
Checklist
d.tsfile added or updated if requiredQA
Additional context
N/A
Testing instructions
While nothing needs to be tested due to the CI pipeline running the Playwright tests, because I have made amendments to the Playwright files, I will need QA approval on the work.