-
Notifications
You must be signed in to change notification settings - Fork 887
test: improve E2E framework #9469
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
Conversation
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.
Some comments/suggestions but in general I think this looks good 👍🏻
@@ -4,7 +4,7 @@ import { STORAGE_STATE } from "./playwright.config" | |||
import { Language } from "../src/components/CreateUserForm/CreateUserForm" | |||
|
|||
test("create first user", async ({ page }) => { | |||
await page.goto("/", { waitUntil: "networkidle" }) | |||
await page.goto("/", { waitUntil: "domcontentloaded" }) |
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 don't think we should necessarily change networkidle
to domcontentloaded
without adding additional wait checks. For instance "wait until element X is present in the html". In a sense, networkidle
should be better as it will wait longer compared to domcontentloaded
which will fire immediately after dom even if network requests are still outbound.
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.
In previous PRs, I added extra "wait until" conditions, so in theory it should be faster and still safe with domcontentloaded
.
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.
Oh, that's perfect 👍🏻
@@ -28,13 +33,13 @@ test("web terminal", async ({ context, page }) => { | |||
], | |||
}) | |||
await createWorkspace(page, template) | |||
await startAgent(page, token) | |||
const agent = await startAgent(page, token) |
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 still have the unfortunate problem that any error in a test will terminate execution and the agent won't be cleaned up. Can we somehow make it a post-test cleanup hook that always runs and checks if an agent is running?
This isn't a big problem per-se but can cause trouble when running locally -> suddenly all tests fail after one failure. And also cause noise in test output.
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 was trying to implement an easy way but failed. The clue is to pass the PID of the process/group of process to the test.afterEach
. Something like "clean used resources", but not necessarily re-discover them.
This PR tweaks e2e tests to make bug investigation easier in the future.
Changes:
pprof
dumps as Github Action artifacts with custom reporternetworkidle
in testsstopAgent
withpkill
orkill
pprof
for the web server (coder server)