Skip to content
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

SWC-6556: add end-to-end test for file CRUD #5207

Merged
merged 2 commits into from
Oct 20, 2023

Conversation

hallieswan
Copy link
Contributor

No description provided.

Comment on lines +28 to +34
await testAuth.step('file page is loaded', async () => {
await page.waitForURL(entityUrlPathname(fileEntityId))
await expect(page.getByText(`Discussion about ${fileName}`)).toBeVisible()

await expect(page.getByText('Loading provenance...')).not.toBeVisible()
await expect(page.getByText(fileEntityId, { exact: true })).toBeVisible()
await expect(page.getByText('Loading provenance...')).not.toBeVisible()
await expect(page.getByText(fileEntityId, { exact: true })).toBeVisible()
})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved this check into its own step, so the Playwright reports are a bit tidier

Comment on lines -83 to -90
const confirmAndClosePermissionsSavedAlert = async (page: Page) => {
const permissionsAlert = page.getByRole('alert').filter({
has: page.getByText('Permissions were successfully saved to Synapse'),
})
await expect(permissionsAlert).toBeVisible()
await permissionsAlert.getByRole('button').click()
await expect(permissionsAlert).not.toBeVisible()
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pulled this into a new method (dismissAlert), so it can be reused across tests

Comment on lines +139 to +143
await testAuth.step('ensure there was not an upload error', async () => {
await expect(
page.getByRole('heading', { name: 'Upload Error' }),
).not.toBeVisible()
})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Useful for distinguishing between a file upload timeout vs a file upload error when debugging a failed test in CI.

Comment on lines +342 to +348
test.fail(
browserName === 'webkit',
`Playwright is not preserving the File's lastModified time in webkit, so
file upload fails because it seems like the file was modified during
upload. See https://github.com/microsoft/playwright/issues/27452. Fix
planned for Playwright v1.40.`,
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In webkit, Playwright uses the current time for the lastModified time, rather than preserving the value on the File. Test marked as failed until the fixed version is released, likely in mid-November.

Copy link
Contributor

Choose a reason for hiding this comment

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

This webkit engine is just causing all sorts of problems!

Comment on lines +454 to +455
await testAuth.step('move file to trash can', async () => {
await testAuth.step('delete file', async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Playwright steps can be nested, which makes reports a bit easier to read

@@ -11,7 +11,7 @@ export type UserPrefixes = (typeof userPrefixes)[number]
const generateUserName = (prefix: string) => {
// uncomment to use static username for troubleshooting:
// ...and ensure that usernames are unique per worker
// return `${prefix}-${process.env.TEST_PARALLEL_INDEX}`
// return `local-${prefix}-${process.env.TEST_PARALLEL_INDEX}`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In local tests using static usernames, the lingering test users on the backend dev stack caused multiple users to appear in the user suggestion dropdown, when we expect one user to appear (e.g. swc-e2e-user-validated-0 and swc-e2e-user-validated-0d930519-5c31-4137-9def-8fe9ea483f22). Add a prefix for local testing, so that the local static test users are easily selected.

Comment on lines +1 to +4
first,second,third
1,one,true
2,two,false
3,three,true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Create static files that can be uploaded via the FileChooser

@hallieswan hallieswan marked this pull request as ready for review October 20, 2023 01:04
@hallieswan hallieswan self-assigned this Oct 20, 2023
Copy link
Contributor

@nickgros nickgros left a comment

Choose a reason for hiding this comment

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

Looks great!

@hallieswan hallieswan merged commit e2c13ee into Sage-Bionetworks:develop Oct 20, 2023
2 checks passed
@hallieswan hallieswan deleted the SWC-6556 branch October 20, 2023 17:21
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