Skip to content

Expanding unit test suite#223

Merged
appleboy merged 9 commits intoappleboy:masterfrom
andyfry01:feature-add-unit-tests
Feb 1, 2018
Merged

Expanding unit test suite#223
appleboy merged 9 commits intoappleboy:masterfrom
andyfry01:feature-add-unit-tests

Conversation

@andyfry01
Copy link
Contributor

@andyfry01 andyfry01 commented Jan 28, 2018

Hey @appleboy

Here is a PR for #221.

To summarize the changes:

  • I added testing for the component props and the component methods.
  • I added Enzyme and JSDOM to the test suite, which makes it possible to test aspects of the component that require a DOM (see here for more info).
  • Component prop testing checks whether the component renders props that are passed in and whether it renders default props successfully when no props are passed.
  • Component method testing consists of checking basic method functionality (whether it updates state appropriately on the component, whether it calls other component methods successfully, etc.).
  • Added a test:watch script to package.json so pre-commit doesn't get hung up because of the --watch flag on Jest.

(also accidentally included 834f4ae, which was a commit for this PR. Forgot to switch branches before committing my unit test commits)

Thank you! Let me know if you have any question.
Andy

tabindex: 'testTabindex',
hl: 'testHl',
badge: 'testBadge'
};
Copy link
Owner

Choose a reason for hiding this comment

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

coding style? Is indent two space?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like indenting is tabs right now, should I change it to two spaces?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See e7ed11d

@appleboy
Copy link
Owner

maybe we can drop the node 4 and 5 support.

https://travis-ci.org/appleboy/react-recaptcha/builds/334740273

@andyfry01
Copy link
Contributor Author

Node v6 was released in spring 2016, so dropping support for v4/5 seems ok. It looks like Travis is failing at the yarn build step, so maybe transitioning from yarn to npm could be an alternative to dropping v4/5 support. Npm has package-lock files now, and that was the primary benefit of yarn vs older versions of npm.

@appleboy
Copy link
Owner

@andyfry01 Could you help to switch yarn to npm? Thanks.

@andyfry01
Copy link
Contributor Author

Sure thing, I'll do a little research and let you know.

@andyfry01
Copy link
Contributor Author

Hey @appleboy, after looking into this more closely, it seems like the issue goes deeper than yarn. It looks like support for JSDOM and Jest in Node < v6 is limited (see this thread). I tried to implement this workaround, but it's a bit above my head to be honest and I'm having a hard time getting it to work.

I could spend some more time on this, or if you don't mind dropping v4/5 support then that can be an option.

@appleboy
Copy link
Owner

@andyfry01 Maybe we can drop v4/5 support and update the message in README. OK?

@andyfry01
Copy link
Contributor Author

Hey @appleboy, updated the readme and dropped Node v4/5 from travis.yml.

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.

2 participants