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

Assertion messages could be confusing #1901

Open
Bludator opened this issue Dec 1, 2022 · 5 comments
Open

Assertion messages could be confusing #1901

Bludator opened this issue Dec 1, 2022 · 5 comments

Comments

@Bludator
Copy link

Bludator commented Dec 1, 2022

Here is the minimal example (body of some class Test):

public class Something { }

public interface IFoo
{
    void Method(Something x);
}
public interface Bar : IFoo { }

[Fact]
public void Test14()
{
    var fake = A.Fake<Bar>();
    fake.Method(new Something()); 
    // asserting different object
    A.CallTo(()=>fake.Method(new Something())).MustHaveHappened();
}

Prints:

FakeItEasy.ExpectationException : 

  Assertion failed for the following call:
    Test.Test+IFoo.Method(x: TestUtil.Attributes.Test+Something)
  Expected to find it once or more but didn't find it among the calls:
    1: Test.Test+Bar.Method(x: TestUtil.Attributes.Test+Something)

The method name/defining type should be the same in both cases not Test+IFoo.Method and Test+Bar.Method.
Now it looks like there is other problem than the non-mathing parameter as it is the only different thing in the calls as printed.

Also it would be helpful if the assert message would be different when no calls to a methods were made and when the parameter assertions does not match, because it is kind of silly to print something like in the example where the actual parameter looks same as the required one.

@blairconrad
Copy link
Member

Hi, @Bludator. Thanks for your interest in FakeItEasy. The question about the method name is giving us at FakeItEasy National some small pause, since default interface methods are kind of a special thing (that I don't have a lot of experience with) and we're not entirely sure what output we expect. We'll continue discussing.

In the meantime, about

it would be helpful if the assert message would be different when no calls to a methods were made and when the parameter assertions does not match

I'm not sure I follow you. In general, the messages are different. Consider the output I get when I omit the fake.Method call from your example:

FakeItEasy.ExpectationException :

Assertion failed for the following call:
  FIEQuestions.Test+IFoo.Method(x: FIEQuestions.Test+Something)
Expected to find it once or more but no calls were made to the fake object.

@Bludator
Copy link
Author

Bludator commented Dec 1, 2022

Hi, @Bludator. Thanks for your interest in FakeItEasy. The question about the method name is giving us at FakeItEasy National some small pause, since default interface methods are kind of a special thing (that I don't have a lot of experience with) and we're not entirely sure what output we expect. We'll continue discussing.

I think that the same thing happened without the default method implementation, I guess I forget to remove it, sorry about that. (and I edited the issue)

I'm not sure I follow you. In general, the messages are different. Consider the output I get when I omit the fake.Method call from your example:

Well, that is true (if there is no other call on the fake). My point is that in:

Assertion failed for the following call:
    TestUtil.Attributes.Test+IBar.Method(x: TestUtil.Attributes.Test+Something)
  Expected to find it once or more but didn't find it among the calls:
    1: TestUtil.Attributes.Test+IBar.Method(x: TestUtil.Attributes.Test+Something)

It is not clear where the problem is. (The call is there so why it won't pass the test?🤷‍♀️)

Ideally, there should be printed what constraint was not matching and an actual value of the parameter but I don't think it is possible (when the fake has multiple rules, calls, etc.). But something like Expected to find it once or more but didn't find any call matching constraints among the calls: and filtering the calls to show only calls to the method would be helpful as well.

@blairconrad
Copy link
Member

blairconrad commented Dec 1, 2022

Okay. Confirmed that the default interface implementation was not required to get the output you posted. So the question I see is why we say we failed for FIEQuestions.Test+IFoo.Method and found a call to FIEQuestions.Test+Bar.Method. I think this is a reasonable question.

there should be printed what constraint was not matching and an actual value of the parameter

We do print the actual value of the parameter, using ToString. In your sample code, Something's ToString method is not sufficient to differentiate between the arguments. Consider instead:

public class Something
{
    private readonly int id;

    public Something(int id)
    {
        this.id = id;
    }

    public override string ToString() => $"Something {id}";
}

[Fact]
public void Test14()
{
    var fake = A.Fake<Bar>();
    fake.Method(new Something(1));
    // asserting different object
    A.CallTo(() => fake.Method(new Something(2))).MustHaveHappened();
}

yields

Assertion failed for the following call:
  FIEQuestions.Test+IFoo.Method(x: Something 2)
Expected to find it once or more but didn't find it among the calls:
  1: FIEQuestions.Test+Bar.Method(x: Something 1)

@Bludator
Copy link
Author

Bludator commented Dec 1, 2022

The printing via ToString() is something I didn't think of and definitely should do 👍.

@blairconrad
Copy link
Member

I'm back with very little information, but in this message

Assertion failed for the following call:
  FIEQuestions.Test+IFoo.Method(x: Something 2)
Expected to find it once or more but didn't find it among the calls:
  1: FIEQuestions.Test+Bar.Method(x: Something 1)

The line that references "Something 2" makes up the reported type name using method.DeclaringType.
The "Something 1" line makes up the reported type name using fakeManager.FakeObjectType.

That's why we're seeing the difference.

I'm half thinking that we might be better off with fakeManger.FakeObjectType all over.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants