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

Feature/dom testing library #226

Merged
merged 15 commits into from
Feb 19, 2021

Conversation

thawkin3
Copy link
Contributor

@thawkin3 thawkin3 commented Feb 17, 2021

I've re-written the test suite using Jest and DOM Testing Library. 🎉

Screen Shot 2021-02-17 at 8 23 53 PM

This PR is somewhat large, so the following summary of changes should help:

  1. Added needed devDependencies: @testing-library/dom, @testing-library/jest-dom, babel-jest, jest, and jest-watch-typeahead
  2. Added Jest configuration: jest.config.js file, setupTests.js file, ESLint configuration, Babel configuration, script in package.json, and GitHub workflow configuration
  3. Wrote the unit tests in four separate files, one for each exported method: focusable, isFocusable, tabbable, and isTabbable
  4. Updated README to mention the Jest unit tests
  5. Updated README to no longer mention support for a[xlink:href] attributes
  6. Stripped out all Karma-related code: test file, devDependencies, npm script, CI config, Karma config file, README info

Closes #225

PR Checklist

Please leave this checklist in your PR.

  • Source changes maintain stated browser compatibility.
  • Issue being fixed is referenced.
  • Unit test coverage added/updated.
  • E2E test coverage added/updated.
  • Typings added/updated.
  • README updated (API changes, instructions, etc.).
  • Changes to dependencies explained.
  • Changeset added (run yarn changeset locally to add one, and follow the prompts).

@changeset-bot
Copy link

changeset-bot bot commented Feb 17, 2021

🦋 Changeset detected

Latest commit: 170cdb7

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
tabbable Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

test/tabbable.jest.test.js Outdated Show resolved Hide resolved
test/tabbable.jest.test.js Outdated Show resolved Hide resolved
@thawkin3
Copy link
Contributor Author

@stefcameron Here's my first stab at it! I still have three TODO items that I mentioned in the PR description. The biggest thing that I wanted to confirm with you is that there's no reason to keep the Karma setup around anymore, right? If that's the case, there is a whole bunch of config code that I can strip out.

@idoros
Copy link
Contributor

idoros commented Feb 17, 2021

Looks good!

In regard to the radio button tests, the difference you get from the previous test code is that the new code doesn't attach the fixtures to the DOM. It doesn't break the behavior of most cases, but radio buttons have some query usage to find other group elements. And it make sense that "tabbable" elements would be attached to the DOM.

Not sure the xlink is related to this PR, but adding a[xlink:href] selector to the candidateSelectors list will probably pass your tests.

Copy link
Member

@stefcameron stefcameron left a comment

Choose a reason for hiding this comment

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

Great work, @thawkin3 , this is looking nice! Thanks for the nice summary in the PR description. I don't see any reason to keep Karma around. There's no other purpose for it beside running tests that are now run by Jest, so it can be ripped out rather than preserved.

test/focusable.jest.test.js Outdated Show resolved Hide resolved
test/isFocusable.jest.test.js Outdated Show resolved Hide resolved
@thawkin3
Copy link
Contributor Author

@stefcameron Awesome, I'll rip out Karma in another commit soon and add it to this PR. This PR is going to get a whole lot bigger with all the deletions!

@thawkin3
Copy link
Contributor Author

In regard to the radio button tests, the difference you get from the previous test code is that the new code doesn't attach the fixtures to the DOM. It doesn't break the behavior of most cases, but radio buttons have some query usage to find other group elements. And it make sense that "tabbable" elements would be attached to the DOM.

@idoros Well I learned something new today! Thanks for pointing that out. To avoid any other possible surprises, I made sure to actually append all of the test examples to the DOM now. You were spot on, and that fixed the radio button tests.

@thawkin3
Copy link
Contributor Author

@stefcameron @idoros This behemoth of a PR is ready for a second round of review! Here's a quick summary of what I've changed since our last conversation:

  • Stripped out Karma and everything related to it
  • Removed the info about the xlink:href attribute in the README
  • Fixed the skipped Jest tests
  • Updated the Jest tests to actually append the container content to the document body
  • Added a patch changeset

@stefcameron
Copy link
Member

@thawkin3 Thanks for the updated summary -- so helpful! -- and the additional work. I'll check this out sometime tomorrow.

Copy link
Member

@stefcameron stefcameron left a comment

Choose a reason for hiding this comment

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

This looks great to me, nice work, thank you! 🎉 I'm really happy to have this repo now also using the Testing Library.

@stefcameron
Copy link
Member

@all-contributors add @thawkin3 for test, infra, doc

@allcontributors
Copy link
Contributor

@stefcameron

I've put up a pull request to add @thawkin3! 🎉

@stefcameron
Copy link
Member

@all-contributors add @idoros for review

@allcontributors
Copy link
Contributor

@stefcameron

I've put up a pull request to add @idoros! 🎉

@thawkin3
Copy link
Contributor Author

This looks great to me, nice work, thank you! 🎉 I'm really happy to have this repo now also using the Testing Library.

@stefcameron Thanks! It was fun to work on. I see you've approved this now, so are we good to merge this today? Or are we waiting on the timing for anything else?

@stefcameron
Copy link
Member

This looks great to me, nice work, thank you! 🎉 I'm really happy to have this repo now also using the Testing Library.

@stefcameron Thanks! It was fun to work on. I see you've approved this now, so are we good to merge this today? Or are we waiting on the timing for anything else?

I'm merging now. I was just pausing just in case there were any last words. 😄

@stefcameron stefcameron merged commit f9f6d25 into focus-trap:master Feb 19, 2021
@stefcameron
Copy link
Member

@all-contributors add @idoros for review

@allcontributors
Copy link
Contributor

@stefcameron

I've put up a pull request to add @idoros! 🎉

@thawkin3 thawkin3 deleted the feature/dom-testing-library branch February 19, 2021 17:31
@thawkin3
Copy link
Contributor Author

I'm merging now. I was just pausing just in case there were any last words. 😄

Haha thanks!

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.

Use @testing-library/dom and Jest for tests
3 participants