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

Add InvokeAsync overload #177

Merged
merged 10 commits into from
Aug 5, 2020
Merged

Conversation

JeroenBos
Copy link
Contributor

@JeroenBos JeroenBos commented Aug 5, 2020

Fixes #166.

Some remarks:

  • In the issue you mention a refactoring, however it's hard to know for me how exactly you'd like that, so I refrained. Besides a standalone PR may be more suitable.
  • I didn't really know where to place the tests. I can (re)move them if you'd like.
  • The third commit contains breaking changes. I would argue in favor because
    • they could be valuable in async testing scenarios. Tests become simpler.
    • they are in line of expectations of high quality async-handling APIs.

However, I did revert those breaking changes, so consider a squash merge.

PR meta checklist

  • Pull request is targeting at DEV branch.
  • Pull request is linked to all related issues, if any.
  • I have read the CONTRIBUTING.md document.

Content checklist

  • My code follows the code style of this project and AspNetCore coding guidelines.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.
  • I have updated the appropriate sub section in the CHANGELOG.md.
  • I have added, updated or removed tests to according to my changes.
    • All tests passed.

@egil egil self-requested a review August 5, 2020 14:17
@egil egil linked an issue Aug 5, 2020 that may be closed by this pull request
…ase, changed IRenderedFragment to enable converting method to extension methods
@egil
Copy link
Member

egil commented Aug 5, 2020

Hi @JeroenBos

Thanks for this. Here is my initial comments:

  1. Added the PR checklist to your PR comment to ensure we remember all the things...
  2. I pushed the refactorings I was thinking of to this PR. It was basically about depopulating the IRenderedComponentBase interface to make it simpler. Let me know what you think.
  3. I moved your tests to a class matching the new extension class. I do question the need for those tests. Are the actually testing a part of bUnit or Blazors dispatcher? If it is the latter, than I think we should change them or remove them.
  4. About changing Render* method to return a Task - the reason I have not done so before is to keep things simple for folks. For the most parts, the Blazor renderer will just return a Task.Completed and then there is nothing to await. In the case where there is something to await, then you usually need to use cut.WaitForState or cut.WaitForAssertion anyway. There is a related issue Investigate running test code and renderer code in the same thread/sync context #124 to this, which might make this change necessary.

@JeroenBos
Copy link
Contributor Author

I do question the need for those tests.

Yes I question the need for them as well. They were more the result of trying to make a good PR.

In the case where there is something to await, then you usually need to use cut.WaitForState

I'm not clear on the design details for WaitForState, and I trust that it has a proper purpose, but from a simple viewpoint whenever possible I would like to be able to simplify

cut.SetParametersAndRender();
await cut.WaitForState();

to

await cut.SetParametersAndRenderAsync();

which is even simpler for folks.

…n.cs

Co-authored-by: Jeroen Bos <jer123bos@gmail.com>
@egil
Copy link
Member

egil commented Aug 5, 2020

I'm not clear on the design details for WaitForState, and I trust that it has a proper purpose, but from a simple viewpoint whenever possible I would like to be able to simplify

cut.SetParametersAndRender();
await cut.WaitForState();

to

await cut.SetParametersAndRenderAsync();

which is even simpler for folks.

In most cases, calling cut.SetParametersAndRender() is enough, and the user does not need to await using WaitFor* methods.

The WaitFor* methods are only needed when something triggered from the test code causes the renderer to perform multiple renders asynchronously. See more here: https://bunit.egilhansen.com/docs/verification/async-assertion.html

@egil
Copy link
Member

egil commented Aug 5, 2020

I just pushed an update to the docs. I think this is done. You?

@egil egil added the enhancement New feature or request label Aug 5, 2020
CHANGELOG.md Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@egil egil merged commit 87d61a0 into bUnit-dev:dev Aug 5, 2020
@egil
Copy link
Member

egil commented Aug 5, 2020

It's merged. Thanks so much @JeroenBos for taking the lead on this and helping out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add overload IRenderedComponentBase.InvokeAsync(Func<Task>)
2 participants