bUnit-dev / bUnit

bUnit is a testing library for Blazor components that make tests look, feel, and runs like regular unit tests. bUnit makes it easy to render and control a component under test’s life-cycle, pass parameter and inject services into it, trigger event handlers, and verify the rendered markup from the component using a built-in semantic HTML comparer.
https://bunit.dev
MIT License
1.12k stars 104 forks source link

feat: extend mouse event extension methods #1427

Closed Qwertyluk closed 5 months ago

Qwertyluk commented 5 months ago

Pull request description

Adds more Mouse Events extension methods. Currently, it's impossible to call asynchronous overloads of mouse events without having to provide a MouseEventArgs parameter. In this PR, I've introduced a way to do so.

PR meta checklist

Code PR specific checklist

Qwertyluk commented 5 months ago

@dotnet-policy-service agree

egil commented 5 months ago

Hey there. I'll take a look at this after Easter.

Qwertyluk commented 5 months ago

Okay, no rush

linkdotnet commented 5 months ago

@egil As a side note: Do we wish to continue to have the async overloads for v2? I do feel it is more consistent not to have them anymore (as you don't have RenderAsync, which would await OnInitAsync and so on). So bUnit always begins something and if you want to await a state you resort to WaitForXXX.

Just my two cents - otherwise the PR looks good so far.

egil commented 5 months ago

Do we wish to continue to have the async overloads for v2? I do feel it is more consistent not to have them anymore (as you don't have RenderAsync, which would await OnInitAsync and so on). So bUnit always begins something and if you want to await a state you resort to WaitForXXX.

I have not through this through. IIRC, awaiting the trigger allows users to await the event handler, not the actual rendering, so it may be worthwhile to keep them.

@Qwertyluk what is your scenario for using the async overloads instead of the non async ones?

Qwertyluk commented 5 months ago

Do we wish to continue to have the async overloads for v2? I do feel it is more consistent not to have them anymore (as you don't have RenderAsync, which would await OnInitAsync and so on). So bUnit always begins something and if you want to await a state you resort to WaitForXXX.

I have not through this through. IIRC, awaiting the trigger allows users to await the event handler, not the actual rendering, so it may be worthwhile to keep them.

@Qwertyluk what is your scenario for using the async overloads instead of the non async ones?

Based on the posts above, I assume these changes won't address my issue. My scenario was about using the non-async versions of event handlers (Click() etc.), which resulted in a component that hadn't re-rendered on time. After reviewing the implementation of these event handlers, I realized they simply call their async counterparts, and I had no opportunity to await them. However, after reading your posts, am I right to conclude that the only way to ensure the component has been re-rendered after the action is by using WaitForXXX and awaiting these event handlers can't guarantee this?

linkdotnet commented 5 months ago

Do we wish to continue to have the async overloads for v2? I do feel it is more consistent not to have them anymore (as you don't have RenderAsync, which would await OnInitAsync and so on). So bUnit always begins something and if you want to await a state you resort to WaitForXXX.

I have not through this through. IIRC, awaiting the trigger allows users to await the event handler, not the actual rendering, so it may be worthwhile to keep them. @Qwertyluk what is your scenario for using the async overloads instead of the non async ones?

Based on the posts above, I assume these changes won't address my issue. My scenario was about using the non-async versions of event handlers (Click() etc.), which resulted in a component that hadn't re-rendered on time. After reviewing the implementation of these event handlers, I realized they simply call their async counterparts, and I had no opportunity to await them. However, after reading your posts, am I right to conclude that the only way to ensure the component has been re-rendered after the action is by using WaitForXXX and awaiting these event handlers can't guarantee this?

Basically yes. The await will just wait for the event handlers code to finish - but that doesn't necessarily translate to "rendering is done". There still could be something in the render-pipeline. For example because of the click you change a parameter that gets passed to a child component. That child component asynchronously (inside OnParametersSetAsync) will fetch something new via an API or so. That part will not be awaited.

Qwertyluk commented 5 months ago

Although these changes don't address my initial problem, I've updated the changelog accordingly.

egil commented 5 months ago

Thanks @Qwertyluk.

You may want to rebase your branch on main. The validate-docs check that fails should be fixed on main.

egil commented 5 months ago

Although these changes don't address my initial problem, I've updated the changelog accordingly.

With this in mind, I think it makes more sense to park this PR for now. We discussed this in our https://www.youtube.com/watch?v=_nStHkIujqc bUnit dev stream today, and I think we want to make these changes more generally to all trigger methods, if at all.

Qwertyluk commented 4 months ago

I understand your decision. I have one more question: If there are plans to make more general changes to the trigger methods, does it make sense to address issue #838 now?

egil commented 4 months ago

@Qwertyluk I think we will keep the current pattern for v1 as is. We have a limited time to work on bUnit, but for v2 we may reimplement the trigger methods as a source generator so all current and future triggers are automatically available.

IF you want to submit a PR which solves #838 in v1 you are very welcome.