Skip to content

Conversation

@bruderstein
Copy link
Collaborator

This is the last batch of tests for the existing functionality. I think that pretty much everything is covered now, although there are obviously many combinations of options that are not covered - we'll add these as bugs are reported, would be my suggestion.

Currently there are 8 failing tests (one for searchable=false and multi=true, and 7 for disabled options).

PR #319 corrects the searchable=false/multi=true test, and PR #346 corrects 4 of the 7 disabled option tests.

I'm sending another PR which fixes the bug causing the last three to fail (but that PR doesn't make sense without #346, hence the separate PR).

I've also got a change for #346, to show the disabled options in a search - I'll send that separately as that's a matter of taste as to whether disabled options should appear after a search. It just "felt" wrong when I tried it in the examples.

I've rebased these PRs and tried out the examples, and all LGTM

Happy to rebase this PR when the fixes are in, if you'd prefer to "stay green" 😄

createControl and createControlWithWrapper are now used
for all tests. This hopefully makes it more obvious
how new tests should be written.
Adds tests for removing existing selection from options
This will not pass until PR JedWatson#319 is merged
Updated unexpected-dom devDep to 1.2.0
This results in better error messages if something fails
e.g. you get 
expected '<div .....>' to contain no elements matching '.Select-option'
instead of 
expected NodeList[<...>, <....>] to have length 0, but was length 2
This assumes that a disabled option cannot be focused.
The tests will need to be changed if this is changed,
such that a disabled option can be focused but not 
selected.
This includes some evil things like all options being
disabled and the first option being disabled
This adds an extra line of coverage, but it is important
because it's one of the many branches in the focusAdjacentOption
function.
@bruderstein
Copy link
Collaborator Author

Rebased.

JedWatson added a commit that referenced this pull request Aug 5, 2015
@JedWatson JedWatson merged commit d4327f6 into JedWatson:master Aug 5, 2015
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