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

MudTextField: Fix double validation on blur (#7034) #8121

Merged
merged 4 commits into from
Feb 19, 2024

Conversation

vernou
Copy link
Contributor

@vernou vernou commented Feb 2, 2024

Description

In the precedent PR #7996, we introduced a boolean "_validated" to know when the validation already occurred and to avoid multiple validation.

But "_validated" is set to true after the validation is finished. When the validation is async, it's possible to start multiple validation in parallel, before "_validated" is set to true.

Resolves #7034.

How Has This Been Tested?

I added a test that test when the validation is async.
The test fail without the fix.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • The PR is submitted to the correct branch (dev).
  • My code follows the code style of this project.
  • I've added relevant tests.

@github-actions github-actions bot added bug Something does not work as intended/expected PR: needs review labels Feb 2, 2024
@vernou
Copy link
Contributor Author

vernou commented Feb 2, 2024

@henon, this PR complements a precedent PR that you contribute.
Can you review this PR?

@mikes-gh
Copy link
Contributor

mikes-gh commented Feb 2, 2024

@vernou Thanks for the PR. Your test is failing.

Copy link
Collaborator

@henon henon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, straight forward and simple. Change looks good.

I don't get why the test doesn't work on the CI, I guess it works locally?

To get an understanding why the test fails on the CI you could temporarily add some Console.WriteLines in key locations to figure out what happens and wheter or not this is a timing issue or not

@henon
Copy link
Collaborator

henon commented Feb 2, 2024

OK, straight forward and simple. Change looks good.

Hmm on second thought, wouldn't this prevent validation entirely because it is et before the call?

@vernou
Copy link
Contributor Author

vernou commented Feb 2, 2024

I don't get why the test doesn't work on the CI, I guess it works locally?

Yes, it's work locally. It's a parallelization problem and the check is done before the validation.
It's fixed.

Hmm on second thought, wouldn't this prevent validation entirely because it is et before the call?

No, because when _validated is set to true, nothing prevents validation :

_validated = true;
await base.ValidateValue();

@mikes-gh
Copy link
Contributor

mikes-gh commented Feb 2, 2024

I am not sure we should be adding delays to make tests past.
@henon Shouldn't we be using WaitForAssertion of some state then run the check?

Copy link

codecov bot commented Feb 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e5b8c80) 88.11% compared to head (8e79b38) 88.12%.
Report is 4 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #8121      +/-   ##
==========================================
+ Coverage   88.11%   88.12%   +0.01%     
==========================================
  Files         394      394              
  Lines       11760    11760              
  Branches     2384     2384              
==========================================
+ Hits        10362    10364       +2     
  Misses        871      871              
+ Partials      527      525       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@vernou
Copy link
Contributor Author

vernou commented Feb 7, 2024

@mikes-gh
I agree, I don't like Task.Delay in a test. But I don't see how to do otherwise.

The first assert check the state has changed, so WaitForAssertion can be used.
But the second assert check the state didn't change, I don't know how avoid delay in this case.

I modified the test in this way.
If you know a better approach, I'm interested.


Also the test didn't work, because it didn't wait before to check the validation isn't called a second time.
It's fixed in the modification.

@vernou
Copy link
Contributor Author

vernou commented Feb 18, 2024

@henon, @mikes-gh
What is the next step?

@henon henon merged commit f150418 into MudBlazor:dev Feb 19, 2024
3 checks passed
@henon
Copy link
Collaborator

henon commented Feb 19, 2024

Thanks for the contribution @vernou and sorry, the PR slipped my mind.

@vernou vernou deleted the fix/multiple-validation branch February 19, 2024 11:06
biegehydra pushed a commit to biegehydra/MudBlazor that referenced this pull request Apr 26, 2024
…or#8121)

* MudTextField: Fix double validation on blur
* Remove double _validated set to true
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something does not work as intended/expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OnFieldChanged event is triggered twice
3 participants