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.

Sign up for GitHub

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

Jump to bottom

Exceptions in event accessors fail tests when using .Monitor() #1948

Open
apazureck opened this issue Jul 2, 2022 · 8 comments · May be fixed by #1954 or #2629
Open

Exceptions in event accessors fail tests when using .Monitor() #1948

apazureck opened this issue Jul 2, 2022 · 8 comments · May be fixed by #1954 or #2629
Labels
enhancement up-for-grabs

Comments

@apazureck
Copy link

apazureck commented Jul 2, 2022 •
edited

Description

When an event handler accessor has an explicit implementation, which throws an exception on the setter, .Monitor() is failing the test completely.

What is my motivation?

I need to test the class behavior without changing the other base event handler accessors behavior. It could cause unwanted side effects. But I do not need to test this event. I just wanted to test PropertyChanged events being raised. Therefore, I would like to have the benefit of having the full class available to write expressions in the assertion.

Suggestion:

Make some more configuration options for the Monitor. Like ignoring failing events, or just subscribing to a subset of interfaces with those events, or putting some events on ignore, without changing the generic Type, which is used to assert / monitor.

Benefit:

Using the great syntax you introduced in monitor on brown field projects or if you are not able to change the behavior of some event accessors. Therefore, write better and cleaner test code from the beginning.

Complete minimal example reproducing the issue

using System.ComponentModel;
using Xunit;

namespace FluentAssertions.Tests;

class FailingEventHandlerSubscription : INotifyPropertyChanged
{
    private event PropertyChangedEventHandler? InnerPropertyChanged;
    public event PropertyChangedEventHandler? PropertyChanged
    {
        add { throw new System.Exception("This is sadly happening due to anonymous method"); }
        remove { InnerPropertyChanged -= value; }
    }

    private object prop;

    public object Property
    {
        get => prop;
        set
        {
            prop = value;
            this.InnerPropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Property)));
        }
    }
}

public class UnitTest2
{
    [Fact]
    public void TestEventHandler()
    {
        // Arrange
        var cut = new FailingEventHandlerSubscription();

       // will fail, because the exception is thrown, when the EventHandler class is subscribing the event.
        using var monitored = cut.Monitor();

        // Act
        cut.Property = new object();

        // Assert
        // If I just use the INotifyPropertyChanged interface, I cannot write this awesome expression.
        monitored.Should().RaisePropertyChangeFor(m => m.Property);
    }
}

Expected behavior:

FluentAssertions is able to have options in the monitor to ignore failing event handlers.

Actual behavior:

It throws an exception when .Monitor() is called and I can do nothing about it, event though I do not want to test this specific event.

Versions

FluentAssertions Version 6.7.0

Additional Information

I already implemented my custom implementation of the monitor and would offer to contribute my suggestion above to the FluentAssertions project. I just wanted to talk to you first, if you would approve of such an enhancement of the monitor or if you disagree on this feature for some reason.

So let me know if you agree or disagree.

cheers!
@dennisdoomen
Copy link
Member

I'm not sure how we can fix this. The job of Monitor is to subscribe to the events exposes by the SUT, so it can track which events was raised. I don't think we should ignore exceptions while subscribing. If we would, an assertion like Should().RaisePropertyChangeFor will throw an exception because the property wasn't raised, even though it never subscribed.

@lg2de
Copy link
Contributor

lg2de commented Jul 6, 2022

The method Monitor could get (optional) options to ignore specific events to be subscribed.

@dennisdoomen
Copy link
Member

OK. That would be possible. However, unless somebody is willing to contribute that change, I don't think it's going to get a lot of priority.

@apazureck
Copy link
Author

apazureck commented Jul 8, 2022 •
edited

@dennisdoomen I already forked and implemented it. The default behavior won't be changed. I added an optional .ConfigureMonitor(...) extension, with which it is possible to suppress the exception either using a configuration object or a fluent api with .IgnoreEventAccessorExceptions().

So a normal call would look like this:

using IMonitor<ClassToMonitor> monitor = cut.ConfigureMonitor().IgnoreEventAccessorExceptions().Monitor();

Would you be OK with that?

@lg2de
Copy link
Contributor

lg2de commented Jul 8, 2022

I recommend to ignore specific events instead of "exception when accessing any event". This looks very specific to your scenario.
The options should be similar to exclusion in BeEquivalentTo.

@apazureck
Copy link
Author

I see your argument. I have no strong opinion about how to resolve this.

What you are suggesting is closer to the rest of the framework, which I think is a good thing to do. I never used the BeEquivalentTo exclusions. I will have a look at them next week and come up with a suggestion.

My approach is more pragmatic to solve the problem, as it will ignore all events which have "broken" event accessors and leave the rest as is. So you do not have to change the test when some more not used event accessors may get broken in the future, too. This is following the SOLID principles (SRP - Only one reason to change). Which I think is also a good thing to consider.

But I am also a fan of having choices, so why not both approaches? I just don't want to do both in one merge request / feature request.

@dennisdoomen
Copy link
Member

I would change it to:

using var monitored = cut.Monitor(opt => opt.IgnoringEventAccessorExceptions());

@apazureck
Copy link
Author

I had this with the .ConfigureMonitoring(opt => ... ), but this is much better. I will change it this week, when I have time and throw out the configure. Thank you!

@apazureck apazureck linked a pull request Jul 12, 2022 that will close this issue
Open
5 tasks
@ITaluone ITaluone linked a pull request Apr 22, 2024 that will close this issue
Open
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement up-for-grabs
Projects
None yet
Milestone
No milestone
3 participants
@dennisdoomen @apazureck @lg2de