End to end testing #711
End to end testing #711
Conversation
There was a problem hiding this comment.
Looks great, approved! I left a bunch of comments, feel free to address the ones you think are relevant and then we can merge
Also, could you explain how the service is ran? So when I send a PR, how does the test server get spun up, and how does the backend get credentials for the database?
| name: Playwright Tests | ||
| on: | ||
| push: | ||
| branches: [ main, master ] |
There was a problem hiding this comment.
nit, we can remove master since that's not used
There was a problem hiding this comment.
The workflow doc was autogenerated by playwright. I plan to dive in more and figure out how to actually implement it. But as of right now I don't really know what any of it means
| - name: Install dependencies | ||
| run: npm ci | ||
| - name: Install Playwright Browsers | ||
| run: npx playwright install --with-deps |
There was a problem hiding this comment.
nit, could we add this as a dev dependency, and then alter the previous install command so that we don't have to install this explicitly?
| @@ -14,14 +15,21 @@ interface BubbleGridProps { | |||
| fontSX: object; | |||
| } | |||
|
|
|||
| const NoStyleButton = styled(Button)({ | |||
There was a problem hiding this comment.
No, that was just from trying to get the name prop to work on the bubbles. I must have forgotten to remove it.
| onClick={() => setIsOpen(true)} | ||
| disabled={isPending || currentPage !== pages.length - 1 || pages[currentPage].candidates.every(candidate => candidate.score === null || pages.some(page => page.warnings))}//disable unless on last page and at least one candidate scored | ||
| disabled={submitButtonDisabled}//disable unless on last page and at least one candidate scored |
There was a problem hiding this comment.
Good idea breaking those out into variables, it's much more readable now
There was a problem hiding this comment.
Yeah, it wasn't behaving how it was supposed to do I had to break it into smaller pieces to make it easier to debug
| // label={"Candidate Name"} | ||
| id={`candidate-name-${index + 1}`} | ||
| name={`candidate-name-${index + 1}`} | ||
| data-testid={`candidate-name-${index + 1}`} |
There was a problem hiding this comment.
Looks like there's several parameters being added in order for the tests to retrieve elements (role, aria-label, name, data-testid, etc). Is there a best practice on which one to use when we're writing tests?
It sounds like role and aria-label are things we should be including anyway for making the site screen reader friendly, so I like the idea of tests forcing us to improve the code
There was a problem hiding this comment.
I'm not sure, if there is or not. I tried to avoid data-testid as much as possible though to use name, role, or label instead so it would be screen reader friendly
| @@ -0,0 +1,437 @@ | |||
| import { test, expect, type Page } from '@playwright/test'; | |||
There was a problem hiding this comment.
Looks like this directory was included by accident?
| test.describe('Create Election', () => { | ||
| test('create poll', async ({ page }) => { | ||
| page.goto('/'); | ||
| await page.getByRole('button', { name: 'New Election' }).click(); |
There was a problem hiding this comment.
Loving these tests, they look really clean and readable!
| await page.getByRole('button', { name: 'Continue' }).click(); | ||
| await page.getByLabel('No').click(); | ||
| await page.getByRole('button', { name: 'Continue' }).click(); | ||
| await page.getByText('Allows multiple votes per device').click(); |
There was a problem hiding this comment.
getByText could be brittle. Ideally we could make changes to en.yaml without breaking any tests. Then again I don't really like the alternative of adding data-testid's to all the elements either 🤔
Anyway, this looks good for now, I just wanted to flag that thought
There was a problem hiding this comment.
Yeah, maybe we could get the text to test against from en.yaml
| await page.getByRole('button', { name: 'Continue' }).click(); | ||
| await page.getByRole('button', { name: 'Email List Provide a list of' }).click(); | ||
| await expect(page.getByText('draft')).toBeVisible(); | ||
| const url = await page.url(); |
There was a problem hiding this comment.
nit, it would be good to make a helper function for "getElectionIdFromUrl", maybe we could add a util file under testing for functions like that?
There was a problem hiding this comment.
Yeah, that would probably be good. I had a hard time getting helper functions to work outside of tests though because the await keyword means something else within a test than it normally does within an async function
| @@ -0,0 +1,5 @@ | |||
| import { test, expect } from '@playwright/test'; | |||
There was a problem hiding this comment.
Should this file be included?
csamuele
force-pushed
the
end-to-end-testing
branch
from
677cc77 to
083e7c1
Compare
Description
Added playwright end to end testing. This includes election creation and voting with several methods and restrictions