Open egil opened 3 years ago
Hi @egil, Could I take a look at it?
JSRuntimeInvocationHandler could be called JSRuntimeInvocationHandlerVoid and this new one JSRuntimeInvocationHandler, or something like JSRuntimeInvocationHandlerFactory, who is constructed from a factory function from BunitJSInteropSetupExtensions.Setup
Hey @jgoday
You can absolutely take a look.
JSRuntimeInvocationHandler could be called JSRuntimeInvocationHandlerVoid and this new one JSRuntimeInvocationHandler, or something like JSRuntimeInvocationHandlerFactory, who is constructed from a factory function from BunitJSInteropSetupExtensions.Setup
I have not looked at this for a while, so I cannot remember how I was thinking on this. I am moving to a different city in a week, so Im very busy these days, but I hope I get a chance to give you better feedback later. However, if you want, feel free to come up with a proposal and post as much detail as you can.
Hey Egil, I hope you are doing well. I just had a look at the issue and the linked PR. And that raised some questions regarding the functionality. As a refresher I am referring to this comment from you.
Especially that scenario: var handler = Setup(i => i.Identifier.StartsWith("myJsLib"));
and the resulting issue between Setup
and Set[Result|Exception|Cancelled]
. I am not sure if we should allow such things in the first place (and what would be a appriopriate use case). .NET Mock-libraries will not allow such things (like Moq or Substitute). And I guess because of those implications you have shown. Furthermore it will make the test less maintainable and readable... besides bUnit code to handle that in the first place.
Therefore I'd recommend that we have kind of 1 Setup <-> 1 SetXXX
.
Now we could have something like Moq (which you described in the PR) but without the invocationmatcher.
public T SetupResult(Func<T> resultFunc);
This would give the flexibility to return different stuff. Or if you need sequences we could go with the Moq approach as well.
Of course this would result in returning object
.
Anyway I guess we could go with a simple approach first and check if it fulfills the needs of the user. Wildcard can be discussed later on. What do you think @egil ?
Hi Steven, thanks, hope you are well too.
This issue might be one of those nice to have on the surface, but not worth the effort, because few users actually have a need. There are certainly some open questions that we need to consider before doing a second attempt at this.
Aside: Some parts of me is a bit sad that I ended up creating my own tailormade mocking lib for this particular purpose, but it has turned out to be an advantage since bUnit can ship with builtin JSInterop handlers for the first party components. That said, I am inclinded to look at an abstraction for version 2 that pulls out bUnits JSInterop core, and instead just exposes an abstraction that Moq/NSubstitute/JustMock etc. can be integrated with, such that users can pick their own favorite and just use that.
Hey. Doing well ;)
From an API "use point of view" I would prefer SetResult<T>
or Returns<T>
more than specifying the return value when setting up the "mock" (JSinterop.Setup<T>("").SetResult()
. Guess that feels more natural for me as the major mocking frameworks behave like that.
Anyway I like your idea to offer an abstraction or a way to mock IJSInterop
. Internally there will be still plenty of use for BunitJSInterop
as more and more Blazor components relay on JavaScript in the first place.
Besides: Folks can provide their own IJSRuntime
mock even today, can't they? I could just add another IJSRuntime
via Services.AddScoped(_ => new Mock<IJSRuntime>().Object);
and this should override the pre-registered one?
Only downside today is that you can't directly mock IJSRuntime.InvokeVoidAsync()
as this is an extension method so you have to do something like jsMock.Setup(j => j.InvokeAsync<IJSVoidResult>("identifier", "arg1")).ReturnsAsync("Hello World")
.
Maybe that would be also a nice starting point.
Anyway if their are some settled ideas, I am glad to help.
I think we might revisit this for v2. There is definitely a good case for having a purpose built abstraction/mock of IJSRuntime for bUnit that enables setting up things in a language/api that feels natural to IJSRuntime API. However, changes like that is very likely to be breaking, so lets float some ideas around towards V2.
A generic
Setup(InvocationMatcher)
method is needed, that does not specify the return type at compile time is needed, to make it easy for folks to set up a "catch all of my calls" in a singleSetup
method call.The suggested solution
Add a new non-generic
Setup
method that takes aInvocationMatcher
as input, and returns new type of handler, with the following signature:This
JSRuntimeInvocationHandler
could serve as a base class for all other handler types, as it is very generic and can be used in all cases.The
Set*
methods allow the user to set results based on the return type of theInvokeAsync
call.Additional context
This feature is especially useful for component vendors that want to have a single
Setup
andSetResult
call in their helper libraries for bUnit. This will allow them to easily ignore all calls made to their JavaScript code. Currently, they have to add oneSetup<T>(...).SetResult(..)
call per type their JavaScript library returns. With this addition, they will be able to create a more generic solution that uses factory methods to do most of the work.