-
Notifications
You must be signed in to change notification settings - Fork 13
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
Add e2e test for meditor validation #1865
Conversation
71035c7
to
8d4aa48
Compare
f5923ae
to
537af67
Compare
28f38da
to
58e1a41
Compare
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.
Approving this because it works and it's blocking other necessary efforts, but I left some questions and suggestions to follow up on later. Let's track those and get them all resolved soon!
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.
What is this file for?
/** | ||
* Read environment variables from file. | ||
* https://github.com/motdotla/dotenv | ||
*/ | ||
// require('dotenv').config(); |
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.
Should we strike this code block?
/* Reporter to use. See https://playwright.dev/docs/test-reporters */ | ||
reporter: 'html', |
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.
I believe we should be using either list
, line
, dot
, or github
when process.env.CI
is set (probably github
, though the docs don't fully explain what output you get when using that reporter).
/* Base URL to use in actions like `await page.goto('/')`. */ | ||
// baseURL: 'http://127.0.0.1:3000', |
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.
We should remove this or set it to http://127.0.0.1:8085
(if that's what's actually needed here).
/* Run your local dev server before starting the tests */ | ||
// webServer: { | ||
// command: 'npm run start', | ||
// url: 'http://127.0.0.1:3000', | ||
// reuseExistingServer: !process.env.CI, | ||
// }, |
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.
Probably get rid of this since we use a custom setup to launch the server and client, etc.
).toBeVisible(); | ||
}; | ||
|
||
const allDandisets = [ |
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.
I'm assuming all these Dandisets' metadatas are loaded into the database at some point?
await page.getByPlaceholder("Password").click(); | ||
await page.getByPlaceholder("Password").fill("password"); | ||
await page.getByRole("button", { name: "Sign In " }).click(); | ||
await page.waitForLoadState("networkidle"); |
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 is cool! However, the docs state
'networkidle' - DISCOURAGED wait until there are no network connections for at least 500 ms. Don't use this method for testing, rely on web assertions to assess readiness instead.
What's our justification for ignoring their (strongly worded) recommendation?
await page.getByLabel("Dandiset title").click(); | ||
await page.keyboard.press("End"); | ||
await page.keyboard.press("Space"); | ||
await page.keyboard.press("Tab"); | ||
await page.getByLabel("Dandiset title").click(); | ||
await page.keyboard.press("End"); | ||
await page.keyboard.press("Backspace"); | ||
await page.keyboard.press("Tab"); |
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.
Why does this part repeat?
await page.getByLabel("Dandiset title").click(); | ||
await page.keyboard.press("End"); | ||
await page.keyboard.press("Space"); | ||
await page.keyboard.press("Tab"); | ||
await page.getByLabel("Dandiset title").click(); | ||
await page.keyboard.press("End"); | ||
await page.keyboard.press("Backspace"); | ||
await page.keyboard.press("Tab"); | ||
await page.locator(".v-card__actions > button").first().click(); | ||
await page.waitForTimeout(3000); | ||
await page.waitForLoadState("networkidle"); |
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 needs a comment to explain what it's doing.
const validIcon = await page.locator(".v-card__actions > i"); | ||
const iconClass = await validIcon.getAttribute("class"); | ||
|
||
if (Object.keys(invalidDandisets).includes(dandisetId)) { |
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.
Can this just be Object.hasOwn(invalidDandisets, dandisetId)
?
"axios": "^1.6.7", | ||
"lodash": "^4.17.21" |
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.
Remove this
🚀 PR was released in |
This adds a new playwright-based E2E test that loads all the current dandisets on dandiarchive.org into the test DB, opens the meditor for each of them, and ensures the validation status is as expected depending on the current state of the metadata.
This will help us be confident that the changes in #1823 don't break anything.