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 3 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
14 changes: 13 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,23 @@ export async function setupExpectDom() {

const isNot = chai.util.flag(this, 'negate') as boolean
const name = chai.util.flag(this, '_name') as string
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)
}
}
29 changes: 17 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,24 @@ 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(async () => {
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 not make it 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.

i need to be able to await the check call. do you want me to use Promise.then instead?
can do it NP, just want to make sure we are aligned

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I meant using promise.then.catch

clearTimeout(intervalId)
chai.util.flag(assertion, '_isLastPollAttempt', true)
await check()
sheremet-va marked this conversation as resolved.
Show resolved Hide resolved
reject(
copyStackTrace(
new Error(`Matcher did not succeed in ${timeout}ms`, {
cause: lastError,
}),
STACK_TRACE_ERROR,
),
)
}, timeout)
check()
})
let awaited = false
Expand Down
4 changes: 2 additions & 2 deletions test/browser/specs/runner.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@ describe('running browser tests', async () => {
console.error(stderr)
})

expect(browserResultJson.testResults).toHaveLength(19 * instances.length)
expect(passedTests).toHaveLength(17 * instances.length)
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
26 changes: 26 additions & 0 deletions test/browser/test/expect-element.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
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.fn(locator.element)
locator.element = locatorElementMock
Copy link
Member

Choose a reason for hiding this comment

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

you can just do vi.spyOn()

const locatorQueryMock = vi.fn(locator.query)
locator.query = locatorQueryMock

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

expect(locatorElementMock).toBeCalledTimes(1)
expect(locatorElementMock).toHaveBeenCalledAfter(locatorQueryMock)
})
15 changes: 14 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,16 @@ 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)
})
})
Loading