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(browser): cleanup keyboard state #6731

Merged
merged 15 commits into from
Oct 21, 2024
1 change: 1 addition & 0 deletions packages/browser/context.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ export interface UserEvent {
* @see {@link https://vitest.dev/guide/browser/interactivity-api.html#userevent-setup}
*/
setup: () => UserEvent
cleanup: () => Promise<void>;
Copy link
Member

Choose a reason for hiding this comment

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

We need documentation for every public API - in jsdoc and vitest.dev

/**
* Click on an element. Uses provider's API under the hood and supports all its options.
* @see {@link https://playwright.dev/docs/api/class-locator#locator-click} Playwright API
Expand Down
11 changes: 10 additions & 1 deletion packages/browser/src/client/tester/context.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@ function triggerCommand<T>(command: string, ...args: any[]) {
return rpc().triggerCommand<T>(contextId, command, filepath(), args)
}

export function createUserEvent(__tl_user_event__?: TestingLibraryUserEvent): UserEvent {
export function createUserEvent(__tl_user_event_base__?: TestingLibraryUserEvent): UserEvent {
let __tl_user_event__ = __tl_user_event_base__?.setup({})
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why this is needed; the first argument is supposed to be called with setup() - see the setup method on L40

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if this what you're asking, but we need to pass around the original userEvent since this happens:

import { userEvent } from "@testing-library/user-event"

const userEvent1 = userEvent.setup();
userEvent1.keyboard("{Alt>}")

// userEvent1 and userEvent2 share the same keyboard state 
const userEvent2 = userEvent1.setup();
userEvent2.keyboard("{Tab}") // altKey = true


// need to call original `userEvent.setup()` to have a fresh state
{
  const userEvent1 = userEvent.setup();
  userEvent1.keyboard("{Alt>}")
  
  const userEvent2 = userEvent.setup();
  userEvent2.keyboard("{Tab}") // altKey = false
}

For L40, we should probably recurse it like createUserEvent(__tl_user_event_base__).

const keyboard = {
unreleased: [] as string[],
}
Expand All @@ -38,6 +39,14 @@ export function createUserEvent(__tl_user_event__?: TestingLibraryUserEvent): Us
setup(options?: any) {
return createUserEvent(__tl_user_event__?.setup(options))
},
async cleanup() {
if (typeof __tl_user_event__ !== 'undefined') {
__tl_user_event__ = __tl_user_event_base__?.setup({})
return
}
await triggerCommand('__vitest_cleanup', keyboard)
keyboard.unreleased = []
},
click(element: Element | Locator, options: UserEventClickOptions = {}) {
return convertToLocator(element).click(processClickOptions(options))
},
Expand Down
3 changes: 2 additions & 1 deletion packages/browser/src/client/tester/runner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import type { VitestExecutor } from 'vitest/execute'
import { NodeBenchmarkRunner, VitestTestRunner } from 'vitest/runners'
import { loadDiffConfig, loadSnapshotSerializers, takeCoverageInsideWorker } from 'vitest/browser'
import { TraceMap, originalPositionFor } from 'vitest/utils'
import { page } from '@vitest/browser/context'
import { page, userEvent } from '@vitest/browser/context'
import { globalChannel } from '@vitest/browser/client'
import { executor } from '../utils'
import { VitestBrowserSnapshotEnvironment } from './snapshot'
Expand Down Expand Up @@ -41,6 +41,7 @@ export function createBrowserRunner(
}

onAfterRunTask = async (task: Task) => {
await userEvent.cleanup()
await super.onAfterRunTask?.(task)

if (this.config.bail && task.result?.state === 'fail') {
Expand Down
3 changes: 2 additions & 1 deletion packages/browser/src/node/commands/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { clear } from './clear'
import { fill } from './fill'
import { selectOptions } from './select'
import { tab } from './tab'
import { keyboard } from './keyboard'
import { keyboard, keyboardCleanup } from './keyboard'
import { dragAndDrop } from './dragAndDrop'
import { hover } from './hover'
import { upload } from './upload'
Expand Down Expand Up @@ -34,4 +34,5 @@ export default {
__vitest_selectOptions: selectOptions,
__vitest_dragAndDrop: dragAndDrop,
__vitest_hover: hover,
__vitest_cleanup: keyboardCleanup,
}
20 changes: 20 additions & 0 deletions packages/browser/src/node/commands/keyboard.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,26 @@ export const keyboard: UserEventCommand<(text: string, state: KeyboardState) =>
}
}

export const keyboardCleanup: UserEventCommand<(state: KeyboardState) => Promise<void>> = async (
context,
state,
) => {
const { provider, contextId } = context
if (provider instanceof PlaywrightBrowserProvider) {
const page = provider.getPage(contextId)
for (const key of state.unreleased) {
await page.keyboard.up(key)
}
}
else if (provider instanceof WebdriverBrowserProvider) {
const keyboard = provider.browser!.action('key')
for (const key of state.unreleased) {
keyboard.up(key)
}
await keyboard.perform()
}
}
hi-ogawa marked this conversation as resolved.
Show resolved Hide resolved

export async function keyboardImplementation(
pressed: Set<string>,
provider: BrowserProvider,
Expand Down
7 changes: 4 additions & 3 deletions packages/browser/src/node/plugins/pluginContext.ts
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,8 @@ async function getUserEventImport(provider: BrowserProvider, resolve: (id: strin
if (!resolved) {
throw new Error(`Failed to resolve user-event package from ${__dirname}`)
}
return `import { userEvent as __vitest_user_event__ } from '${slash(
`/@fs/${resolved.id}`,
)}'\nconst _userEventSetup = __vitest_user_event__.setup()\n`
return `\
import { userEvent as __vitest_user_event__ } from '${slash(`/@fs/${resolved.id}`)}'
const _userEventSetup = __vitest_user_event__
`
}
55 changes: 55 additions & 0 deletions test/browser/fixtures/user-event/cleanup1.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
import { expect, onTestFinished, test } from 'vitest'
import { userEvent } from '@vitest/browser/context'

test('cleanup1', async () => {
let logs: any[] = [];
function handler(e: KeyboardEvent) {
logs.push([e.key, e.altKey]);
};
document.addEventListener('keydown', handler)
onTestFinished(() => {
document.removeEventListener('keydown', handler);
})

await userEvent.keyboard('{Tab}')
await userEvent.keyboard("{Alt>}")
expect(logs).toMatchInlineSnapshot(`
[
[
"Tab",
false,
],
[
"Alt",
true,
],
]
`)
})

// test per-test cleanup
test('cleanup1.2', async () => {
let logs: any[] = [];
function handler(e: KeyboardEvent) {
logs.push([e.key, e.altKey]);
};
document.addEventListener('keydown', handler)
onTestFinished(() => {
document.removeEventListener('keydown', handler);
})

await userEvent.keyboard('{Tab}')
await userEvent.keyboard("{Alt>}")
expect(logs).toMatchInlineSnapshot(`
[
[
"Tab",
false,
],
[
"Alt",
true,
],
]
`)
})
30 changes: 30 additions & 0 deletions test/browser/fixtures/user-event/cleanup2.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
import { expect, onTestFinished, test } from 'vitest'
import { userEvent } from '@vitest/browser/context'

// test per-test-file cleanup just in case

test('cleanup2', async () => {
let logs: any[] = [];
function handler(e: KeyboardEvent) {
logs.push([e.key, e.altKey]);
};
document.addEventListener('keydown', handler)
onTestFinished(() => {
document.removeEventListener('keydown', handler);
})

await userEvent.keyboard('{Tab}')
await userEvent.keyboard("{Alt>}")
expect(logs).toMatchInlineSnapshot(`
[
[
"Tab",
false,
],
[
"Alt",
true,
],
]
`)
})
17 changes: 17 additions & 0 deletions test/browser/fixtures/user-event/vitest.config.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import { fileURLToPath } from 'node:url'
import { defineConfig } from 'vitest/config'

const provider = process.env.PROVIDER || 'playwright'
const name =
process.env.BROWSER || (provider === 'playwright' ? 'chromium' : 'chrome')

export default defineConfig({
cacheDir: fileURLToPath(new URL("./node_modules/.vite", import.meta.url)),
test: {
browser: {
enabled: true,
provider,
name,
},
},
})
12 changes: 12 additions & 0 deletions test/browser/specs/runner.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -137,3 +137,15 @@ error with a stack
expect(stderr).toContain('Access denied to "/inaccesible/path".')
})
})

test('user-event', async () => {
const { ctx } = await runBrowserTests({
root: './fixtures/user-event',
})
expect(Object.fromEntries(ctx.state.getFiles().map(f => [f.name, f.result.state]))).toMatchInlineSnapshot(`
{
"cleanup1.test.ts": "pass",
"cleanup2.test.ts": "pass",
}
`)
})
Loading