-
-
Notifications
You must be signed in to change notification settings - Fork 8.9k
test(vapor): use browser mode instead of pupeteer to run tests #13934
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
base: minor
Are you sure you want to change the base?
test(vapor): use browser mode instead of pupeteer to run tests #13934
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| test('vapor', { timeout: E2E_TIMEOUT }, async () => { | ||
| createVaporApp(App).mount('#app') | ||
|
|
||
| expect(css('.main')).not.toBeVisible() |
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.
Technically it's always better to use expect.element(locator).not.toBeVisible(), but prettier changes this 1 line into 3, so I kept more error prone version for esthetic reasons. I wonder if we can improve this on Vitest side.
| .use(sirv(path.resolve(import.meta.dirname, '../dist'))) | ||
| .listen(port) | ||
| process.on('SIGTERM', () => server && server.close()) | ||
| test('vapor', { timeout: E2E_TIMEOUT }, async () => { |
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.
Putting timeout here makes prettier format the test body with indentation of 2 instead of 4, although maybe it's even better to move the timeout to the config.
| }, | ||
| { | ||
| extends: true, | ||
| extends: './packages-private/vapor-e2e-test/vite.config.ts', |
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.
The test config reuses the Vite config from a private repo instead of the main one (so, aliases and defines are not actually inherited, but tests seem to run ok...)
Size ReportBundles
Usages
|
@vue/compiler-core
@vue/compiler-dom
@vue/compiler-sfc
@vue/compiler-ssr
@vue/compiler-vapor
@vue/reactivity
@vue/runtime-core
@vue/runtime-dom
@vue/runtime-vapor
@vue/server-renderer
@vue/shared
vue
@vue/compat
commit: |
|
I've actually been planning to switch e2e tests to vitest browser mode too, but I haven't done it yet since vitest browser mode is still experimental. I have a question: after switching to vitest browser mode, does the e2e test execution time increase or decrease? If the difference isn't too significant, I think this PR is worthwhile. |
The time is unpredictable with the previous approach for me, the first time it's ~2s, the second time ~1,5s. The browser mode runs for 2s (the MVC test is always 1,5s unlike with pupeteer where it's either 1,5s or 700ms). I think it might be because the previous setup was modifying the DOM directly (by setting |
|
@sheremet-va |
|
I will come back to this PR at some point. @edison1105 right now I am struggling with running unit tests in Vitest beta. It seems like |
|
@sheremet-va |
| export function nextFrame() { | ||
| // this page is not same as Playwright's page | ||
| // how to wait for the next frame? | ||
| return page.evaluate(() => { |
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.
@sheremet-va
I merged the latest code of minor into the current branch, but I encountered a problem and don't know how to solve it. this page is not same as Playwright's page. how to wait for the next frame? I couldn't find a solution in the vitest documentation.
| ) | ||
| // comp leave | ||
| await css(btnSelector).click() | ||
| expect(css(containerSelector).element().innerHTML).toBe( |
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.
Accessing element() is also not a good idea, checking toContainHTML is more resilient
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.
I think it's better to leave the remaining test case refactoring work to you. I'll go and handle other PRS first.
|
The main issue I am having with VItest 4 is that unit tests are failing. Did you have a loot at them yet, @edison1105? |
// in vitest 3.2.4
const hook = vi.fn((el, done) => {})
test('hook length', () => {
expect(hook.length).toBe(2) // true
})
// in vitest 4.x
const hook = vi.fn((el, done) => {})
test('hook length', () => {
expect(hook.length).toBe(2) // should be 2 but got 0
})The above difference is the cause of the core/packages/runtime-core/src/components/BaseTransition.ts Lines 360 to 371 in 45547e6
Another test case (componentPublicInstance.spec.ts:348:28) failed. Simply change the 4 to 3. Refer to the comments in L344. (It equals 3 in jest) - You already fixed it via 7e56f8d. core/packages/runtime-core/__tests__/componentPublicInstance.spec.ts Lines 344 to 348 in 45547e6
|
That one is an expected breaking change, yeah.
Thank you so much for investigating, this is indeed a bug in Vitest! |
This PR rewrites the tests for Vue Vapor mode to use Vitest Browser Mode. The logic of the test stayed the same, although it maybe also good to rewrite some of it or split it into separate tests. There are no big noticeable changes in speed.
Happy to get any feedback.
If there is desire, I can also update older e2e tests.
Running tests with
--browser.headless=falsealso shows Vitest UI: