MudFormComponent: Fix bad test causing invalid operation exception by igotinfected · Pull Request #7579 · MudBlazor/MudBlazor

igotinfected · 2023-09-30T10:33:27Z

Description

Fixes an issue causing an InvalidOperationException.

I am not sure what kind of impact this change can have on an application, never needed to wrap a StateHasChanged call with InvokeAsync.

Stacktrace (#7578):

System.InvalidOperationException: The current thread is not associated with the Dispatcher. Use InvokeAsync() to switch execution to the Dispatcher when triggering rendering or component state.
   at Microsoft.AspNetCore.Components.Dispatcher.AssertAccess()
   at Microsoft.AspNetCore.Components.RenderTree.Renderer.AddToRenderQueue(Int32 componentId, RenderFragment renderFragment)
   at Microsoft.AspNetCore.Components.ComponentBase.StateHasChanged()
   at MudBlazor.MudFormComponent`2.ValidateValue() in /_/src/MudBlazor/Base/MudFormComponent.cs:line 348
   at MudBlazor.MudFormComponent`2.<BeginValidateAsync>b__62_0() in /_/src/MudBlazor/Base/MudFormComponent.cs:line 243
   at MudBlazor.MudDatePicker.SetDateAsync(Nullable`1 date, Boolean updateValue) in /_/src/MudBlazor/Components/DatePicker/MudDatePicker.cs:line 54
   at MudBlazor.TaskExtensions.AndForget(Task task, Boolean ignoreExceptions) in /_/src/MudBlazor/Extensions/TaskExtensions.cs:line 33

Previously this test would pass but threw an exception:

Now it passes without exception:

How Has This Been Tested?

Re-ran all unit tests. Had to adapt one ColorPicker test for flakiness.

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.

codecov · 2023-09-30T10:37:55Z

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (160757e) 90.58% compared to head (6441140) 90.57%.

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #7579      +/-   ##
==========================================
- Coverage   90.58%   90.57%   -0.02%     
==========================================
  Files         427      427              
  Lines       15219    15219              
==========================================
- Hits        13786    13784       -2     
- Misses       1433     1435       +2

Files	Coverage Δ
src/MudBlazor/Base/MudFormComponent.cs	`78.43% <ø> (ø)`

... and 1 file with indirect coverage changes

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

ScarletKuro · 2023-09-30T22:23:53Z

For the context, the error was discovered during this CI run: https://github.com/MudBlazor/MudBlazor/actions/runs/6361995369/job/17276757228?pr=7578

Do I understand it correctly, that this exception is not related to #7578 PR and it's was a flaky test introduced here #7564?
I just rerun the CI and next run was without errors, so I assume it's flaky.

I am not sure what kind of impact this change can have on an application, never needed to wrap a StateHasChanged call with InvokeAsync.

Usually, there shouldn't be any impact, if the call is on the correct thread the StateHasChanged will be invoked as usual, if not then in will be queued in the dispatcher.

My understanding is that the the problematic line is here in the Date parameter property of MudDatePicker:

MudBlazor/src/MudBlazor/Components/DatePicker/MudDatePicker.cs

Line 27 in 160757e

set => SetDateAsync(value, true).AndForget();

which this test is calling:

MudBlazor/src/MudBlazor.UnitTests/Components/DatePickerTests.cs

Line 1199 in 160757e

picker.Date = oldDate;

and it not always doing it in the UI context?
Shouldn't you modify the component parameters through SetParam -> comp.SetParam(p => p.Date, oldDate); to avoid this?

@igotinfected can you please test if the SetParam fixes the test without the InvokeAsync change, because I think it's better to fix the test in the correct way, instead of adjusting the core code, because I don't think this validation method should be called outside of the UI anyway? @henon

igotinfected · 2023-09-30T22:55:25Z

Do I understand it correctly, that this exception is not related to #7578 PR and it's was a flaky test introduced here #7564?
I just rerun the CI and next run was without errors, so I assume it's flaky.

@ScarletKuro Seems like it, the CI for that PR has the exception in the stacktrace but didn't fail: https://github.com/MudBlazor/MudBlazor/actions/runs/6355882553/job/17264634698

The re-run you launched for my other PR passes, but the exception is still present: https://github.com/MudBlazor/MudBlazor/actions/runs/6361995369/job/17283133249

@igotinfected can you please test if the SetParam fixes the test without the InvokeAsync change, because I think it's better to fix the test in the correct way, instead of adjusting the core code, because I don't think this validation method should be called outside of the UI anyway?

Good spot! Can't reproduce the exception if I swap out to SetParam. I'll gladly limit my changes to the test itself but is there a way to avoid this from repeating itself? Seems easy enough to miss, and it's weird that my other PR's CI failed while others passed, even though the exception is still present in those. Locally I get the exception for every run, but cannot reproduce a test run fail.

ScarletKuro · 2023-10-01T00:57:19Z

@ScarletKuro Seems like it, the CI for that PR has the exception in the stacktrace but didn't fail: https://github.com/MudBlazor/MudBlazor/actions/runs/6355882553/job/17264634698
The re-run you launched for my other PR passes, but the exception is still present: https://github.com/MudBlazor/MudBlazor/actions/runs/6361995369/job/17283133249

Wow, I didn't notice. Totally unexpected for me that tests pass but there is exception under tests, pretty weird behavior for me which shouldn't be acceptable.

The problem exist in the #7564 PR run https://github.com/MudBlazor/MudBlazor/pull/7564/checks
And in merge to the main branch https://github.com/MudBlazor/MudBlazor/actions/runs/6361049251/job/17275208289
Luckily it's not a regression, just incorrect test.

Good spot! Can't reproduce the exception if I swap out to SetParam. I'll gladly limit my changes to the test itself

Yeah, lets just remove the InvokeAsync change, and fix the OldDateWithDefinedKind_SetValue_KindUnchanged and GridView_ChooseColor()

Is there a way to avoid this from repeating itself? Seems easy enough to miss

Don't think so, this is a job for the code review (us) to not miss this, this time it went through :)
At least I don't know any way to make unit test to alert this, it's pretty weird that there is exception but unit test is silent like it may or not may fail the test run.

igotinfected · 2023-10-01T10:41:25Z

Pushed the change!

Wow, I didn't notice. Totally unexpected for me that tests pass but there is exception under tests, pretty weird behavior for me which shouldn't be acceptable.

From what I can tell the reason we get a stacktrace at all is because of the default implementation of MudGlobal.UnhandledExceptionHandler:

MudBlazor/src/MudBlazor/Services/MudGlobal.cs

Lines 27 to 31 in bd04d94

    
           /// </summary> 
        
           private static void OnDefaultExceptionHandler(Exception ex) 
        
           { 
        
               Console.Write(ex); 
        
           }

Which is then called here:

MudBlazor/src/MudBlazor/Extensions/TaskExtensions.cs

Lines 28 to 32 in bd04d94

    
           catch (Exception ex) 
        
           { 
        
               if (!ignoreExceptions) 
        
                   MudGlobal.UnhandledExceptionHandler?.Invoke(ex); 
        
           }

No clue if there is any way to make tests fail with that though. You would have to await the AndForget's for the exception to be have an effect, which would defeat the purpose of AndForget.

henon · 2023-10-01T11:22:27Z

Thanks @igotinfected !

…udBlazor#7579) * MudFormComponent: Fix invalid operation exception * Fix test instead of component

MudFormComponent: Fix invalid operation exception

7f1c818

github-actions bot added bug Unexpected behavior or functionality not working as intended PR: needs review labels Sep 30, 2023

Fix test instead of component

6441140

henon approved these changes Oct 1, 2023

View reviewed changes

henon changed the title ~~MudFormComponent: Fix invalid operation exception~~ MudFormComponent: Fix invalid operation exception caused by bad testcase Oct 1, 2023

henon changed the title ~~MudFormComponent: Fix invalid operation exception caused by bad testcase~~ MudFormComponent: Fix bad test causing invalid operation exception Oct 1, 2023

henon merged commit cacd5ad into MudBlazor:dev Oct 1, 2023

ScarletKuro removed the PR: needs review label Oct 1, 2023

igotinfected deleted the fix/invalid-operation-exception branch October 1, 2023 13:22

ilovepilav pushed a commit to ilovepilav/MudBlazor that referenced this pull request Nov 25, 2023

MudFormComponent: Fix bad test causing invalid operation exception (M…

351a3d5

…udBlazor#7579) * MudFormComponent: Fix invalid operation exception * Fix test instead of component

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

MudFormComponent: Fix bad test causing invalid operation exception#7579

MudFormComponent: Fix bad test causing invalid operation exception#7579
henon merged 2 commits intoMudBlazor:devfrom
igotinfected:fix/invalid-operation-exception

igotinfected commented Sep 30, 2023 •

edited

Loading

Uh oh!

codecov bot commented Sep 30, 2023 •

edited

Loading

Uh oh!

ScarletKuro commented Sep 30, 2023 •

edited

Loading

Uh oh!

igotinfected commented Sep 30, 2023

Uh oh!

ScarletKuro commented Oct 1, 2023 •

edited

Loading

Uh oh!

igotinfected commented Oct 1, 2023

Uh oh!

henon commented Oct 1, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

igotinfected commented Sep 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

How Has This Been Tested?

Types of changes

Checklist:

Uh oh!

codecov bot commented Sep 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ScarletKuro commented Sep 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

igotinfected commented Sep 30, 2023

Uh oh!

ScarletKuro commented Oct 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

igotinfected commented Oct 1, 2023

Uh oh!

henon commented Oct 1, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

igotinfected commented Sep 30, 2023 •

edited

Loading

codecov bot commented Sep 30, 2023 •

edited

Loading

ScarletKuro commented Sep 30, 2023 •

edited

Loading

ScarletKuro commented Oct 1, 2023 •

edited

Loading