Add default JsRuntime by Siphonophora · Pull Request #32 · bUnit-dev/bUnit

Siphonophora · 2020-01-12T23:42:01Z

Hi. This should resolve #26.

egil · 2020-01-13T08:57:56Z

Hi @Siphonophora, thanks for this. I really appreciate you spending your time on this.

I will look over the code and review on Wednesday. I am busy these days preparing for .net conf tomorrow, so time is limited.

Siphonophora · 2020-01-13T16:07:53Z

@egil sounds good. I saw the promo for .net conf the other day, which is actually what reminded me that i wanted to check out this project.

egil · 2020-01-15T09:30:12Z

Hi @Siphonophora,

I just took a quick look through your code. When I originally added the related issue, my thought was simply to have a placeholder class, that when instantiated by the DI container, would throw an exception with a helpful exception message. My thinking was that it would stop a test as soon as a component was being rendered the first time.

Now I am thinking that maybe your approach is better, since it will allow components with a jsruntime dependency to be rendered and tested, as long as the logic being tested doesn't involve calling the jsruntime.

Was this something you considered?

egil · 2020-01-15T09:13:32Z

+using System.Threading.Tasks;
+using Microsoft.JSInterop;
+
+[assembly: InternalsVisibleTo("Egil.RazorComponents.Testing.Library.Tests")]


Lets move this to an Assembly.cs file at the root of the project.

egil · 2020-01-15T09:16:38Z

+    /// <summary>
+    /// This JsRuntime is used to provide users with helpful exceptions if they fail to provide a mock when required. 
+    /// </summary>
+    internal class DefaultJsRuntime : IJSRuntime


So I know I used the word "default" in the issue related to this, but I do not think default is the right word here. We need a word that indicates that this JsRuntime is actually not usable, and just a placeholder for something else that implements IJSRuntime.

I will think about naming more. At least since its internal the users won't be confused.

What do you think about placeholder?

I noticed I started using that word in the comments,

That sounds good, should be clear its not a real version.

egil · 2020-01-15T09:21:00Z

+            var ex =  new Exception(MissingJsRuntimeMessage);
+            ex.HelpLink = MissingJsRuntimeHelpLink;
+            ex.Data.Add("identifier", identifier);
+            ex.Data.Add("args", args);
+            throw ex;


Very good idea to use the HelpLink property in exceptions to point to the wiki.

However, all of this needs to be moved to its own custom exception type, such that the InvokeAsync methods simply throw that.

Question: what was your thinking around passing args to the exception? Do you think it will be usable to the users?

I can add a new type.

I was thinking that someone who didn't expect their test to invoke any JS might be aided if we provided everything we know about the invocation. Maybe its low value, but it was easy so I threw it in.

Still, when I was trying to debug into this to see the args, I ran into issues because of the aggregate exception logic. If users are going to have this issue, I would probably just move the identifier into the exception message.

I think adding the identifier to exception message is a good idea.

Also, in the custom exception, you can simply add properties for the identifier and args, such that they are easily accessible.

Siphonophora · 2020-01-16T18:23:09Z

Hi @egil,

I thought this style of implementation was what you were thinking about, so I didn't look at throwing exceptions whenever the @inject IJSRuntime is present. Personally I would prefer the option where I only have to supply the mock if it is used, just because it will make the tests a little cleaner.

I should be able to make changes this weekend.

egil · 2020-01-16T18:25:17Z

Great, thanks. I like your solution better too.

Btw. change your merge target to the DEV branch please.

Co-Authored-By: Egil Hansen <egil@assimilated.dk>

Siphonophora · 2020-01-20T16:47:15Z

@egil This should be all set.

egil · 2020-01-20T20:22:00Z

Thanks. I'll take a look later tonight or tomorrow morning.

egil

Hi again Michael

Thanks for the effort. I have a few small changes I would like, but you should be able to apply them directly from GitHub except one.

When they are out of the way, ill merge your PR.

Thanks again.

egil · 2020-01-20T21:47:54Z

+        private static string PrintArguments(IReadOnlyList<object> arguments)
+        {
+            return string.Join(", ", arguments.Select(x => x.ToString()));
+        }


This is not used.

Removed this.

Co-Authored-By: Egil Hansen <egil@assimilated.dk>

* Fixed spelling mistake in comment * Reordered parameters to top of TestRenderer * Add support for asynchronous Razor tests (#27) * Add support for asyncrhonous Razor tests * Rename Fixture.AsyncTest to TestAsync * Update description of Fixture Co-Authored-By: Egil Hansen <egil@assimilated.dk> * Dotnetconf samples (#33) * Basic example tests * Add missing MarkupMatches overload * Added docs to MockHttp extensions * dotnet conf samples * Added unit tests of CompareTo, Generic and Collection assert extensions. * Added tests for general events and touch events dispatch extensions * Added tests for anglesharp extensions, JsRuntimeInvocation and ComponentParameter * Removed assert helpers that conflict with Shoudly * Tests of JsRuntimeAsserts * Reorganized test library * Suppressing warnings in sample * Changed ITestContext to have a CreateNodes method instead of HtmlParser property * Removed empty test * Added missing code documentation * Moved MockJsRuntime to its own namespace * Pulled sample from main solution into own solution * Update main.yml * Change GetNodes and GetMarkup to Nodes and Markup properties in IRenderedFragment (#34) * Add SetupVoid and SetVoidResult capabilities to JsRuntime mock (#35) * Moved MockJsRuntime to its own namespace * Add SetupVoid() and SetVoidResult() to PlannedInvocation and Mock * Updates to template and sample * Add default JsRuntime (#32) * Add default JsRuntime * Update src/Mocking/JSInterop/DefaultJsRuntime.cs Co-Authored-By: Egil Hansen <egil@assimilated.dk> * Response to review. Add custom exception and code cleanup * Remove unneded method * Update src/Mocking/JSInterop/MissingMockJsRuntimeException.cs Co-Authored-By: Egil Hansen <egil@assimilated.dk> * Update src/Mocking/JSInterop/MissingMockJsRuntimeException.cs Co-Authored-By: Egil Hansen <egil@assimilated.dk> * Update src/Mocking/JSInterop/MissingMockJsRuntimeException.cs Co-Authored-By: Egil Hansen <egil@assimilated.dk> * Update src/Mocking/JSInterop/MissingMockJsRuntimeException.cs Co-Authored-By: Egil Hansen <egil@assimilated.dk> * Update src/Mocking/JSInterop/MissingMockJsRuntimeException.cs Co-Authored-By: Egil Hansen <egil@assimilated.dk> * Update src/Mocking/JSInterop/MissingMockJsRuntimeException.cs Co-Authored-By: Egil Hansen <egil@assimilated.dk> Co-authored-by: Egil Hansen <egil@assimilated.dk> * Add .vscode to gitignore * TestServiceProvider now explictly implements IServiceCOlelction (#40) * TestServiceProvider now explictly implements IServiceCOlelction * Tweaks to namespaces * Added async setup method to snapshot test * Added test of SnapshotTests use of setup methods * Update to readme * Removed duplicated MarkupMatches method * Removed PR trigger Co-authored-by: Rastislav Novotný <duracellko@outlook.sk> Co-authored-by: Michael J Conrad <32316111+Siphonophora@users.noreply.github.com>

Add default JsRuntime

403f025

egil self-requested a review January 13, 2020 08:56

egil requested changes Jan 15, 2020

View reviewed changes

Siphonophora and others added 2 commits January 16, 2020 13:31

Update src/Mocking/JSInterop/DefaultJsRuntime.cs

653fb5a

Co-Authored-By: Egil Hansen <egil@assimilated.dk>

Response to review. Add custom exception and code cleanup

89ad90b

Siphonophora changed the base branch from master to dev January 20, 2020 16:43

Merge branch 'dev' into Add-default-jsruntime

116d0e0

egil requested changes Jan 20, 2020

View reviewed changes

Siphonophora and others added 7 commits January 21, 2020 21:47

Remove unneded method

ca2af8a

Update src/Mocking/JSInterop/MissingMockJsRuntimeException.cs

f784b35

Co-Authored-By: Egil Hansen <egil@assimilated.dk>

Update src/Mocking/JSInterop/MissingMockJsRuntimeException.cs

2b6f92a

Co-Authored-By: Egil Hansen <egil@assimilated.dk>

Update src/Mocking/JSInterop/MissingMockJsRuntimeException.cs

0584252

Co-Authored-By: Egil Hansen <egil@assimilated.dk>

Update src/Mocking/JSInterop/MissingMockJsRuntimeException.cs

6f8d735

Co-Authored-By: Egil Hansen <egil@assimilated.dk>

Update src/Mocking/JSInterop/MissingMockJsRuntimeException.cs

6bd0a53

Co-Authored-By: Egil Hansen <egil@assimilated.dk>

Update src/Mocking/JSInterop/MissingMockJsRuntimeException.cs

4e7ea27

Co-Authored-By: Egil Hansen <egil@assimilated.dk>

egil approved these changes Jan 22, 2020

View reviewed changes

egil merged commit 7411c68 into bUnit-dev:dev Jan 22, 2020

Siphonophora deleted the Add-default-jsruntime branch January 22, 2020 14:46

Uh oh!

Uh oh!

Conversation

Siphonophora commented Jan 12, 2020

Uh oh!

egil commented Jan 13, 2020

Uh oh!

Siphonophora commented Jan 13, 2020

Uh oh!

egil commented Jan 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Siphonophora commented Jan 16, 2020

Uh oh!

egil commented Jan 16, 2020

Uh oh!

Siphonophora commented Jan 20, 2020

Uh oh!

egil commented Jan 20, 2020

Uh oh!

egil left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

egil commented Jan 15, 2020 •

edited

Loading