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

fix: only use locator.element on last expect.element attempt (fix #7139) #7152

Merged
merged 7 commits into from
Jan 3, 2025
Merged
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
17 changes: 16 additions & 1 deletion packages/browser/src/client/tester/expect-element.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,26 @@ export async function setupExpectDom() {

const isNot = chai.util.flag(this, 'negate') as boolean
const name = chai.util.flag(this, '_name') as string
// element selector uses prettyDOM under the hood, which is an expensive call
// that should not be called on each failed locator attempt to avoid memory leak:
// https://github.com/vitest-dev/vitest/issues/7139
const isLastPollAttempt = chai.util.flag(this, '_isLastPollAttempt')
Copy link
Member

Choose a reason for hiding this comment

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

let's also add a comment why we are doing this with a link to an issue

// special case for `toBeInTheDocument` matcher
if (isNot && name === 'toBeInTheDocument') {
return elementOrLocator.query()
}
return elementOrLocator.element()

if (isLastPollAttempt) {
return elementOrLocator.element()
}

const result = elementOrLocator.query()

if (!result) {
throw new Error(`Cannot find element with locator: ${JSON.stringify(elementOrLocator)}`)
}

return result
}, options)
}
}
33 changes: 21 additions & 12 deletions packages/vitest/src/integrations/chai/poll.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,19 +66,9 @@ export function createExpectPoll(expect: ExpectStatic): ExpectStatic['poll'] {
const STACK_TRACE_ERROR = new Error('STACK_TRACE_ERROR')
const promise = () => new Promise<void>((resolve, reject) => {
let intervalId: any
let timeoutId: any
let lastError: any
const { setTimeout, clearTimeout } = getSafeTimers()
const timeoutId = setTimeout(() => {
clearTimeout(intervalId)
reject(
copyStackTrace(
new Error(`Matcher did not succeed in ${timeout}ms`, {
cause: lastError,
}),
STACK_TRACE_ERROR,
),
)
}, timeout)
const check = async () => {
try {
chai.util.flag(assertion, '_name', key)
Expand All @@ -90,9 +80,28 @@ export function createExpectPoll(expect: ExpectStatic): ExpectStatic['poll'] {
}
catch (err) {
lastError = err
intervalId = setTimeout(check, interval)
if (!chai.util.flag(assertion, '_isLastPollAttempt')) {
intervalId = setTimeout(check, interval)
}
}
}
timeoutId = setTimeout(() => {
clearTimeout(intervalId)
chai.util.flag(assertion, '_isLastPollAttempt', true)
const rejectWithCause = (cause: any) => {
reject(
copyStackTrace(
new Error(`Matcher did not succeed in ${timeout}ms`, {
cause,
}),
STACK_TRACE_ERROR,
),
)
}
check()
.then(() => rejectWithCause(lastError))
.catch(e => rejectWithCause(e))
}, timeout)
check()
})
let awaited = false
Expand Down
7 changes: 5 additions & 2 deletions test/browser/specs/runner.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,11 @@ describe('running browser tests', async () => {
console.error(stderr)
})

expect(browserResultJson.testResults).toHaveLength(19 * instances.length)
expect(passedTests).toHaveLength(17 * instances.length)
// This should match the number of actual tests from browser.json
// if you added new tests, these assertion will fail and you should
// update the numbers
expect(browserResultJson.testResults).toHaveLength(20 * instances.length)
Copy link
Contributor Author

@tsirlucas tsirlucas Dec 31, 2024

Choose a reason for hiding this comment

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

is this correct? i assumed i needed to bump those since i added new test cases

if thats the case, maybe its a good idea to leave a comment here saying its fine to bump it if you added new tests... i can do it in scope of this PR if you want

Copy link
Member

Choose a reason for hiding this comment

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

Yes, you are correct. Feel free to add a comment

expect(passedTests).toHaveLength(18 * instances.length)
expect(failedTests).toHaveLength(2 * instances.length)

expect(stderr).not.toContain('optimized dependencies changed')
Expand Down
24 changes: 24 additions & 0 deletions test/browser/test/expect-element.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import { page } from '@vitest/browser/context'
import { expect, test, vi } from 'vitest'

// element selector uses prettyDOM under the hood, which is an expensive call
// that should not be called on each failed locator attempt to avoid memory leak:
// https://github.com/vitest-dev/vitest/issues/7139
test('should only use element selector on last expect.element attempt', async () => {
const div = document.createElement('div')
const spanString = '<span>test</span>'
div.innerHTML = spanString
document.body.append(div)

const locator = page.getByText('non-existent')
const locatorElementMock = vi.spyOn(locator, 'element')
const locatorQueryMock = vi.spyOn(locator, 'query')

try {
await expect.element(locator, { timeout: 500, interval: 100 }).toBeInTheDocument()
}
catch {}

expect(locatorElementMock).toBeCalledTimes(1)
expect(locatorElementMock).toHaveBeenCalledAfter(locatorQueryMock)
})
43 changes: 42 additions & 1 deletion test/core/test/expect-poll.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { expect, test, vi } from 'vitest'
import { chai, expect, test, vi } from 'vitest'

test('simple usage', async () => {
await expect.poll(() => false).toBe(false)
Expand Down Expand Up @@ -106,3 +106,44 @@ test('toBeDefined', async () => {
}),
}))
})

test('should set _isLastPollAttempt flag on last call', async () => {
const fn = vi.fn(function (this: object) {
return chai.util.flag(this, '_isLastPollAttempt')
})
await expect(async () => {
await expect.poll(fn, { interval: 100, timeout: 500 }).toBe(false)
}).rejects.toThrowError()
fn.mock.results.forEach((result, index) => {
const isLastCall = index === fn.mock.results.length - 1
expect(result.value).toBe(isLastCall ? true : undefined)
})
})

test('should handle success on last attempt', async () => {
const fn = vi.fn(function (this: object) {
if (chai.util.flag(this, '_isLastPollAttempt')) {
return 1
}
return undefined
})
await expect.poll(fn, { interval: 100, timeout: 500 }).toBe(1)
})

test('should handle failure on last attempt', async () => {
const fn = vi.fn(function (this: object) {
if (chai.util.flag(this, '_isLastPollAttempt')) {
return 3
}
return 2
})
await expect(async () => {
await expect.poll(fn, { interval: 10, timeout: 100 }).toBe(1)
}).rejects.toThrowError(expect.objectContaining({
message: 'Matcher did not succeed in 100ms',
cause: expect.objectContaining({
// makes sure cause message reflects the last attempt value
message: 'expected 3 to be 1 // Object.is equality',
}),
}))
})
Loading