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

Vitest: use browser mode #3222

Merged
merged 82 commits into from
Jul 16, 2024
Merged

Vitest: use browser mode #3222

merged 82 commits into from
Jul 16, 2024

Conversation

amanmahajan7
Copy link
Contributor

@amanmahajan7 amanmahajan7 commented May 8, 2023

https://vitest.dev/guide/browser.html
https://vitest.dev/guide/browser/interactivity-api.html

I think we should check all the tests if we can improve them now that they are running in a real browser.

@amanmahajan7 amanmahajan7 self-assigned this May 8, 2023
const resizeHandle = frame.locator('[role="columnheader"][aria-colindex="2"] div');
const { x, y } = (await resizeHandle.boundingBox())!;
await resizeHandle.hover({
position: { x: 5, y: 5 }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to use a specific position otherwise the resized width is incorrect

@@ -13,7 +28,8 @@ export default defineConfig({
},
resolve: {
alias: {
lodash: 'lodash-es'
lodash: isTest ? 'lodash' : 'lodash-es',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this needed?

Copy link
Contributor

Choose a reason for hiding this comment

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

@testing-library/jest-dom installs lodash, can't hurt to keep it.

@amanmahajan7 amanmahajan7 marked this pull request as ready for review July 15, 2024 16:10
test/virtualization.test.ts Show resolved Hide resolved
test/virtualization.test.ts Show resolved Hide resolved
test/virtualization.test.ts Outdated Show resolved Hide resolved
test/utils.tsx Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
test/rowHeight.test.ts Outdated Show resolved Hide resolved
test/rowHeight.test.ts Show resolved Hide resolved
test/column/draggable.test.ts Outdated Show resolved Hide resolved
expect(screen.getByRole('spinbutton', { name: 'col1-editor' })).toHaveValue(1230);
const spy = vi.spyOn(window.HTMLElement.prototype, 'scrollIntoView');
await userEvent.keyboard('{enter}');
expect(spy).toHaveBeenCalled();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did we remove these checks?

expect(screen.getByRole('spinbutton', { name: 'col1-editor' })).toHaveValue(1230);

this checked that when typing on a cell, it opens the editor and the editor receives focus, and we continue typing in the editor

    const spy = vi.spyOn(window.HTMLElement.prototype, 'scrollIntoView');
    await userEvent.keyboard('{enter}');
    expect(spy).toHaveBeenCalled();

this checked that we scrolled back to the cell even if we had scrolled past it (see await scrollGrid({ scrollTop: 2000 }); above)

Maybe we need more comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it is better to check the actual row in the view instead of spying on scrollIntoView

expect(getCellsAtRowIndex(0)).toHaveLength(2);
expect(getRows()[1]).toHaveTextContent('11');
const editor = screen.getByRole('spinbutton', { name: 'col1-editor' });
expect(editor).toHaveValue(1230);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure what the new checks are for

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Basically we select the first cell and then scroll to the bottom. When we enable the editor (userEvent.keyboard('123');) then the grid should scroll back to the top so I am checking
expect(getRows()[1]).toHaveTextContent('11');

Copy link
Contributor

@nstepien nstepien Jul 16, 2024

Choose a reason for hiding this comment

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

getRows()[1] gets the second rendered row, no? Not necessarily the true "second" row.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct. Is it not enough to check the second rendered row? We just want to ensure the editor is visible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

May be we can use something like toBeVisible 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we can just check grid.scrollTop

vite.config.ts Outdated Show resolved Hide resolved
vite.config.ts Outdated Show resolved Hide resolved
nstepien
nstepien previously approved these changes Jul 16, 2024
test/column/renderEditCell.test.tsx Show resolved Hide resolved
expect(getGrid()).toHaveStyle({ gridTemplateColumns: '100px 327.703px' });
});

test('should use the maxWidth if specified on auto resize', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could also test minWidth

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will add it in the nest PR
#3547

@amanmahajan7 amanmahajan7 enabled auto-merge (squash) July 16, 2024 14:49
@amanmahajan7 amanmahajan7 merged commit 874562c into main Jul 16, 2024
2 checks passed
@amanmahajan7 amanmahajan7 deleted the am-vitest-browser branch July 16, 2024 14:52
adityatoshniwal pushed a commit to pgadmin-org/react-data-grid that referenced this pull request Jul 31, 2024
Using the new browser mode to run test in a real browser
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.

2 participants