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

More tests #321

Merged
merged 23 commits into from
Jul 28, 2015
Merged

More tests #321

merged 23 commits into from
Jul 28, 2015

Conversation

bruderstein
Copy link
Collaborator

Some more tests, this time based around options and value.

I've split this into two commits - the first adds some pretty general test cases, the second adds a single failing test, that shows the bug if the value is set, then the matching option arrives.

I've checked, and this issue is resolved by a rebased PR #79

(Realistically, this is all I'm going to manage for the next couple of weeks - but, I'll be back - there's more to come :)

@bruderstein
Copy link
Collaborator Author

@JedWatson - not sure if you've seen this, or are leaving it as the build is failing. How would you like to deal with these? I've got a few more failing tests, which I've fixed in the same commit - I'll push those when I've got a decent connection again. I've not yet found anything else that needs a lot of fixing (just been one liners), so I'm hoping there's no more where it doesn't make sense to include the fix with the test.

This specifically tests when the options change to include
the value. This is a bug.
Add a pressEscape() function
event needs to be passed on to the clearValue() method
Tests check that the escape handling is correct 
in the two states (open and closed)
Otherwise wallaby uses the io.js default, which
means jsdom has issues (different V8 ABI)
using autoload=true and autoload=false
Coverage now up to ~80%
Pressing escape when clearable=false and menu
is closed now has no effect
One batch for a single character (;), and another
for a multi-char string
onKeyDown was checking the state, not props, for disabled
This didn't affect the normal operation, as the control
was not focusable when disabled, but could have potentially
caused issues in certain browsers
className should be appended to selected,
not replaced
Second click closes the options
Right click is ignored
@bruderstein
Copy link
Collaborator Author

Whilst there are still some props not tested (notably valueRenderer), this is now a more complete test suite. There are certainly cases and combinations that aren't covered, but coverage is up in the 90s, and a good number of the common cases are now covered. I'm not sure how the late options issue has been fixed - probably via the placeholder PR, but it now works too.

I'll send a separate PR for the currently untested props when I get chance.

I've tried to keep everything black-box, so the only thing it relies on, is that options have .Select-option, and the search input has .Select-input etc. This should make it easy to refactor stuff :)

There's a couple of bugfixes in here - marked with "fix for...". The would-be failing tests are in the same commit.

I've tried to keep the commits logical, so each prop/feature has tests in a single commit.

Comments obviously welcome.

JedWatson added a commit that referenced this pull request Jul 28, 2015
@JedWatson JedWatson merged commit f79ce8d into JedWatson:master Jul 28, 2015
@JedWatson
Copy link
Owner

Comments obviously welcome.

Epic. Thank you. This is awesome.

@JedWatson
Copy link
Owner

(also sorry for the delay - I was really sick for a week or so after getting home from React Europe and had a huge number of notifications, this did get lost amongst them and I'm working through getting back on top of everything now!)

@bruderstein
Copy link
Collaborator Author

No worries - hope you're feeling better!

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