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

feat(await-async-events): instance of userEvent is recognized as async #830

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Kvanttinen
Copy link

@Kvanttinen Kvanttinen commented Oct 15, 2023

Checks

Changes

Adds a feature that events from the instance of userEvent are recognized as async events.

Example:
const user = userEvent.setup();
user.click(button);

The 'user' will be flagged in this case.

Context

Closes #812

My first open source PR, and not used to GitHub that much, so there might be some errors with the process. Changes itself are quite simple, but I didn't find a more elegant way to do that.

@Kvanttinen Kvanttinen marked this pull request as draft October 15, 2023 16:05
@Kvanttinen Kvanttinen force-pushed the pr/await-async-events-rule-validates-userevent-session branch from 89e5f97 to f28f7e1 Compare October 15, 2023 16:41
@Kvanttinen Kvanttinen marked this pull request as ready for review October 15, 2023 16:52
@Belco90 Belco90 self-requested a review October 16, 2023 06:48
@Kvanttinen Kvanttinen force-pushed the pr/await-async-events-rule-validates-userevent-session branch from f28f7e1 to 8ba464f Compare October 16, 2023 11:44
@ka2jun8
Copy link

ka2jun8 commented Dec 19, 2023

any updates?

@Belco90
Copy link
Member

Belco90 commented Jan 12, 2024

@ka2jun8 Sorry for the late response! I'll review this soon.

feat(await-async-events): added comments

feat(await-async-events): better test case

feat(await-async-events): edge case fixed, test added

feat(await-async-events): use actual userEvent import for check, tests
@Belco90 Belco90 force-pushed the pr/await-async-events-rule-validates-userevent-session branch from 8f1e313 to 636dee8 Compare January 12, 2024 12:41
Comment on lines +336 to +341
const getUserEventImportIdentifier = (node: ImportModuleNode | null) => {
if (!node) {
return null;
}

if (isImportDeclaration(node)) {
Copy link
Member

Choose a reason for hiding this comment

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

This function should always receive a node. If the node is not available, then avoid calling this function in the corresponding code.

Suggested change
const getUserEventImportIdentifier = (node: ImportModuleNode | null) => {
if (!node) {
return null;
}
if (isImportDeclaration(node)) {
const getUserEventImportIdentifier = (node: ImportModuleNode) => {
if (isImportDeclaration(node)) {

}

return null;
return getUserEventImportIdentifier(importedUserEventLibraryNode);
Copy link
Member

Choose a reason for hiding this comment

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

Here you should call getUserEventImportIdentifier only if importedUserEventLibraryNode exists.

Comment on lines +703 to +711
if (
token.type === 'Identifier' &&
token.value === userEventImport.name &&
tokensAndComments[index + 1].value === '.' &&
tokensAndComments[index + 2].value === 'setup' &&
tokensAndComments[index + 3].value === '(' &&
tokensAndComments[index - 1].value === '='
) {
return tokensAndComments[index - 2].value;
Copy link
Member

Choose a reason for hiding this comment

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

This looks a bit brittle and hardcoded… I'm trying to think a better way to implement this in a more generic way with AST selectors.

* Finds if the userEvent is used as an instance
*/

export function getUserEventInstance(
Copy link
Member

Choose a reason for hiding this comment

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

I think this one should belong to the detect-testing-library-utils too.

@@ -32,7 +32,7 @@ const USER_EVENT_ASYNC_FUNCTIONS = [
'upload',
] as const;
const FIRE_EVENT_ASYNC_FRAMEWORKS = [
'@testing-library/vue',
// '@testing-library/vue',
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be commented.

@Belco90
Copy link
Member

Belco90 commented Jan 12, 2024

I gave it a first round. After the changes requested are applied, I'll give it another round.

@ttbui11
Copy link

ttbui11 commented Aug 28, 2024

any updates on this please?

@MichaelDeBoey
Copy link
Member

@Kvanttinen Are you still interested in implementing this feature?

If so, can you please rebase onto latest main & fix @Belco90's remarks?

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.

await-async-events doesn't seem to check calls from user sessions returned by userEvent.setup()
5 participants