Get the tests working with 1.0.0

[JedWatson]

There are a lot of failing tests at the moment. @bruderstein, if you've got time at the moment I could really use your help, it feels like a lot of them should be passing but there's something I'm missing (updates on render don't seem to complete before the DOM is queried, for one)

I've already fixed quite a few tests, updated others to use new class names, etc. and added some TODO notes where major API changes mean tests need to be rewritten.

[bruderstein]

I'll see what I can do - stretched for time at the moment myself, between real-work and a new testing lib that should make these sort of tests easier. I'll take a look though, maybe there's some quick wins.

[bruderstein]

OK, had a play around, and I think I understand the main issue :-)

The tests are still assuming the selected value is uncontrolled - ie. When we click on an option, that option is then displayed. Of course, that isn't the case any more. I'm inclined to remove a lot of those tests, as we need to check that onChange is correctly called, and when value is set, that the right option is displayed, but we shouldn't be checking that the selected value changes after the onChange is called, as that is no longer our responsibility (yay, by the way!)

I'll go through the tests and work out what doesn't make sense any more, and where we're maybe missing stuff. Prob be the weekend before I get chance though.

[bruderstein]

Just a quick update - I've started to get somewhere with this. I'll send a PR as soon as I've got the bulk done. One regression I've found so far is when value is set to a value that is not in the options, it doesn't select anything, previously it would use the value as the label. Not sure how you want to handle that now, but I've left the test in so you can decide.

[nkbt]

I've started to debug tests too today =).

First one issue found:

UnexpectedError: 
expected
<div class="Select is-focused is-pseudo-focused is-searchable has-value" data-reactid=".l">
  <input data-reactid=".l.0" name="form-field-name" type="hidden" value="one">
  <div class="Select-control" data-reactid=".l.1">
    <div class="Select-value" data-reactid=".l.1.0">...</div>
    <div class="Select-input" data-reactid=".l.1.1" style="display: inline-block">...</div>
    <span aria-label="Clear value" class="Select-clear-zone" data-reactid=".l.1.3" title="Clear value">...</span>
    <span class="Select-arrow-zone" data-reactid=".l.1.4">...</span>
  </div>
</div>
queried for .Select-placeholder to have text 'Select...'

Legitimate issue since select has no value and does not render placeholder

return !this.state.inputValue ? <div className="Select-placeholder">{this.props.placeholder}</div> : null;

@bruderstein just found you are also working on this =) Let me know when you pause, push PR, so I can continue on other tests. This way we can split the task

[nkbt]

Another test:

UnexpectedError: 
expected
<div class="Select Select--multi is-focused is-searchable" data-reactid=".u">
  <div class="Select-control" data-reactid=".u.1">
    <div class="Select-placeholder" data-reactid=".u.1.0">...</div>
    <div class="Select-input" data-reactid=".u.1.1" style="display: inline-block">...</div>
    <span aria-label="Clear all" class="Select-clear-zone" data-reactid=".u.1.3" title="Clear all">...</span>
    <span class="Select-arrow-zone" data-reactid=".u.1.4">...</span>
  </div>
</div>
queried for
.Select-value .Select-value-label to satisfy [
  expect.it('to have text', 'Two'),
  expect.it('to have text', 'One')
]
  The selector .Select-value .Select-value-label yielded no results

Yes, valueArray has nothing in there. Test legitimately fail.

The problem is with expandValue function

value = value.split(instance.props.delimiter)
["2", "1"]

value.map(instance.expandValue)
[undefined, undefined]

Actually, value is compared with === but there is a type mismatch. Split value is "2" but value in options is 2

[bruderstein]

Hi @nkbt ,

Thanks for helping out. I've pushed where I'm up to here: https://github.com/bruderstein/react-select/tree/fix-tests-1.0

Could you maybe edit your comment with the names of the tests that are actual regressions - it's a bit tricky to know which tests you're talking about with just the error message.

I've tried to keep each "change" on a separate commit, so we can follow if there's work to be done later. For example, I've removed some tests that really need redoing, using the new logic. But step 1 is to get us back to green.

Thanks
Dave

[bruderstein]

PS, @nkbt - I'm probably not going to be able to do much more until Thursday, so if you get any further, how about you send a PR to my branch, then we can coordinate efforts?

We will be green again 🎉

[nkbt]

@brianreavis I'll have a look at your branch then (hopefully tomorrow and later). Fork, update, pr - that would work totally ok. Cheers.

[JedWatson]

@nkbt I think you meant @bruderstein :)

Thanks guys, really appreciate the help!

re:

One regression I've found so far is when value is set to a value that is not in the options, it doesn't select anything, previously it would use the value as the label

This is working as designed in the new version, I'll have to document the change. Using values that aren't in the options is still supported, but you have to provide value objects (we're not expanding strings as before)

... unless someone thinks that's a bad design decision? I think it makes sense that if you want this behaviour, it's fine to pass value as an object. One less piece of magic. Also plays nicely with the "controlled" value behaviour change.