Tests!

[ryanalane]

Set up a testing/coverage environment (enzyme + mocha + chai + isparta (istanbul) + various helper modules).

Added various types of tests: 1) Behavioral acceptance tests, 2) Keyboard Event Handlers, 3) unit tests for Autocomplete#renderMenu.

[whichsteveyp]

🎉I'll get to reviewing this straight away. What are your thoughts/preferences on rebase+squashing commits? I've generally tried to follow that for a lot of the internal projects I've worked on - this repo currently does not have a "standard best practice" so I'd hate to try and enforce something that isn't being done and also folks don't want to do.

Thanks a ton for doing these, I was wanting to move forward on some of the other PRs and was a bit nervous to do this without tests. <3

[ryanalane]

@sprjr I have no preferences on rebasing, but am happy to do so if it would be preferable. How do you generally go about it for your internal projects?

Also, my pleasure! I'm glad it's enabling. :)

[whichsteveyp]

We've been pushing for rebase then merge, and we try to do the "single feature branches squash down to single commits."

Although since you have multiple authors you might also want to adjust that - and it seems like much ado about nothing since the project itself doesn't enforce/suggest it.

I'll merge it in tomorrow as is after validating locally. Thanks!

Sent from my iPhone

On Mar 7, 2016, at 4:19 AM, Ryan Lane [email protected] wrote:

@sprjr I have no preferences on rebasing, but am happy to do so if it would be preferable. How do you generally go about it for your internal projects?

Also, my pleasure! I'm glad it's enabling. :)

—
Reply to this email directly or view it on GitHub.

[whichsteveyp]

I ran the tests locally and saw a failure:

I'm on [email protected] and installed from [email protected]. I did notice that babel-register is not in the dev dependencies so if you don't install it locally (or use it globally perhaps?) you can't just do npm i && npm test. If you could:

• Add babel-register to devDeps
• Update README.md with info about running tests
• Updated package.json with the proper contributors info ;)
• Help me figure out why that one test is failing

We should be good to merge. Thanks!

[whichsteveyp]

fwiw, I did a rebuild on node 0.12.x and received a jsdom error. I thought that might be a potential fix as it looks like there is some jsdom stuff in the stack trace.

[ryanalane]

Phew, this was an interesting dependency quest!

First the real bug in this branch you discovered:
The failed test as-per-your-screenshot is complaining about an incorrect use of HTMLInputElement#setSelectionRange in lines 160-162. I must have accidentally deleted the second parameter.

Second, why the test didn't fail for me:
Older versions of jsdom (< 7.x.x) don't support or somehow don't care about the HTMLInputElement API, so the test passed. Now how did I end up with an older version? Turns out that the latest available version of jsdom (*) is specified only as a peerDependency for mocha-jsdom. I was building with [email protected], which doesn't install peer dependencies, and thus was unwittingly cowboying around with an old jsdom declared as a (seemingly unused!) dependency for cheerio.

Solution: add back the deleted parameter and pin jsdom to ^8.1.0. Just pushed. :)

[ryanalane]

As for updating README.md and package.json, what would you like in them specifically? Happy to make the changes. :)

[whichsteveyp]

Oh wow, thanks for digging in to that! I figured we should update the README.md to say there are tests now and now to run them, instead of indicating they aren't there.

package.json is really just optional if you'd like to add yourselve(s) as contributors. You are in fact, contributing.

[email protected] is still not listed as a devDep, so if you don't have it globally (which I am guessing you do) you can't run the tests unless you install it locally first.

edit: also confirmed this cleans up the tests on my fresh install / setup. 🎉

[kmudrick]

If you are adding jsdom 8, I think you will also need to update .travis.yml, since it currently tries to build with node 0.10 (https://github.com/reactjs/react-autocomplete/blob/master/.travis.yml#L3), which is not compatible with the newer versions of jsdom (https://github.com/tmpvar/jsdom/blob/master/Changelog.md#700).

Also, a warning in the README that it will only build with node >= 4 might be useful.

[ryanalane]

@sprjr Hmmm, perhaps you're viewing old versions of those files? I'll tag you in the diffs.

[ryanalane]

@sprjr README test instructions. Is there more information that would be useful here?

[whichsteveyp]

Yep, forgot that I needed to git fetch and not just git pull my mistake. Thanks!

[ryanalane]

@sprjr My pleasure! Thanks for the feedback and support. :) Always good to hear that efforts have been helpful.