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

Enable shallow rendering mode #17

Closed
egil opened this issue Dec 16, 2019 · 15 comments
Closed

Enable shallow rendering mode #17

egil opened this issue Dec 16, 2019 · 15 comments
Assignees
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@egil
Copy link
Member

egil commented Dec 16, 2019

The popular shallow rendering mode in Vue/React/Angular testing frameworks, that only render the component under test directly, and not any sub-components it might be using, could be useful in Blazor testing as well.

The TestRenderer and Htmlizer class might allow this. Needs more investigation.

This has been superseded by #396.

@egil egil added enhancement New feature or request help wanted Extra attention is needed labels Dec 16, 2019
@Siphonophora
Copy link
Contributor

@egil I'm happy to try and take this on. Were you thinking altering the html in the component to either remove or hide the sub-components? Removing the component and leaving a div with a css selector to mark where it would have been seems fairly simple.

@egil
Copy link
Member Author

egil commented Jan 22, 2020

That would be awesome.

I think, ideally, with shallow rendering, we completely avoid rendering anything but the the requested components, since child components might be set up to affect it's parents in advanced scenarios.

I am not sure how easy this will be or if it is possible. Might require a custom renderer.

The next best thing would be to just skip any nested components in Htmlizer, which I think is more easily achievable.

@egil
Copy link
Member Author

egil commented Jan 23, 2020

This simple definition of what shallow render is, is what I think we should be aiming at: https://reactjs.org/docs/shallow-renderer.html

However, the shallow rendering should most likely allow a component wrapped in CascadingValue to be rendered, even if it is not at the top level. E.g. the following test setup should not consider the CascadingValue component as the target of the test and not render <MyComponent/>. But I think that might be the only special case, e.g. one or more cascading values.

<Fixture ...>
  <ComponentUnderTest>
    <CascadingValue ...>
      <MyComponent />
    </CascadingValue>
  </ComponentUnderTest>
</Fixture>

@SQL-MisterMagoo

This comment has been minimized.

@egil
Copy link
Member Author

egil commented Jan 23, 2020

Thanks for the suggestion @SQL-MisterMagoo. @Siphonophora I think this might be a doable approach. If you can find something that does not involve reflection, it would be better though.

Anyway, I am reorganizing the project quite a bit for the next release to solve issue #5. Thus, at first, just create a proof of concept for this, dont spend too much time getting this working in the existing project structure.

@SQL-MisterMagoo
Copy link

I hid my comment as further testing showed that was a false positive - nested components didn't work.

If I find a way to make that work, I will unhide or re-post, but for now it is best to ignore that suggestiong.

@SQL-MisterMagoo
Copy link

SQL-MisterMagoo commented Jan 23, 2020

Ok, I'm happier with this as a sample you could work from

protected override void BuildRenderTree(RenderTreeBuilder builder)
{
    // First render out the childcontent to our own RenderTree
    var dummy = new RenderTreeBuilder();
    dummy.AddContent(1, ChildContent);

    // Now get the Append method through reflection because they mean and hid it
    MethodInfo AddFrame = builder.GetType().GetMethod("Append", BindingFlags.Instance | BindingFlags.NonPublic);

    // This will be used to track skipped frames
    int skipTo = 0;

    // Check each frame in the RenderTree
    foreach (var frame in dummy.GetFrames().Array)
    {
        Console.WriteLine($"Got Frame: {frame.Sequence} : {frame.FrameType} : {skipTo}");

        // GetFrames returns at least 16 frames, filled with FrameType None
        // but the renderer doesn't like them, so skip them or get errors
        if (frame.FrameType != RenderTreeFrameType.None)
        {
            // This is how we shallow render - by skipping Component frames
            if (frame.FrameType == RenderTreeFrameType.Component)
            {
                // Set the next sequence we are going to allow to render
                skipTo = frame.Sequence + frame.ComponentSubtreeLength;

                // debug - cos you wanna see it happen
                Console.WriteLine($"Skipping Component with length: {frame.ComponentSubtreeLength}");

                // The renderer doesn't like it if we just drop the frame, so 
                // for now I am replacing it with comment text saying the name of the component
                // but you could do something different
                for (int i = 0; i < frame.ComponentSubtreeLength; i++)
                {
                    builder.AddMarkupContent(frame.Sequence + i, $"<!-- {frame.ComponentType.Name}-{i} -->");
                }
            }
            else
            {
                if (frame.Sequence >= skipTo)
                {
                    // Put the rendered frames we want on the actual RenderTree
                    AddFrame.Invoke(builder, new object[] { frame });
                }
            }
        }
    }
}

@Siphonophora
Copy link
Contributor

@SQL-MisterMagoo's Thanks for the example.

@egil

I would like to start with confirming exactly what the goal is.

  1. In the React example <Subcomponent foo="bar" /> is in the final HTML. So we ideally want to leave the component declarations intact in the same way. That seems like it might be possible, except for c# thats in the component html.
  2. We want to ensure that the subcomponent code never runs, so it can't throw exceptions, raise events, or anything like that. (In the example above, do you know if dummy.AddContent(1, ChildContent) could throw exceptions if there are problems with child components?)

What about subcomponents from the Microsoft.AspNetCore.Components.Forms namespace like EditForm? I'm guessing users may want flexibility on allowing some but not all subcomponents to render, because of these.

I have an idea for another approach, which avoids reflection to get at private methods, but given how new I am to blazor's internals, I wanted to outline it here before trying to implement it.

Briefly, we could create a new 'shallow' component on the fly:

  1. Find the original component.g.cs
  2. Alter the text in the BuildRenderTree method to replace the _builder.OpenComponent with _builder.AddMarkupContent or _builder.OpenElement.
  3. Compile a new dll with the shallow component and call RenderComponent on that .

I'm not sure if the IO and string manipulation required for this would make it more fragile than @SQL-MisterMagoo's idea. The main benefit I see is it relies on the public API. Let me know what you think. I'm happy to pursue either option.

@egil
Copy link
Member Author

egil commented Jan 30, 2020

@Siphonophora thanks for the update.

  1. I think it is worth investigating the benefit of leaving the placeholder for unrendered components in the RenderTree as React does it. I have done very little testing in React/Vue/Angular, where ShallowRender is a thing, so I am not sure of the benefits. It might just be a side effect of how their renderer works. So can you investigate the benefits and report back before we make a decision?

  2. I believe dummy.AddContent(1, ChildContent); actually does a full render, inside the dummy RenderTreeBuilder, but @SQL-MisterMagoo can probably confirm that.

  3. I like the idea of making the shallow render configurable, to e.g. allow certain subcomponents to be rendered as well. Or indeed, if a component is rendered that has a CascadingValue as the top level component, you would never get to the actual target component. So lets investigate what configuration options makes sense (e.g. allow all in namespace X).

  4. I do not think we can depend on having access to the source files. They might be in a different project, or not available at all. Reflection is not a big issue, and we can certain avoid most performance problems by simply caching the result of builder.GetType().GetMethod(...) in a static variable.

@Siphonophora
Copy link
Contributor

@egil,

Sorry about the delay. Ive had a few false starts trying while trying to get a handle on this and want to run by you my current status.

First, I spent some time looking into 1 above, and it looked like those frameworks leave the component declaration in place, but as you say, it sounded like this was a result of how the shallow rendering was accomplished. For one of the frameworks (I forget which) shallow rendering meant the renderer wouldn't know about the child components, and would therefore leave their markup alone. Importantly, none of the articles I read about shallow rendering made any specific points about this as a feature with important benefits. So, I would suggest we just leave <ComponentName/> in the location of a skipped component. Actually trying to reconstruct the .razor markup seems like it would add a lot of complexity, and in some cases wouldn't be possible (e.g.attribues with @ symobols)

In terms of producing a working demo I have had much more trouble than I expected in extending the example @SQL-MisterMagoo produced into a bigger proof of concept. Here are some notes from the last week (which I did using the current master).

  1. I had started off trying to fit @SQL-MisterMagoo's code into ComponentParamenterExtensions.ToComponentRenderFragment. I expected to work with the components here, but after adding the component to the dummy RenderTreeBuilder, I found the component frame's frame.Component property was null. Presumably its a placeholder at this point and needs to be initalized somehow.
  2. If all we need to do is remove the child component markup, that I got working easily in Htmlizer.RenderChildComponent. The downside I see here is the child components still exist.
    1. This can cause exceptions. We could, I think, suppress exceptions originating in the children in TestRenderer.AssertNoSynchronousErrors.
    2. User might need to provide additional cascading parameters to satisfy all the child components.
    3. Worse, the child could have some event callbacks (maybe on a timer) and be changing the state of the parent component.

So, I think #1 is still a better option if it could work, but it looks like it would be difficult. I'm not sure what the best approach is. What do you think?

@egil
Copy link
Member Author

egil commented Mar 11, 2020

No worries @Siphonophora, this is not a small change, and we are all working for free. I am just happy to be getting your help at all!

I agree, we cannot rely on Htmlizer to do shallow rendering, since all components are still rendered, we are just not generating HTML. I also like the idea of leaving in the <ComponentName /> in the render tree to make it obvious that it is not rendered. To make it easier to detect that is not rendered, we could add a special attribute to it, e.g. <ComponentName blazor:renderSkipped="true" />.

Next week I hope to get some time to start working on a refactor, that will result in a bunit.core library, that does not have anything specific to HTML inside it, that will be in a bunit.web library. The idea is to enable other variants, such as bunit.mobile, and also remove direct dependency on xunit. Anyway, the point is, that your shallow rendering should exist in bunit.core. But do go ahead with prototyping, but do not use to much time making things pretty/clean, as I will be moving things around.

@egil egil added the backlog Enhancements which are relevant but not a priority at the moment. label May 6, 2020
@Siphonophora
Copy link
Contributor

Siphonophora commented May 18, 2020

@egil I wanted to update you on some recent progress. Which is here: https://github.com/Siphonophora/bunit/tree/shallow-render2

Its messy, but here is whats happening: I've been using TestRenderTest.Test001 and as you can see there is a component called Shallow. All it does is display child content. Then ShallowBuilder.CreateShallowFragement is called. That ended up being an extention of what sqlmrmagoo produced above, except that you have to do a full render of the component in order to get the frames. That gets us the expected markup:

<button blazor:onclick="1">Click to count</button>
<p>0</p>
<Simple1 blazor:renderSkipped="true" blazor:renderSkippedFrameCount="2"/>
<div></div>

<EditForm blazor:renderSkipped="true" blazor:renderSkippedFrameCount="4"/>

Except the cascading value and button clicking don't work. I think I have successfully severed the markup from the component, but then all the methods and values on the component aren't there to be used. I tried putting the cascading parameter and the click handler on the shallow component to see if that would help, but it didn't make any difference.

I think if I had a deeper understanding of the internals of blazor, this might be going better. I'm having trouble telling if the right combination of RenderTreeFrames could be added to simulate the component still existing, or to point to an instance of the component without using its BuildRenderTree method.

The only other idea I have had is to try and inherit from the component, or maybe proxy calls to it, but substituting a different BuildRenderTree. A quick test of doing that manually provided the same markup as before. But it looks like the cascading value and click events still aren't working

    public class TestRendererTest : ComponentTestFixture
    {
        [Fact(DisplayName = "Renderer pushes render events to subscribers when renders occur")]
        public void Test001()
        {
            var res = new ConcurrentRenderEventSubscriber(Renderer.RenderEvents);
            //var sut = RenderComponent<ChildrenHolder>();

            ShallowClickCounter.Fragement = ShallowBuilder.CreateShallowFragement<ClickCounter>();
            var sut = RenderComponent<ShallowClickCounter>(
                CascadingValue(42));

            File.WriteAllText($@"c:\temp\click counter markup {DateTime.Now.ToString("yyyyMMdd HHmmssffffff")}.txt", sut.Markup);

            res.RenderCount.ShouldBe(1);

            sut.Find("button").Click();
            File.WriteAllText($@"c:\temp\click counter markup {DateTime.Now.ToString("yyyyMMdd HHmmssffffff")}.txt", sut.Markup);

            res.RenderCount.ShouldBe(2);
        }
    }

    public class ShallowClickCounter : ClickCounter
    {
        public static RenderFragment Fragement { get; set; }

        protected override void BuildRenderTree(RenderTreeBuilder __builder)
        {
            Fragement(__builder);
        }
    }

Let me know what you think. Also, I see you marked this as backlog. So, let me know what you think in terms of putting more time into this now.

@egil
Copy link
Member Author

egil commented May 18, 2020

hi @Siphonophora, thanks for your efforts in this. Regarding marking this issue as "backlog", that's just because I don't feel it is a need-to-have feature before going for v1 final release. Maybe I should rename it to nice-to-have :)

As for understanding the internals, I do feel your pain. I've been digging around the the base renderers code, and it's quite complex.

I am not sure the approach you seem to be taking by providing an override to ChildContent will work in all cases, what we really need is a way to hook into the RenderTreeBuilder that is passed to the BuildRenderTree method of each component.

One challenge with that is that each component seems to get its own instance of the RenderTreeBuilder (see ComponentState class), and that means we cannot create a Wrapper component whose child content is the component under test. If they shared a RenderTreeBuilder, we could hack into the one received by the wrapper and add our own logic. And sadly, the RenderTreeBuilder type is also sealed, so it will mean a full copy/paste and a custom Renderer/RenderHandle to hook into the pipeline, as far as I can see. That would probably not be a good long term solution, as we would end up having to copy/paste most of the existing internal renderer code into bUnit.

There could quite likely be other options I am not seeing. We could create an issue in the aspnetcore repo and ask for some input on this. If it is not possible at all with the current renderer architecture, then we might inspire them to add some hook points for us in the future.

@Siphonophora
Copy link
Contributor

@egil I agree, everything I have tried seems fragile in one way or another. I did spend time thinking about #53 in relation to this. Anything that would allow you to substitute a mock should, I think, allow us to do shallow rendering. If there was an option for a component to be specified by an interface then registered DI that would be great.

Asking the aspnetcore team for input on modifying or substituting the component does seem like a good idea. I see a few potentially related open items, like dotnet/aspnetcore#19642 but it won't hurt to ask.

In any case, I'm happy to put some time into another issue that's higher priority for the initial release if you want to suggest one.

@egil
Copy link
Member Author

egil commented May 19, 2020

Awesome. Lets park this for now. Take a look at the beta-8 milestone issues. Those are next in line :-)

@egil egil closed this as completed May 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants