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

Better testing timeouts for less painful development #1317

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions playwright.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ const config = {
use: {
...devices["Desktop Chrome"],
contextOptions: {
timeout: 60000
timeout: 10000
},
hasTouch: true
}
Expand All @@ -17,20 +17,21 @@ const config = {
use: {
...devices["Desktop Firefox"],
contextOptions: {
timeout: 60000
timeout: 10000
},
hasTouch: true
}
}
],
browserStartTimeout: 60000,
timeout: 10000,
browserStartTimeout: 10000,
retries: 2,
testDir: "./src/tests/",
testMatch: /(functional|integration)\/.*_tests\.js/,
webServer: {
command: "yarn start",
url: "http://localhost:9000/src/tests/fixtures/test.js",
timeout: 120 * 1000,
timeout: 10000,
// eslint-disable-next-line no-undef
reuseExistingServer: !process.env.CI
},
Expand Down
2 changes: 1 addition & 1 deletion src/tests/functional/rendering_tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -322,7 +322,7 @@ test("waits for some time, but renders if CSS takes too much to load", async ({
})

await page.click("#additional-assets-link")
await nextEventNamed(page, "turbo:render")
await nextEventNamed(page, "turbo:render", {}, 5000)

assert.equal(await page.textContent("h1"), "Additional assets")
assert.equal(await isStylesheetEvaluated(page), false)
Expand Down
24 changes: 20 additions & 4 deletions src/tests/helpers/page.js
Copy link
Contributor

Choose a reason for hiding this comment

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

Changing the global timeout is definitely an improvement. I'm skeptical that making event reading timeouts configurable is worthwhile. Adding a fourth positional argument to the next-prefixed methods awkward, and I think it's less likely to require one-off re-configurations.

Instead, what do you think about adding a 2000ms timeout directly to the readEventLogs and readBodyMutationLogs functions? If that works, maybe it'd be worthwhile to source the 2000ms value from a global configuration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@seanpdoyle Agreed on all counts.

The wrench in the gears is that this specific test is doing a weird waiting thing: "waits for some time, but renders if CSS takes too much to load". So it needs a specifically longer timeout just for this one event reading, so it looks to me like we have to make it configurable at this level.

Honestly, I think this whole nextEventNamed noNextEventNamed system needs to be reworked into explicit assertions, e.g. assert.eventNamed. The more I look at it, the more problems I see with it. The noNextEventNamed doesn't wait at all, so I'm seeing false positives when I KNOW that the event is later thrown. This is probably okay if you are careful to only call it after a positive event assertion, but it seems like a footgun at the very least.

Original file line number Diff line number Diff line change
Expand Up @@ -96,9 +96,13 @@ export async function nextPageRefresh(page, timeout = 500) {
return sleep(pageRefreshDebouncePeriod + timeout)
}

export async function nextEventNamed(page, eventName, expectedDetail = {}) {
export async function nextEventNamed(page, eventName, expectedDetail = {}, timeout = 2000) {
let record
const startTime = new Date()
while (!record) {
if (new Date() - startTime > timeout) {
throw new Error(`Event ${eventName} with ${JSON.stringify(expectedDetail)} wasn't dispatched within ${timeout}ms`)
}
const records = await readEventLogs(page, 1)
record = records.find(([name, detail]) => {
return name == eventName && Object.entries(expectedDetail).every(([key, value]) => detail[key] === value)
Expand All @@ -107,9 +111,13 @@ export async function nextEventNamed(page, eventName, expectedDetail = {}) {
return record[1]
}

export async function nextEventOnTarget(page, elementId, eventName) {
export async function nextEventOnTarget(page, elementId, eventName, timeout = 2000) {
let record
const startTime = new Date()
while (!record) {
if (new Date() - startTime > timeout) {
throw new Error(`Element ${elementId} didn't dispatch event ${eventName} within ${timeout}ms`)
}
const records = await readEventLogs(page, 1)
record = records.find(([name, _, id]) => name == eventName && id == elementId)
}
Expand All @@ -128,9 +136,13 @@ export async function listenForEventOnTarget(page, elementId, eventName) {
}, eventName)
}

export async function nextBodyMutation(page) {
export async function nextBodyMutation(page, timeout = 2000) {
let record
const startTime = new Date()
while (!record) {
if (new Date() - startTime > timeout) {
throw new Error(`body mutation didn't occur within ${timeout}ms`)
}
[record] = await readBodyMutationLogs(page, 1)
}
return record[0]
Expand All @@ -141,9 +153,13 @@ export async function noNextBodyMutation(page) {
return !records.some((record) => !!record)
}

export async function nextAttributeMutationNamed(page, elementId, attributeName) {
export async function nextAttributeMutationNamed(page, elementId, attributeName, timeout = 2000) {
let record
const startTime = new Date()
while (!record) {
if (new Date() - startTime > timeout) {
throw new Error(`Element ${elementId}'s ${attributeName} attribute mutation didn't occur within ${timeout}ms`)
}
const records = await readMutationLogs(page, 1)
record = records.find(([name, id]) => name == attributeName && id == elementId)
}
Expand Down
Loading