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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update flakey test for no-selected-start-before-selected-end #1854

Conversation

@krissalvador27
Copy link
Contributor

krissalvador27 commented Nov 4, 2019

What's this fix?
Fixes the breaking test in master from merged in PR #1608

What happened?
The broken test was validating that a change in end date would call deleteModifier for no-selected-start-before-selected-end on all visible days. Turns out the test was flakey because the visible days object calculated by DayPickerRangeController is determined by the initialVisibleMonthThunk.

const initialVisibleMonthThunk = initialVisibleMonth || (
   startDate ? () => startDate : () => this.today
);

DayPickerRangeController.jsx Line 1065.

The default initialVisibleMonthThunk does not take into account a passed in end date and by defaulting to today, the test cannot rely on a hardcoded number of times that deleteModifier will run. This isn't a major issue since DayPickerRangeController is most likely consumed by using the DateRangePicker kitchen sink that determines initialVisibleMonthThunk like so

const initialVisibleMonthThunk = initialVisibleMonth || (
    () => (startDate || endDate || moment())
);

DayPickerRangeController.jsx Line 460.

Happy to update the default of DayPickerRangeController to match the default on the DateRangePicker for both consistency and expected behavior, but wanted to prioritize fixing the flakey test for now.

Let me know if this works @majapw @ljharb! 馃榿

@ljharb
ljharb approved these changes Nov 4, 2019
Copy link
Collaborator

ljharb left a comment

seems reasonable to me

@krissalvador27

This comment has been minimized.

Copy link
Contributor Author

krissalvador27 commented Nov 5, 2019

@ljharb Would we be able to merge and publish a new minor? I feel some slight nervousness for being the pull request that is breaking master right now 馃槄

@ljharb

This comment has been minimized.

Copy link
Collaborator

ljharb commented Nov 5, 2019

No need to publish anything just for tests; I鈥檓 going to defer to @majapw to merge this tho.

@majapw majapw merged commit 5b479ee into airbnb:master Nov 7, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 80.724%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can鈥檛 perform that action at this time.