Skip to content

Add Some Tests #16

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Add Some Tests #16

wants to merge 5 commits into from

Conversation

JustinNusca
Copy link

Summary

This PR adds enzyme and an appropriate adapter to the devDependencies, and uses them to run a newly-added test suite.

I wanted to avoid testing specific implementation details, instead focusing on the prop interface (ie, the onReflow/Resize/Position callback props), and ensuring that they were being called (or not called) in the appropriate scenarios.

Coverage is unfortunately not 100% just yet — I had some concerns around the handling of the actual calls to add/removeEventListener, and recent changes that may be preventing the event listener from being appropriately cleaned up when the last component instance is unmounted that I think should be resolved first.

I also needed to move onto some other tasks, but I felt it would be better to submit my work so far and revisit it later, rather than hold onto it until it's complete.

Changes

  • Adds homepage, bugs, and repository fields to the package.json file. ad6496c
  • Adds enzyme & enzyme-adapter-react-16 dependencies and updates other deps accordingly. 32db9e6, cd8cf8f
    • A number of moderate and high security vulnerabilities were detected in the devDependencies when running npm audit, which was resolved via the automated npm audit fix command.
  • Fixes a typo: removeSResizeListenerremoveResizeListener. 72c2ad9
  • Adds a test suite for the components prop interface. 53c6c7c

This resolves warnings from npm regarding a number of moderate and high security vulnerabilities when running `npm audit`.
This package-lock file is the automated result of running `npm audit fix`.
- Adds homepage, bugs, and repository fields.
@JustinNusca JustinNusca changed the title Add Tests Add Some Tests Jul 30, 2018
@izaakschroeder
Copy link
Contributor

I haven't really dug in too deep but I appreciate having tests either way! 😄Unfortunately this kind of library is really hard to test properly without actual browser testing since it exploits some very funny behaviour of scroll events. You can look into puppeteer which has some support for jest to do integration testing if you really want to go down that road.

Either way this is a great start! Thanks! 🎉

Copy link

@tchon tchon left a comment

Choose a reason for hiding this comment

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

LGTM @JustinNusca one question.


afterEach(() => {
jest.clearAllMocks();
wrapper.unmount();
Copy link

Choose a reason for hiding this comment

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

should we clear document.body.innerHTML here too as part of the teardown?

Copy link

@roydelgado roydelgado left a comment

Choose a reason for hiding this comment

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

lgtm, nice work @JustinNusca

Copy link

@ajoshi0 ajoshi0 left a comment

Choose a reason for hiding this comment

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

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.

5 participants