fix(select): revisit select

[ismay]

closes #64 (does not address the CI setup btw., that needs to be split out)

Todo:

• debounce menu width measurement
• menu width breaks if open while resizing (moved to Popper dropdown for select positioned incorrectly after resize #67)
• find a proper place for the debounce helper
• add debounce test
• separate out stories for this step the top of the menu is aligned with the bottom of the input
• address test failures
• check and review test changes for multiselect (remove unused code), after checking what fixed the tests, apply the fix to singleselect (and other areas).

Notes:

• Slight overlap with the all helper. That allows executing multiple cypress commands. This executes multiple get commands. Speaking of, I think we have a bit too many and too specific test helpers. I find it difficult to understand what happens in the tests at times because of them. I've refactored the tests slightly for the Selects to demonstrate an approach that uses a very basic testhelper (getAll), but other than that duplicates code instead of relying on a higher level test helper.
• Moved the assertions to use should instead of then, so cypress keeps retrying for a while.
• I've debounced setMenuWidth, as otherwise it'd fire continuously during resize. It's an edge case, but also an easy fix.
• I've removed waitForResizeObserver as it was no longer necessary with cypress now waiting automatically.
• The placement for the Select menu is bottom-start now, not just bottom, as this resolves some of the positioning issues I was seeing.

This should resolve all the flakiness we've been seeing with the select. I couldn't find any other potential issues.

Follow up:

• If we all like this approach, my suggestion would be to take a look at the other integration tests in this repo after v5. We could simplify the helpers, and refactor all (async) tests to use should. Basically, follow the recommendations from this article: https://mtlynch.io/good-developers-bad-tests

[cypress]

---

Test summary

438 • 0 • 0 • 0

---

Run details

Project	ui
Status	Passed
Commit	f355ea6
Started	May 20, 2020 12:47 PM
Ended	May 20, 2020 12:56 PM
Duration	08:54 💡
OS	Linux Ubuntu Linux - 18.04
Browser	Electron 80

View run in Cypress Dashboard ➡️

---

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

[ismay]

In looking at our Select test failures I've noticed two things:

• The tests fail intermittently for me locally as well. I think that this is actually related to the (suspected) race condition from Popper dropdown for select positioned incorrectly after resize #67
• With the way we've written our test helpers, cypress doesn't keep retrying for a certain amount of time. If we pass our custom assertions as a callback to .should cypress will keep retrying it for a certain while. That seems nice for allowing async behaviour to resolve:

See: https://docs.cypress.io/guides/references/assertions.html#Should-callback

[Mohammer5]

Nice find with the .should callback!
Another thing I've observed with the focused tests is that the stories use the initiallyFocused prop, but the component is rendered once without being focused so a ref can be generated.
For me, the cypress tests were quick enough to pick up the yet unfocused element and do the assertion which caused the test to fail. I think we should always test what the story prepares instead of relying on it being there already. So either the given step does something or it asserts something to make sure it's assumptions are correct

[ismay]

Nice find with the .should callback!
Another thing I've observed with the focused tests is that the stories use the initiallyFocused prop, but the component is rendered once without being focused so a ref can be generated.
For me, the cypress tests were quick enough to pick up the yet unfocused element and do the assertion which caused the test to fail. I think we should always test what the story prepares instead of relying on it being there already. So either the given step does something or it asserts something to make sure it's assumptions are correct

Yeah I agree. I've added some assertions to do that in the tests here, though I'm sure they could be expanded upon.

[HendrikThePendric]

I like most things in this PR but I also have some concerns reg. the test changes.

I like these parts of this PR:

1. Switching to should so cypress waits
2. Removing the waitForResizeObserver helper, which was a bit of a desperate attempt
3. Adding some mechanism to prevent setMenuWidth from being called too often when the user resizes the window.

I don't really get the comment above the definition of setMenuWidth in the Select. On the one hand you are suggesting a change to requestAnimationFrame, but you have clearly opted for using setTimeout instead. You mention "the added complexity of solving this in another manner does not seem worth it". Are you referring to the added complexity of solving this using requestAnimationFrame? If so, could you explain why this would be so complex?

Another thing I'm wondering about, is the removal of the getAnchorAndMenuRects and the general implications this might/should have for the rest of the codebase. Switching to synchronous code in a should callback seems like a good move when looking at the cypress docs. But does this mean we should move to .should everywhere? I think we are using .then all over the place... I think there is technically nothing wrong with using .then when we are asserting things that happen synchronously, should probably only gives an advantage for asynchronous stuff. However, mixing .then and .should all over our codebase might get confusing/ is inconsistent. What do you think our approach should be regard this?

Anyway, the two points above are just questions, the code looks good, so I've approved.

[ismay]

I don't really get the comment above the definition of setMenuWidth in the Select. On the one hand you are suggesting a change to requestAnimationFrame, but you have clearly opted for using setTimeout instead. You mention "the added complexity of solving this in another manner does not seem worth it". Are you referring to the added complexity of solving this using requestAnimationFrame? If so, could you explain why this would be so complex?

Yeah actually, I'm mixing two different concepts in this comment:

1. Not called this method too often during a resize (the debounce)
2. Measuring at the appropriate time, so we don't trigger a forced layout (that's where requestPostAnimationFrame would be useful).

A complete solution would mix the debounce, and requestPostAnimationFrame, that would be the most performant solution. But, requestPostAnimationFrame is just a proposal, so at the moment there's just a polyfill. We could use the polyfill, and it's rather small, but since usually people don't resize their viewport I omitted it. Seems like it's not worth the added complexity.

I'll update the comment to make this all a bit more clear.

Another thing I'm wondering about, is the removal of the getAnchorAndMenuRects and the general implications this might/should have for the rest of the codebase.

Yeah I would prefer using simpler (or no) helpers everywhere else as well. I'm trying to follow the recommendations outlined here: https://mtlynch.io/good-developers-bad-tests

Switching to synchronous code in a should callback seems like a good move when looking at the cypress docs. But does this mean we should move to .should everywhere? I think we are using .then all over the place... I think there is technically nothing wrong with using .then when we are asserting things that happen synchronously, should probably only gives an advantage for asynchronous stuff. However, mixing .then and .should all over our codebase might get confusing/ is inconsistent. What do you think our approach should be regard this?

What I tried in this PR is to move any assertions that were wrapped in a then, to a should. This is what cypress recommends in the docs as well:

As they note, we should be aware that the callback in a should could be run multiple times, so we should make sure that there are no side effects in the callback.

So I'd say:

• If it's an assertion, use .should instead of .then
• If you want to work with the subject yielded from the previous command, and it's not an assertion, use .then

[dhis2-bot]

🎉 This PR is included in version 5.0.0-alpha.17 🎉

The release is available on:

• npm package (@alpha dist-tag)
• npm package (@alpha dist-tag)
• npm package (@alpha dist-tag)
• npm package (@alpha dist-tag)
• npm package (@alpha dist-tag)
• npm package (@alpha dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[dhis2-bot]

🎉 This PR is included in version 5.0.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• npm package (@latest dist-tag)
• npm package (@latest dist-tag)
• npm package (@latest dist-tag)
• npm package (@latest dist-tag)
• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀