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.

Sign up for GitHub

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

Jump to bottom

Add a Setup<X>() method to MockJSRuntime #234

Merged
merged 6 commits into from
Oct 26, 2020
Merged

Add a Setup<X>() method to MockJSRuntime #234

merged 6 commits into from
Oct 26, 2020

Conversation

nemesv
Copy link
Contributor

@nemesv nemesv commented Oct 18, 2020 •
edited by egil
Loading

Implements #206

Pull request description

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.

@codecov
Copy link

codecov bot commented Oct 18, 2020 •
edited
Loading

Codecov Report

Merging #234 into dev will decrease coverage by 0.07%.
The diff coverage is 83.78%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #234      +/-   ##
==========================================
- Coverage   81.03%   80.95%   -0.08%     
==========================================
  Files         112      112              
  Lines        3395     3419      +24     
  Branches      430      432       +2     
==========================================
+ Hits         2751     2768      +17     
- Misses        515      520       +5     
- Partials      129      131       +2
Impacted Files Coverage Δ
...estDoubles/JSInterop/JSRuntimePlannedInvocation.cs 77.27% <68.75%> (-22.73%) ⬇️
...estDoubles/JSInterop/MockJSRuntimeInvokeHandler.cs 96.51% <95.00%> (-3.49%) ⬇️
...oubles/JSInterop/JSRuntimePlannedInvocationBase.cs 84.00% <100.00%> (+0.12%) ⬆️
...it.core/Extensions/WaitForHelpers/WaitForHelper.cs 90.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3528e90...0ab78e6. Read the comment docs.

@egil
Copy link
Member

egil commented Oct 18, 2020

Hi @nemesv,

Thanks for this, it's much appreciated. I'll take a look at this ASAP one of the coming days and provide feedback.

egil
egil requested changes Oct 19, 2020
View reviewed changes
Copy link
Member

@egil egil left a comment

Choose a reason for hiding this comment

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

Hi @nemesv

Great work. It seems you covered all the test cases!

I added a few comments in the code below. Besides those, I wonder if we should also add a SetupVoid() variant?

Let me know what you think.

src/bunit.web/TestDoubles/JSInterop/MockJSRuntimeInvokeHandler.cs Outdated Show resolved Hide resolved
src/bunit.web/TestDoubles/JSInterop/MockJSRuntimeInvokeHandler.cs Outdated Show resolved Hide resolved
@nemesv
Copy link
Contributor Author

nemesv commented Oct 19, 2020

I am not sure about the SetupVoid() variant. In which scenario would someone use it?
Isn't it is just the same behavior of JSRuntimeMockMode.Loose?

@egil
Copy link
Member

egil commented Oct 19, 2020

I am not sure about the SetupVoid() variant. In which scenario would someone use it?
Isn't it is just the same behavior of JSRuntimeMockMode.Loose?

SetupVoid() is parallel to InvokeVoidAsync, e.g. a call that should not return anything. That would not match InvokeAsync<string>, so I think it has value and is not just like "loose" behavior.

@egil egil linked an issue Oct 20, 2020 that may be closed by this pull request
MockJSRuntime: Setup<X>() without parameters and identifier to match all with type X #206
Closed
@egil
Copy link
Member

egil commented Oct 23, 2020

Hey @nemesv

Sorry, I completely missed that you pushed some changes. I will review ASAP tonight.

@egil egil self-requested a review October 23, 2020 16:44
@nemesv
Copy link
Contributor Author

nemesv commented Oct 23, 2020 •
edited
Loading

No worries :) In the meanwhile I've finished and pushed the implementation of the suggested SetupVoid(), so you can also review that.

egil
egil approved these changes Oct 23, 2020
View reviewed changes
Copy link
Member

@egil egil left a comment

Choose a reason for hiding this comment

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

Looks great. I pushed a tweak the JSRuntimePlannedInvocation to remove an unneeded class in the hierarchy... thought it easier to tweak myself then try to explain 🙂

Anyway, last thing on the checklist is adding a small extra example/section to the documentation. Are you up for that?

@nemesv
Copy link
Contributor Author

nemesv commented Oct 23, 2020

Sure, I can update the documentation tomorrow. Have you meant this page https://github.com/egil/bUnit/blob/main/docs/site/docs/test-doubles/mocking-ijsruntime.md?

@egil
Copy link
Member

egil commented Oct 23, 2020

Sure, I can update the documentation tomorrow. Have you meant this page https://github.com/egil/bUnit/blob/main/docs/site/docs/test-doubles/mocking-ijsruntime.md?

Awesome.

Correct, that's the page. Do check out the readme in the docs folder for instructions for writing docs.

egil
egil approved these changes Oct 26, 2020
View reviewed changes
Copy link
Member

@egil egil left a comment

Choose a reason for hiding this comment

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

Thank for the docs update. Looks good to me.

@egil egil merged commit 928fda7 into bUnit-dev:dev Oct 26, 2020
@egil
Copy link
Member

egil commented Oct 26, 2020

Thank you very much for the effort @nemesv. Let me know if you want to help out with others issues. I can always use skilled coders on bUnit!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@egil egil egil approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

MockJSRuntime: Setup<X>() without parameters and identifier to match all with type X
2 participants
@nemesv @egil