Skip to content

Use testing-library ESLint plugin#9870

Merged
aduth merged 1 commit intomainfrom
aduth-eslint-plugin-testing-library
Jan 8, 2024
Merged

Use testing-library ESLint plugin#9870
aduth merged 1 commit intomainfrom
aduth-eslint-plugin-testing-library

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Jan 5, 2024

🛠 Summary of changes

Adds eslint-plugin-testing-library to enforce additional lints surrounding expected usage of Testing Library related features.

Why?

  • Allows us to remove one of our own custom "restricted syntax" rules which targeted part of what this plugin offers
  • It will catch legitimate issues with usage of Testing Library functions, as evidenced by the included changes necessary to get existing tests passing

📜 Testing Plan

Verify that both lint and tests pass:

yarn lint
yarn test

changelog: Internal, Code Quality, Add stricter linting for DOM testing
submitButton = getByText('forms.buttons.submit.default');
expect(isFormValid(submitButton.closest('form'))).to.be.true();

// eslint-disable-next-line require-await
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is somewhat strange and unexpected. Without it, it identifies the new Promise argument function as being async without and await call, but it's not an async function, and I'm not sure why it would suddenly start being flagged.

My guess is that some transitive dependency of what's changed in yarn.lock changed how the file is being parsed?

Copy link
Contributor

@dawei-nava dawei-nava left a comment

Choose a reason for hiding this comment

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

LGTM

"app/javascript/packages/verify-flow/**",
// In progress: enabling these rules for all files in packages/document-capture
"app/javascript/packages/document-capture/context/**",
"app/javascript/packages/document-capture/higher-order/**",
Copy link
Contributor

Choose a reason for hiding this comment

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

what's gonna lint this file to detect trailing spaces? 😛

server.use(rest.post(locationsURL, (_req, res, ctx) => res(ctx.json([{ name: 'Baltimore' }]))));
server.use(
rest.post(locationsURL, (_req, res, ctx) => res(ctx.json([{ name: 'Baltimore' }]))),
rest.put(locationsURL, (_req, res, ctx) => res(ctx.json({ success: true }))),
Copy link
Contributor

Choose a reason for hiding this comment

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

was this like a missing stub?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

was this like a missing stub?

Yes, with the previous code, we weren't awaiting findAllByText, so the thing it was "clicking" was a promise object, not the intended button element. Clicking the button issues a network request before redirecting. Once the correction was made to await the result to click the button, these changes were also necessary to stub the resulting network response.

@aduth aduth merged commit bafa646 into main Jan 8, 2024
@aduth aduth deleted the aduth-eslint-plugin-testing-library branch January 8, 2024 14:22
@amirbey amirbey mentioned this pull request Jan 9, 2024
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.

3 participants