-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Changes from 3 commits
c35f89d
2cc9d40
03458b5
db53172
1a3f861
4ade736
e1fb0d9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
|
@@ -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 () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. let's not make it async There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i need to be able to await the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I meant using |
||
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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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') | ||
|
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you can just do |
||
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) | ||
}) |
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.
let's also add a comment why we are doing this with a link to an issue