Skip to content

Conversation

@hi-ogawa
Copy link
Contributor

@hi-ogawa hi-ogawa commented Oct 17, 2024

Description

I added userEvent.cleanup to reset keyboard state and hooked into tester cleanup runner onAfterRunTask.

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. If the feature is substantial or introduces breaking changes without a discussion, PR might be closed.
  • Ideally, include a test that fails without this PR but passes with it.
  • Please, don't make changes to pnpm-lock.yaml unless you introduce a new test example.

Tests

  • Run the tests with pnpm test:ci.

Documentation

  • If you introduce new functionality, document it. You can run documentation with pnpm run docs command.

Changesets

  • Changes in changelog are generated from PR name. Please, make sure that it explains your changes in an understandable manner. Please, prefix changeset messages with feat:, fix:, perf:, docs:, or chore:.

@netlify
Copy link

netlify bot commented Oct 17, 2024

Deploy Preview for vitest-dev ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit b23c8ae
🔍 Latest deploy log https://app.netlify.com/sites/vitest-dev/deploys/67159487dc4c840008bda622
😎 Deploy Preview https://deploy-preview-6731--vitest-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@hi-ogawa hi-ogawa marked this pull request as ready for review October 17, 2024 05:44
@hi-ogawa hi-ogawa changed the title fix(browser): cleanup keybaord release state fix(browser): cleanup keybaord state Oct 17, 2024
@sheremet-va sheremet-va changed the title fix(browser): cleanup keybaord state fix(browser): cleanup keyboard state Oct 17, 2024
@sheremet-va
Copy link
Member

sheremet-va commented Oct 17, 2024

Maybe for per-test isolation we can provide a userEvent inside the test context:

const test = base.extend({
  userEvent: async ({}, use) => {
    const user = userEvent.setup()
    await use(user)
    await user.cleanup()
  },
})

test('my test', ({ userEvent }) => {
 // ...
})

I don't think we can use .extend on our side because it would break this use case:

beforeEach(ctx => {
  ctx.dynamicAssigment = true
})

But we can maybe do it the same way we inject expect 🤔

@hi-ogawa
Copy link
Contributor Author

I didn't realize but I guess OP expected per-test isolation #6693 (comment) because vitest-browser-svelte provides automatic document/component cleanup from beforeEach, so ideally we should do the same for userEvent. Let me think about it.

@hi-ogawa hi-ogawa force-pushed the fix-browser-keyboard-up-cleanup branch from 85301a8 to 983ea44 Compare October 18, 2024 02:26
@hi-ogawa
Copy link
Contributor Author

I moved userEvent.cleanup to runner onAfterRunTask for test isolation. For @testing-library/user-event cleanup on preview provider, I need to tweak structure a bit.


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__).

* @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

@sheremet-va sheremet-va merged commit 19278f4 into vitest-dev:main Oct 21, 2024
17 checks passed
@sheremet-va
Copy link
Member

I added userEvent.cleanup to reset keyboard state and hooked into tester cleanup runner onAfterRunTask.

Hm, actually, our vitest-browser-* wrappers cleanup the state before the test runs, not after.

@hi-ogawa hi-ogawa deleted the fix-browser-keyboard-up-cleanup branch October 22, 2024 00:25
@hi-ogawa
Copy link
Contributor Author

hi-ogawa commented Oct 22, 2024

Good point. I thought it's trivial to move it to onBeforeTryTask, but just realized it's difficult for playwright since we need to reset keyboard based on previous userEvent instance before moving to next test file.

Btw, I thought I was using onAfterTryTask but realized it's onAfterRunTask. I should probably update this one at least to simulate afterEach like behavior.

renovate bot added a commit to mmkal/eslint-plugin-mmkal that referenced this pull request Oct 28, 2024
##### [v2.1.4](https://github.com/vitest-dev/vitest/releases/tag/v2.1.4)

#####    🚀 Features

-   **browser**: Allow custom HTML path, respect plugins `transformIndexHtml`  -  by [@sheremet-va](https://github.com/sheremet-va) in vitest-dev/vitest#6725 [<samp>(16902)</samp>](vitest-dev/vitest@169028f0)

#####    🐞 Bug Fixes

-   Don't normalize drive case letter in root  -  by [@sheremet-va](https://github.com/sheremet-va) in vitest-dev/vitest#6792 [<samp>(b28cd)</samp>](vitest-dev/vitest@b28cd2e3)
-   **browser**:
    -   Fix default browser port  -  by [@hi-ogawa](https://github.com/hi-ogawa) in vitest-dev/vitest#6700 [<samp>(9c518)</samp>](vitest-dev/vitest@9c518c14)
    -   Optimize expect-type  -  by [@sheremet-va](https://github.com/sheremet-va) in vitest-dev/vitest#6713 [<samp>(07918)</samp>](vitest-dev/vitest@07918538)
    -   Don't polyfill process.env  -  by [@sheremet-va](https://github.com/sheremet-va) in vitest-dev/vitest#6718 [<samp>(da6d2)</samp>](vitest-dev/vitest@da6d2ea7)
    -   Increment browser port automatically if there are several projects with browser.enabled  -  by [@sheremet-va](https://github.com/sheremet-va) in vitest-dev/vitest#6717 [<samp>(a9397)</samp>](vitest-dev/vitest@a939779f)
    -   Cleanup keyboard state  -  by [@hi-ogawa](https://github.com/hi-ogawa) in vitest-dev/vitest#6731 [<samp>(19278)</samp>](vitest-dev/vitest@19278f4c)
    -   Don't add `v=` queries to setup files imports  -  by [@sheremet-va](https://github.com/sheremet-va) in vitest-dev/vitest#6759 [<samp>(b8258)</samp>](vitest-dev/vitest@b82584c9)
    -   User event cleanup on retry  -  by [@hi-ogawa](https://github.com/hi-ogawa) in vitest-dev/vitest#6764 [<samp>(bdd15)</samp>](vitest-dev/vitest@bdd15dd1)
    -   Ignore non mocked msw requests  -  by [@sheremet-va](https://github.com/sheremet-va) in vitest-dev/vitest#6770 [<samp>(9d9ba)</samp>](vitest-dev/vitest@9d9bad5b)
    -   Initiate MSW in the same frame as tests  -  by [@sheremet-va](https://github.com/sheremet-va) in vitest-dev/vitest#6772 [<samp>(2444f)</samp>](vitest-dev/vitest@2444ff22)
-   **deps**:
    -   Update dependency sirv to v3  -  in vitest-dev/vitest#6701 [<samp>(fde5d)</samp>](vitest-dev/vitest@fde5d509)
-   **expect**:
    -   Correct behavior of `toThrowError` with empty string parameter  -  by [@shulaoda](https://github.com/shulaoda) in vitest-dev/vitest#6710 [<samp>(a6129)</samp>](vitest-dev/vitest@a61293e9)
-   **mocker**:
    -   Remove spy from peer dependencies  -  by [@sheremet-va](https://github.com/sheremet-va) in vitest-dev/vitest#6777 [<samp>(3a8b5)</samp>](vitest-dev/vitest@3a8b56bf)
-   **vitest**:
    -   Clarify slowTestThreshold, print slow tests in non-TTY mode  -  by [@sheremet-va](https://github.com/sheremet-va) in vitest-dev/vitest#6715 [<samp>(2e6aa)</samp>](vitest-dev/vitest@2e6aa647)
    -   Print warnings form Vite plugins  -  by [@sheremet-va](https://github.com/sheremet-va) in vitest-dev/vitest#6724 [<samp>(121b1)</samp>](vitest-dev/vitest@121b161f)
    -   Don't fail if the working directory starts with a lowercase drive letter  -  by [@sheremet-va](https://github.com/sheremet-va) in vitest-dev/vitest#6779 [<samp>(df6d7)</samp>](vitest-dev/vitest@df6d750b)
    -   Silence import analysis warning  -  by [@hi-ogawa](https://github.com/hi-ogawa) in vitest-dev/vitest#6785 [<samp>(39041)</samp>](vitest-dev/vitest@39041ee5)
-   **vitest,runner**:
    -   Simplify `test.extend` type exports  -  by [@hi-ogawa](https://github.com/hi-ogawa) in vitest-dev/vitest#6707 [<samp>(e5c38)</samp>](vitest-dev/vitest@e5c388f0)

#####    🏎 Performance

-   Use `hash` to replace `createHash`  -  by [@btea](https://github.com/btea) in vitest-dev/vitest#6703 [<samp>(5d07b)</samp>](vitest-dev/vitest@5d07bba6)

#####     [View changes on GitHub](vitest-dev/vitest@v2.1.3...v2.1.4)
spamshaker pushed a commit to spamshaker/vitest that referenced this pull request Jun 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

userEvent.keyboard doesn't release pressed key with playwright provider

2 participants