Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
[daily-playwright] Add tests for appointments list page #16672
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Uh oh!
There was an error while loading. Please reload this page.
[daily-playwright] Add tests for appointments list page #16672
Changes from all commits
977b18f78cc3b9File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
formatandaddDaysare imported and never used. Forty years in and I still can't get people to read their own lint output. Delete the import before CI does it for you.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (await todayOption.isVisible())and then assertingurlcontainsdate_fromanyway? If the button isn't there, this either flakes or asserts on state you never created. Either the filter exists and you assert it, or you don't test it. Conditional assertions are how tests quietly stop testing anything.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This entire test is
if (visible) expect(visible). It is structurally incapable of failing. That's not a test, that's a 10-line no-op with a docblock. Delete it or assert the practitioner filter actually exists.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same disease: the loop result
statusFilterFoundis computed, branched on, and then thrown away — the only real assertion is "some heading exists". You could delete the status-filter logic entirely and this still passes. Assert the actual status filter, please.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[class*="appointment"]— coupling a test to Tailwind-ish class name fragments. That selector will match anything from a wrapper div to a stray utility class and will break the moment someone renames a component. Use a stabledata-cy/data-testid, which the codebase already uses elsewhere.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
waitForLoadState("networkidle")in every test plus hardcodedwaitForTimeout(1000)/(500)sprinkled around. Playwright's docs explicitly discourage both — they're slow on CI and still racy. Web-first assertions with auto-retry already do this job.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getByRole("link").first() || getByRole("button").first()does NOT fall back — a Locator object is always truthy, so the button branch is dead code and you get a strict-mode/visibility failure instead. Use.or():firstDataRow.getByRole("link").or(firstDataRow.getByRole("button")).first().There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When there are no rows this test logs to the console and passes green. A test that silently succeeds because there was no data is worse than no test — seed an appointment in setup, or
test.skip()so at least the report tells the truth.Uh oh!
There was an error while loading. Please reload this page.