Occasional "Collection was modified"

[anddrzejb]

In a number of my tests I occasionally get this exception:

 AntDesign.Tests.MultiRangeSlider.MultiRangeSlider_Moving_Mouse_Tests.Move_by_pushing_if_reached_to_attached_range_other_edgewhen_AllowOverlap_and_attach_with_gap
   Source: MultiRangeSlider.Moving.Mouse.Tests.razor line 389
   Duration: 30 ms

  Message: 
System.InvalidOperationException : Collection was modified; enumeration operation may not execute.

  Stack Trace: 
Enumerator.MoveNext()
Renderer.Dispose(Boolean disposing)
TestRenderer.Dispose(Boolean disposing) line 205
Renderer.Dispose()
Renderer.DisposeAsync()
ServiceProviderEngineScope.<DisposeAsync>g__Await|15_0(Int32 i, ValueTask vt, List`1 toDispose)
TestServiceProvider.Dispose() line 114
TestContextBase.Dispose(Boolean disposing) line 93
TestContextBase.Dispose() line 72
AntDesignTestBase.Dispose() line 31
ReflectionAbstractionExtensions.DisposeTestClass(ITest test, Object testClass, IMessageBus messageBus, ExecutionTimer timer, CancellationTokenSource cancellationTokenSource) line 79

The problem is that it does not happen every time. More than not, my tests are passing. Only occasionally, I get this error. Always in the tests for the same component, but that might not be related, as the component in question has the most tests. In this component tests, I register js mocks in the constructor like this:

    public MultiRangeSlider_Moving_Mouse_Tests()
    {
        JSInterop.Setup<OverlayPosition>(JSInteropConstants.OverlayComponentHelper.AddOverlayToContainer, _ => true)
            .SetResult(new OverlayPosition());
        JSInterop.Setup<OverlayPosition>(JSInteropConstants.OverlayComponentHelper.UpdateOverlayPosition, _ => true)
            .SetResult(new OverlayPosition());
        JSInterop.Setup<string>(JSInteropConstants.AddDomEventListener, _ => true)
            .SetResult("");
        JSInterop.Setup<HtmlElement>(JSInteropConstants.GetDomInfo, _ => true)
            .SetResult(_sliderDimensions);
#if !NET6_0_OR_GREATER
        JSInterop.SetupVoid(JSInteropConstants.Focus, _ => true);
#endif
    }

I have a feeling I am doing something wrong, but I cannot spot the mistake.

[egil]

Interesting. If I read that stack trace right, the exception is coming from Blazors renderer. My guess is that it happens because your component under test is doing async work and is still adding components to the render tree when the test exits the test method.

This might help hint at a solution: https://bunit.dev/docs/interaction/awaiting-async-state.html

[anddrzejb]

I actually am using WaitForState quite extensively in the tests that get the exception. So let me describe in more detail how the component works & also show you one of the tests that sometimes fails.

That component I am trying to test has 2 types of events - blazor and js. By blazor events I mean things like @onmousedown=OnMouseDown. Js events are the interop callbacks from javascript element.addEventListener("mouseup", callback). During some of my tests in a single test I actually need to call blazor event and then immediately after the js callback. For the tests in question, I need to issue mousedown and then mousemove (the test is checking if my dragging simulation is working correct - this is not drag & drop so I cannot really use drag&drop hooks here). However:

• mouse down is a blazor event on a particular element (beautifully executed by bUnit)
• mouse move is a js event on the whole window, so I can still react to the movement and not force the user to be exactly on the element. Normally I probably would catch the callback using JSInterop, but in case of AntBlazor, I use Moq to do it. Not to go too deep into the details, to avoid re-registering certain events multiple times (like for example mousemove on window), there is an internal service that stores "shared" events & callbacks and fire the callbacks when appropriate event is being called from js.

The test is basically testing this type of dragging:

You will notice that once the mouse is pressed the dragging continues regardless if the mouse is in the element's boundary.

So one of the basic tests that occasionally fails is this one (I added comments to explain):

    Action<JsonElement>? onMouseMoveCallback = default;

    public MultiRangeSlider_Moving_Mouse_Tests()
    {
        //exposed in the base class
        var mock = base.MockedDomEventListener;
        //AddShared() method is adding a callback to a dictionary of callbacks registered for type of event on dom object
        //By mocking the DomEventService, I can get the private callback and assign it to private Action onMouseMoveCallback
        mock.Setup(s => s.AddShared<JsonElement>("window", "mousemove", It.IsAny<Action<JsonElement>>(), It.IsAny<bool>()))
            .Callback((object dom, string eventName, Action<JsonElement> callback, bool preventDefault) =>
                {                
                    if (callback.Target is AntDesign.RangeItem)
                    {
                        onMouseMoveCallback = callback;                
                    }
                });
    }

    [Fact]
    public void MouseMove_should_move_edge() {
        //Arrange
        JSInterop.Setup<HtmlElement>(JSInteropConstants.GetDomInfo, _ => true)
            .SetResult(new HtmlElement { ClientWidth = 100 }); //slider length
        var cut = Render<AntDesign.MultiRangeSlider>(@<AntDesign.MultiRangeSlider Value="@_valuesWithNoOverlap"/>);
        //will test single edge being dragged - edge is part of the RangeItem component
        var rangeItems = cut.FindComponents<AntDesign.RangeItem>();
        //the edges are stored in a tuple (double, double), so get the original value that will be used later for assertion
        var originalValue = rangeItems[2].Instance.Value;
        //mouseMove callback expects a JsonElement with mouse position - will use that to calculate new allowed position for moved edge
        var jsonElement = JsonDocument.Parse($"{{\"clientX\":{originalValue.Item1 - 1},\"clientY\":0, \"pageX\":{originalValue.Item1 - 1}, \"pageY\":0}}").RootElement;  
        //Act       
        //initiate MouseDown; inside it, the js move event will be added and will be picked up by Moq
        rangeItems[2].Find("div.ant-multi-range-slider-handle-1").MouseDown();
        //onMouseMoveCallback is going to be registered during MouseDown, I am waiting here for the code to finish
        cut.WaitForState(() => onMouseMoveCallback is not null);
        //Execute simulated mouse movement
        onMouseMoveCallback!.Invoke(jsonElement);
        //Assert
        rangeItems[2].Instance.Value.Should().Be((originalValue.Item1 - 1, originalValue.Item2));        
    }

I tried replacing cut.WaitForState(() => onMouseMoveCallback is not null); with cut.WaitForAssertion(() => onMouseMoveCallback.Should().NotBeNull()); - no difference. I also have a couple of [Theory] tests, where I get the Collection was modified exception the most. I even had to switch from WaitForState to my own implementation of WaitFor, otherwise I would get 50% to 90% of the theory tests failed due to assertion fails or the aforementioned exception. My own WaitFor is just a method that runs await Task.Delay(10), checks the predicate and waits again if predicate fails (up to 100 times, so approx 1sec).

[egil]

First off, cool feature with the drag and drop.

I am not sure I am any wiser as to what is causing the issue. The exception happens, according to your stack trace, when the test finishes running, and xUnit is diposing the test class by calling AntDesignTestBase.Dispose(). That in turn causes the Blazor Renderer's Dispose method to be called, and I believe it is in this foreach loop inside it, that the exception is coming from. That loop loops over the Renderer's internal state of each component it has rendered, which leads me to believe that new components are being added to the collection after the test finishes running.

For debugging this, it would be helpful if you can add logging to the test like its described here: Capturing logs from ILogger in test output.

That will allow us to see inside the rendering pipeline and what Blazor is doing, and might clear up what components are still rendering at the disposing time.

[anddrzejb]

Yeah, I figure it was that foreach you pointed to. What was irritating me the most is that it does not happen every time. And it happens only when I try to run all my tests. If I run only tests related to the component in question, no problem (or they just never had a chance to manifest).

Now regarding logging - that's funny, because I already tried logging and couldn't figure out what was wrong. Only now, when I was preparing this message, I noticed that tooltip was rendering - which clearly for tests is not needed. And it is rendering asynchronously (actually 2 tooltips, for each edge). Now that I added the parameter turning the tooltip off (after all these tests do not need it), the exception does not appear. Go figure... I feel ashamed to bother you for such a stupid mistake... But as usual, you were most helpful and I thank you!

Thanks for your kind words about the new component. It is a pending PR, I hope the AntBlazor maintainer will approve it. I work a LOT with multiple periods (dates, times, overlapping, non-overlapping, etc) and I need such a component. It obviously can do much more than it was presented on the gif.

[egil]

No worries Andrzej, happy to help and glad that you found a solution!