-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 all 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 |
---|---|---|
|
@@ -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) | ||
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,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) | ||
}) |
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