Skip to content

Conversation

@sheremet-va
Copy link
Member

Description

Closes #6082

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:.

@sheremet-va
Copy link
Member Author

@edoardocavazza I am actually not sure about having a global state - it leaks to other tests (I had to add .setup to some tests to not break them because previous tests were releasing buttons). What do you think?

@edoardocavazza
Copy link

I suppose that a "global state" actually exists anyway on the playwright/webdriver side.

Consider this test:

describe('try keyboard', () => {
    it('shift', async () => {
        await userEvent.keyboard('{Shift>}');
    });

    it('letter', async () => {
        const { key, shiftKey } = await new Promise((resolve) => {
            document.addEventListener('keydown', resolve);
            userEvent.keyboard('{a}');
        });
        expect(key).toBe('a');
        // this fails, because playwright keeps the shift key pressed from the previous test
        expect(shiftKey).toBe(false);
    });
});

A vitest global state should be useful to reset the keyboard state between tests.

@sheremet-va
Copy link
Member Author

sheremet-va commented Jul 11, 2024

Consider this test:

Shift will not be released in your example - not before, not after this PR, so I am not sure what it demonstrates 🤔 Are you saying you agree we should release everything after the test is completed?

@edoardocavazza
Copy link

Sorry, I may have misunderstood the question.
What I wanted to demonstrate with that test is that having a global state may still be necessary to clean the state and avoid leaks in subsequent tests.

@sheremet-va
Copy link
Member Author

Yes, I was saying in my previous question that I also found those leaks, but I am not sure what to do with them. Do we resolve them automatically? If you had more tests in between, or test.each it would be harder to find why shift was pressed, for example. Automatic cleanup might cause unexpected issues because it will call keyup handlers that might throw an error, for example.

@edoardocavazza
Copy link

Ok, now I understand your concerns. My opinion is that however we need to deviate slightly from the functioning of the original library. Automatic cleaning at the end of each test seems to me to be the most desired solution. It should be the developer's responsibility to make sure to clean up the test state by using afterEach and deleting added listeners. This is just my preference, but I am open to other solutions.

@sheremet-va sheremet-va force-pushed the fix/browser-support-setup branch from be15d61 to 5121921 Compare July 23, 2024 13:15
@netlify
Copy link

netlify bot commented Jul 23, 2024

Deploy Preview for vitest-dev ready!

Name Link
🔨 Latest commit 0c22a59
🔍 Latest deploy log https://app.netlify.com/sites/vitest-dev/deploys/669fb3ea5c269a000810faf2
😎 Deploy Preview https://deploy-preview-6088--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.

@sheremet-va sheremet-va merged commit 883f348 into vitest-dev:main Jul 23, 2024
@sheremet-va sheremet-va deleted the fix/browser-support-setup branch July 23, 2024 13:56
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 always releases pressed key with playwright provider

2 participants