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

Allow untyped JSInterop.Setup methods, that matches with any type #361

Open
egil opened this issue Apr 6, 2021 · 6 comments
Open

Allow untyped JSInterop.Setup methods, that matches with any type #361

egil opened this issue Apr 6, 2021 · 6 comments
Labels
enhancement New feature or request
Projects
Milestone

Comments

@egil
Copy link
Member

egil commented Apr 6, 2021

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 single Setup method call.

The suggested solution

Add a new non-generic Setup method that takes a InvocationMatcher as input, and returns new type of handler, with the following signature:

public JSRuntimeInvocationHandler SetResult<TReturnType>(Func<JSRuntimeInvocation, TReturnType> resultFactory);
public JSRuntimeInvocationHandler SetCanceled<TReturnType>();
public JSRuntimeInvocationHandler SetException<TResultType, TException>(Func<JSRuntimeInvocation, TException> exceptionFactory) where TException : Exception

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 the InvokeAsync call.

Additional context

This feature is especially useful for component vendors that want to have a single Setup and SetResult 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 one Setup<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.

@egil egil added the enhancement New feature or request label Apr 6, 2021
@jgoday
Copy link
Contributor

jgoday commented Jul 1, 2021

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

@egil
Copy link
Member Author

egil commented Jul 1, 2021

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.

@linkdotnet
Copy link
Sponsor Collaborator

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 ?

@egil
Copy link
Member Author

egil commented Feb 8, 2022

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.

@linkdotnet
Copy link
Sponsor Collaborator

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.

@egil
Copy link
Member Author

egil commented Feb 9, 2022

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.

@egil egil added this to To do in v2 May 21, 2022
@egil egil added this to the 2.0.0 milestone May 26, 2022
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
No open projects
v2
  
To do
Development

Successfully merging a pull request may close this issue.

3 participants