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

MudAutoComplete: Fix CoerceText not working on empty search results (#2617) #2619

Merged

Conversation

BederBourahmah
Copy link
Contributor

Fixes #2617

A call to ToggleMenu is not being triggered when clicking away from the MudAutocomplete component. ToggleMenu is where the call to CoerceTextToValue happens. I found that IsOpen was being set to false whenever the search function returned an empty list, but this didn't actually do anything, from what I could tell. So, removing this one line fixes this issue.

I looked into adding it to OnInputBlurred, but that doesn't work because when a selection is made it also triggers OnInputBlurred, as the comment by @henon pointed out.)

@henon
Copy link
Collaborator

henon commented Sep 1, 2021

Please add a testcase to our autocomplete test suite showing that it works and protecting this fix against accidental unfixing by future PRs. You can look at the other tests to see how it is done.

@BederBourahmah
Copy link
Contributor Author

It looks like there was already a test that was supposed to exercise CoerceText in the case of no results found, but it was not catching this bug. I figured that I just needed to add a check to make sure IsOpen was still true after an empty search result was returned. However, when I tested that change on master it was still passing. I found that because OnSearch was an async void method, the state was not getting updated fast enough before the check on IsOpen was executed in the test. I changed OnSearch so it returned a Task and added await in the methods that called it. After that change, the test failed on master and passed on this branch.

@henon
Copy link
Collaborator

henon commented Sep 2, 2021

ok, your solution is fine but there is also another way. you could have done a

comp.WaitForAssertion(()=>autocomplete.IsOpen.Should().BeTrue());

if I am not mistaken.

@BederBourahmah
Copy link
Contributor Author

I don't think WaitForAssertion would work here because the core problem in the unit test was that IsOpen was returning true (the passing state) when I tested on master, when it really should have been returning false because OnSearch was erroneously setting it to false when no results were found. The test was incorrectly seeing a pass condition for IsOpen value because there was still code executing within the component when it was performing the check. I actually tried WaitForAssertion on master and I was getting inconsistent results. Sometimes the test passed, sometimes it failed because WaitForAssertion timed out, and sometimes it failed because Text didn't match the expected on the last line. I attribute that inconsistency to a race condition set into motion by the async void fire-and-forget pattern that was in place. The Task & await pattern guarantees that the code is done executing by the time the test checks IsOpen.

@henon
Copy link
Collaborator

henon commented Sep 3, 2021

Awesome. Thanks for the thorough explanation!

@henon henon merged commit 2259b12 into MudBlazor:dev Sep 3, 2021
@henon henon added this to the 5.1.3 milestone Sep 3, 2021
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.

MudAutoComplete CoerceText only works when there are matches
2 participants