-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
feat: support async act #10
Open
MatanBobi
wants to merge
4
commits into
vitest-dev:main
Choose a base branch
from
MatanBobi:fix/support-async-act
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,7 @@ | ||
export function HelloWorld(): React.ReactElement { | ||
return <div>Hello World</div> | ||
export function HelloWorld({ | ||
name = 'World', | ||
}: { | ||
name?: string | ||
}): React.ReactElement { | ||
return <div>{`Hello ${name}`}</div> | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,19 +1,29 @@ | ||
import { Suspense } from 'react' | ||
import { expect, test } from 'vitest' | ||
import { page } from '@vitest/browser/context' | ||
import { render } from '../src/index' | ||
import { HelloWorld } from './fixtures/HelloWorld' | ||
import { Counter } from './fixtures/Counter' | ||
|
||
test('renders simple component', async () => { | ||
const screen = render(<HelloWorld />) | ||
const screen = await render(<HelloWorld />) | ||
await expect.element(page.getByText('Hello World')).toBeVisible() | ||
expect(screen.container).toMatchSnapshot() | ||
}) | ||
|
||
test('renders counter', async () => { | ||
const screen = render(<Counter initialCount={1} />) | ||
const screen = await render(<Counter initialCount={1} />) | ||
|
||
await expect.element(screen.getByText('Count is 1')).toBeVisible() | ||
await screen.getByRole('button', { name: 'Increment' }).click() | ||
await expect.element(screen.getByText('Count is 2')).toBeVisible() | ||
}) | ||
|
||
test('renders child component on mount for suspense boundary which is not suspending', async () => { | ||
const { getByText } = await render(<HelloWorld name="Vitest" />, { | ||
wrapper: ({ children }) => ( | ||
<Suspense fallback={<div>Suspended!</div>}>{children}</Suspense> | ||
), | ||
}) | ||
await expect.element(getByText('Hello Vitest')).toBeInTheDocument() | ||
}) |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This component won't trigger the suspense boundary since it doesn't throw a promise /
use
. Testing this behavior will need a specific component implementation. A positive assertion of the suspense boundary being present should help ensure the test is valid.If React 19 isn't ready to be used in this project yet, maybe something like this?
I think React somehow resumes execution if the thrown promise is already resolved, but if not a boolean flag could be added to gate the
throw
.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.
Thanks for the feedback.
Two things:
Suspense
fallback and it has already flushed as part of mounting (that's why we're wrapping therender
function). The use case you're mentioning should be covered by the asynchronous nature of the locators. This test is from a reproduction in the issue attached :) I'll rename the test case to make that clear.act
should be awaited and the sync version ofact
will be deprecated at some point (per theact
docs).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.
If you're referring to #10, that was my reproduction! My point is mainly that if the component doesn't actually suspend, the boundary will not have been rendered in the first place. I have tested non-suspending components wrapped in suspense boundaries and they work without problems as-is; it's only suspending components which present an issue.
You'll see in the reproduction that the HelloWorld component implementation was altered to suspend via
use
of a promise: https://github.com/a-type/vitest-browser-react-suspense-repro/blob/main/vitest-example/HelloWorld.tsx#L14That said, I only wanted to point out that the test wasn't verifying the behavior in question. I don't have an opinion on whether a test is necessary for that behavior, and I'm not a contributor / maintainer anyway!
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.
Thanks @a-type :) That's my bad, I totally missed the purpose of the test. Let me have a look and try to implement that.
Your point is valid and I try to take any comment seriously even if it's not from maintainers :)
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.
@a-type I pushed a fix though I'm hesitating a bit. I wasn't able to figure out yet if the suspense behavior is consistent. In some cases - React will not show the fallback and just show the content (it looks like that depends on how much time we suspend - as can be seen in the failure in CI. If we extend the timeout - the test will pass). I'm trying to figure that out but I don't have a lot of free time right now :)