bUnit-dev / bUnit

bUnit is a testing library for Blazor components that make tests look, feel, and runs like regular unit tests. bUnit makes it easy to render and control a component under test’s life-cycle, pass parameter and inject services into it, trigger event handlers, and verify the rendered markup from the component using a built-in semantic HTML comparer.
https://bunit.dev
MIT License
1.13k stars 105 forks source link

Change Uri in NavigationManager from async method in component is not always visible in test #445

Closed OuttaSpaceTime closed 3 years ago

OuttaSpaceTime commented 3 years ago

Describe the bug

I have a OnValidSubmit which triggers the HandleValidSubmit method. This updates my data and afterwards triggers my navigationmanager to redirect. When I test it within browser redirection works fine but in my test it returns navMan.Uri => "http://localhost/"

Example: Testing this component:

@page "/Angebot-erstellen"

<div class="container">
    <div class="row">
        <div class="col-md-12">
            <div class="card shadow mb-3">
                <div class="card-header py-3">
                    <h4 class="m-0 font-weight-bold">Angebot erstellen</h4>
                                <EditForm Model="@_currProposal" OnValidSubmit="@HandleValidSubmit">
                                        <label class="h4 mt-2">Titel</label><InputText @bind-Value="_currProposal.Title" class="form-control" type="text" required="" minlength="3"/><label class="h4 mt-2">Beschreibung</label><InputTextArea @bind-Value="_currProposal.Description" class="form-control" style="margin-top: 11px;" required="" minlength="10" rows="5"/><label class="h4 mt-2">Branche</label>
                                        <InputSelect @bind-Value="_currProposal.Field" class="form-control form-control-sm custom-select custom-select-sm h5">
                                            <option value="0" selected="">Bitte Branche auswählen...</option>
                                            <option value="Architektur/Bau/Immobilien">Architektur/Bau/Immobilien</option>
                                            <option value="Automobil">Automobil</option>
                                            <option value="Banken/Finanzsektor/Versicherung">Banken/Finanzsektor/Versicherung</option>
                                            <option value="Beratung">Beratung</option>
                                            <option value="Bildung">Bildung</option>
                                            <option value="Chemie">Chemie</option>
                                            <option value="Dienstleistung">Dienstleistung</option>
                                            <option value="Druck/Verpackung">Druck/Verpackung</option>
                                            <option value="IT">IT</option>
                                            <option value="Elektro/Elektronik">Elektro/Elektronik</option>
                                            <option value="Energie">Energie</option>
                                            <option value="Forschung/Entwicklung">Forschung/Entwicklung</option>
                                            <option value="Gesundheit/Soziales/Pflege">Gesundheit/Soziales/Pflege</option>
                                            <option value="Kunst/Kultur">Kunst/Kultur</option>
                                            <option value="Landwirtschaft/Nahrungsmittel">Landwirtschaft/Nahrungsmittel</option>
                                            <option value="Logistik/Transport/Verkehr">Logistik/Transport/Verkehr</option>
                                            <option value="Luft- und Raumfahrt">Luft- und Raumfahrt</option>
                                            <option value="Marketing/Werbung/PR">Marketing/Werbung/PR</option>
                                            <option value="Maschinen- und Anlagenbau">Maschinen- und Anlagenbau</option>
                                            <option value="Medizin/Pharma">Medizin/Pharma</option>
                                            <option value="Medizintechnik">Medizintechnik</option>
                                            <option value="Optik">Optik</option>
                                            <option value="Robotik">Robotik</option>
                                            <option value="Sport">Sport</option>
                                            <option value="Steuerberatung/Wirtschaftsprüfung">Steuerberatung/Wirtschaftsprüfung</option>
                                            <option value="Tourismus">Tourismus</option>
                                            <option value="Gastronomie">Gastronomie</option>
                                        </InputSelect>
                                        <div class="p-1 text-right mb-0">
                                            <button style="background: #61a48c;color: rgb(255,255,255);" type="submit" >Next<i class="oi oi-arrow-right"></i></button>
                                        </div>
                                    </EditForm>
                </div>
        </div>
    </div>
</div></div>

using System;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Components;
using Team01.Models;
using Team01.Services.Interfaces;

namespace Team01.Pages
{

    public partial class CreateProposal
    {
        private Proposal _currProposal;
        [Inject] private IProposalService ProposalService { get; set; }
        [Inject] private NavigationManager NavigationManager { get; set; }

        protected override async Task OnInitializedAsync()
        {
            _currProposal = new Proposal()
            {
                DateCreated = DateTime.Now,
            };
        }

        private async Task HandleValidSubmit()
        {
            await ProposalService.UpdateProposalAsync(_currProposal);
            NavigationManager.NavigateTo("Angebot-erstellen/skills");
        }

    }
}

With this test:

  [Test]
        public async Task CreateProposal_SuccsessfullyNavigatesToSkillPage()
        {
            using var ctx = new Bunit.TestContext();
            ctx.Services.AddSingleton<IProposalService>(_proposalService);
            var navMan = ctx.Services.GetRequiredService<NavigationManager>();

            var cut = ctx.RenderComponent<CreateProposal>();

            cut.Find("input").Change("TestTitle");
            cut.Find("textarea").Change("A proposal description");
            cut.Find("select").Change("Beratung");
            cut.Find("form").Submit();

            Assert.AreEqual($"{navMan.BaseUri}Angebot-erstellen/skills", navMan.Uri);

        }

Results in this output:

  Expected string length 41 but was 17. Strings differ at index 17.

Expected behavior: it should go to the desired link like it does within in the browser.

Version info:

Additional context: Once I add an onclick to the button and trigger the nav manager here it works as expected.

within test:

            cut.Find("form").Submit();
            cut.Find("button").Click();

            Assert.AreEqual($"{navMan.BaseUri}Angebot-erstellen/skills", navMan.Uri);

and html:

<button style="background: #61a48c;color: rgb(255,255,255);" @onclick=@(() => NavigationManager.NavigateTo("/Angebot-erstellen/skills")) type="submit" >Next<i class="oi oi-arrow-right"></i></button>
egil commented 3 years ago

Hi @FelixOutta

Does the line in await ProposalService.UpdateProposalAsync(_currProposal) ever return?

And if it does, does it do so immediately with a Task.CompletedTask or something that is really async?

If it's the latter then you might want to either change ProposalService to be something that returns a completed task, or, if you can't, then wrap your assertion in cut.WaitForAssertion.

More details on async code in tests here: https://bunit.dev/docs/verification/async-assertion

OuttaSpaceTime commented 3 years ago

Hi @egil

Thanks for having a look at the issue! it returns a task and updates data async. it actually does return since the following test passes without any problem:

  [Test]
        public async Task CreateProposal_SuccsessfullyCreatesProposalData()
        {
            using var ctx = new Bunit.TestContext();
            ctx.Services.AddSingleton<IProposalService>(_proposalService);
            var cut = ctx.RenderComponent<CreateProposal>();

            cut.Find("input").Change("TestTitle");
            cut.Find("textarea").Change("A proposal description");
            cut.Find("select").Change("Beratung");
            cut.Find("form").Submit();

           Assert.AreEqual( new Proposal() {Id = 1, Description = "A proposal description", Title = "TestTitle", Field = "Beratung" }, 
               DataHelper.QueryData<Proposal>("Proposal", 1, _config)
               );
        }

this is what my service looks like:

public async Task UpdateProposalAsync(Proposal proposal)
        {            
            var parameters = new {Id = proposal.Id, Title = proposal.Title};
            string sql = null;
            var sql_query = "SELECT * FROM proposal WHERE id = @Id";
            var queriedProposal = DbConnection_second.QuerySingleOrDefault<Proposal>(sql_query, parameters);
            if (queriedProposal != null)
            {
                sql = "update proposal SET Title = @Title, Field = @Field, Description = @Description, DateCreated = @DateCreated where id = @Id";
            }
            else
            {
                sql = "insert into proposal (Title, Field, Description, DateCreated) values (@Title, @Field, @Description, @DateCreated)";
            }
            var result = await DbConnection.ExecuteAsync(sql, proposal);

        }

I would prefer to keep it async though turning it to synchronous wouldn't be that much of a deal here I guess. The other workaround I posted works as well, just putting the navMan into @onclick for the button though it doesn't feel as clean. Do you see any drawbacks with this approach? I tried to use assertion as you mentioned:

cut.WaitForAssertion(() => Assert.AreEqual($"{navMan.BaseUri}Angebot-erstellen/skills", navMan.Uri));

Though it still gives the same error.

egil commented 3 years ago

That's a bit weird. Can you try changing the HandleValidSubmit to look like this:

private Task HandleValidSubmit()
{
    NavigationManager.NavigateTo("Angebot-erstellen/skills");
    return ProposalService.UpdateProposalAsync(_currProposal);
}

Another thing you can look at, assuming ProposalService.UpdateProposalAsync takes more than one second, is to increase the timeout:

cut.WaitForAssertion(() => Assert.AreEqual($"{navMan.BaseUri}Angebot-erstellen/skills", navMan.Uri), TimeSpan.FromSecond(5));

You have already confirmed that the navigation manager correctly updated the url when its called, so there is something going on specifically in HandleValidSubmit that misbehaves.

OuttaSpaceTime commented 3 years ago

Changing the HandleValidSubmit like you posted, made the test pass! thank you. any idea what was going on here?

egil commented 3 years ago

I might have.

Can you verify that the NavigationManager.NavigateTo("Angebot-erstellen/skills"); line is actually hit during testing when the method is in the original order? E.g. set a breakpoint on it and run the tests in debug mode.

OuttaSpaceTime commented 3 years ago

it actually hits the line

egil commented 3 years ago

OK, then it might be a synchronization issue between different CPU cores. When we have components doing async work, its runs in a different thread than the test runs in. If those two threads runs on different CPU cores on your computer, then the updated URL string might get stuck in the cache of one cpu core and not be visible on the other CPU.

It will take some experimentation to figure out how to trigger that sync. Usually a lock statement around the Uri property will help. https://source.dot.net/#Microsoft.AspNetCore.Components/NavigationManager.cs is the real implementation which we might need to copy more from instead of the more simple fake there is right now.

egil commented 3 years ago

@FelixOutta, can I get you to create a minimal example that reproduces this, e.g. a component that just have a button which when clicked calls an async operation and then a navMan, and see if that also breaks on your computer? The async operation should be as simple as possible, e.g. Task.Delay(1).

OuttaSpaceTime commented 3 years ago

I will try to do it though got a lot going on due to exams. could I simplify my code for this purpose as well?

egil commented 3 years ago

I will try to do it though got a lot going on due to exams.

No worries, appreciate the help, since these issues can be hard to replicate on other machines with different processors/OS's.

could I simplify my code for this purpose as well?

Yes. In essens, replace the content of your UpdateProposalAsync(Proposal proposal) with return Task.Delay(1) and see if it still has the problem.

OuttaSpaceTime commented 3 years ago

I have to change it like this so it is working:

 public async Task UpdateProposalAsync(Proposal proposal)
        {
            await Task.Delay(1);
        }

and changed the method to:

 private async Task HandleValidSubmit()
        {
            await ProposalService.UpdateProposalAsync(_currProposal);
            NavigationManager.NavigateTo("Angebot-erstellen/skills");

        }

When I run the test now it is basically the same erorr:

Expected string length 41 but was 17. Strings differ at index 17.

egil commented 3 years ago

I have to change it like this so it is working:

 public async Task UpdateProposalAsync(Proposal proposal)
        {
            await Task.Delay(1);
        }

and changed the method to:

 private async Task HandleValidSubmit()
        {
            await ProposalService.UpdateProposalAsync(_currProposal);
            NavigationManager.NavigateTo("Angebot-erstellen/skills");

        }

When I run the test now it is basically the same erorr:

Expected string length 41 but was 17. Strings differ at index 17.

Thanks. Ill file this a bug for now. However, I like won't have time to fix this before after the summer, but if you want to take a stab at it, I can probably find time to do a PR review.

egil commented 3 years ago

I think this might be an easy fix actually.

If the line Uri = ToAbsoluteUri(uri).ToString(); is copied inside the renderer.Dispatcher.InvokeAsync... lambda, then the assignment should happen on the right thread as well as the test thread.

I'm out camping now, so can I get you to try this out locally @FelixOutta and see if it fixes this for you?

        /// <inheritdoc/>
        protected override void NavigateToCore(string uri, bool forceLoad)
        {
            Uri = ToAbsoluteUri(uri).ToString();

            renderer.Dispatcher.InvokeAsync(
                () => 
                {
                Uri = ToAbsoluteUri(uri).ToString();
                  NotifyLocationChanged(isInterceptedLink: false);
                });
        }
OuttaSpaceTime commented 3 years ago

@egil I am not sure how to use it, can you further exemplify?

egil commented 3 years ago

@FelixOutta, create a FelixFakeNavigationManager class in your project, that looks like this:


using System;
using System.Diagnostics.CodeAnalysis;
using Bunit.Rendering;
using Microsoft.AspNetCore.Components;

namespace Felix
{
    public sealed class FelixFakeNavigationManager: NavigationManager
    {
        private readonly ITestRenderer renderer;

        public FakeNavigationManager(ITestRenderer renderer)
        {
            this.renderer = renderer ?? throw new ArgumentNullException(nameof(renderer));
            Initialize("http://localhost/", "http://localhost/");
        }

        protected override void NavigateToCore(string uri, bool forceLoad)
        {
            Uri = ToAbsoluteUri(uri).OriginalString;

            renderer.Dispatcher.InvokeAsync(() => 
            {
                Uri = ToAbsoluteUri(uri).OriginalString;
                NotifyLocationChanged(isInterceptedLink: false);
            });
        }
    }
}

Then, in your test, do Services.AddSingleton<NavigationManager, FelixFakeNavigationManager>() to replace the built-in FakeNavigationManager with the one above.

egil commented 3 years ago

Hi @FelixOutta, if you get the latest preview from nuget.org, that includes the proposed fix (in the built-in FakeNavigationManager). Let me know if it solves the issue for you.

OuttaSpaceTime commented 3 years ago

@egil I just tried, everything looks good so far. thanks for coming up with new solutions :)

egil commented 3 years ago

Great. I'll close this and push the next release out soon. Thanks for helping out with this.