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

Comparing datatables with dbnull in expectation doesn't cause test to fail when subject has a value #2

Open
brendon-r opened this issue Jul 20, 2022 · 9 comments
Labels
bug Something isn't working

Comments

@brendon-r
Copy link

brendon-r commented Jul 20, 2022

Description

Comparing DataTables where a column in the expectation containing a dbnull value passes comparison regardless of the value in the same column in the subject.

Complete minimal example reproducing the issue

// Arrange
var expected = new DataTable();
expected.Columns.AddRange(new[] 
{ 
    new DataColumn("Column1", typeof(string)), 
    new DataColumn("Column2", typeof(string)), 
    new DataColumn("Column3", typeof(string)) 
});

expected.Rows.Add("test", null, "1" );

// Act
var actual = new DataTable();
actual.Columns.AddRange(new[] 
{
    new DataColumn("Column1", typeof(string)),
    new DataColumn("Column2", typeof(string)),
    new DataColumn("Column3", typeof(string))
});
actual.Rows.Add("test", "fail", "1");

// Assert
actual.Should().BeEquivalentTo(expected);

Expected behavior:

The test to fail because "Column2" in the expectation is null while it has a value of "fail" in the subject

Actual behavior:

The test passes. Reversing the order leads to the test failing as expected, i.e.,

// Assert
expected.Should().BeEquivalentTo(actual);

fails.

Versions

  • Which version of Fluent Assertions are you using? 6.7.0
  • Which .NET runtime and version are you targeting? .NET 6

Additional Information

Any additional information, configuration or data that might be necessary to reproduce the issue.

@jnyrup
Copy link
Member

jnyrup commented Jul 21, 2022

Thanks for reporting this.

I did some quick investigations, but I don't have a solution/workaround yet.
Here are my findings so far.

The case can effectively be reduced to this:

[Fact]
public void DbNull_is_not_equivalent_to_something_else()
{
    var actual = new { Value = "fail" };
    var a = new { Value = DBNull.Value };

    actual.Should().NotBeEquivalentTo(a);
}

Our property Value is not IsRoot so we do not throw at line 40 and continue until line 51, i.e. objects are considered equal.

https://github.com/fluentassertions/fluentassertions/blob/b02b44dbe22c6c65f975737b431e963c7e5514d5/Src/FluentAssertions/Equivalency/Steps/StructuralEqualityEquivalencyStep.cs#L37-L51

@jnyrup jnyrup added the bug Something isn't working label Jul 22, 2022
@dennisdoomen
Copy link
Member

The reason for this is that DbNull.Value is a singleton which does not override Equals. You can call Equals on it, but it will defer to the default object.Equals implementation, which will use reference equality. Since Value always returns the same instance, comparing it to itself will work. Because of this behavior, our HasValueSemantics extension method will not treat it as a value type and thus tell BeEquivalentTo to treat it as a complex type. But since the DbNull instance doesn't have properties or fields, nothing will happen.

You can work around this by using the ComparingByValue<DBNull>() option, but the default formatter won't make the message very nice:

Expected property actual.Value to be , but found "fail".

So maybe we should treat DbNull as something special?

@jnyrup
Copy link
Member

jnyrup commented Jul 23, 2022

ComparingByValue is not currently available on IDataEquivalencyAssertionOptions<T> which is what DataTableAssertions.BeEquivalentTo uses, but it could easily be exposed.

One workaround for now is to define an extension method

public static class DataEquivalencyAssertionOptionsExtensions
{
    public static IDataEquivalencyAssertionOptions<T> ComparingByValue<T, TType>(this IDataEquivalencyAssertionOptions<T> options)
    {
        ((EquivalencyAssertionOptions<T>)options).ComparingByValue<TType>();
        return options;
    }
}

and then write the test as

actual.Should().BeEquivalentTo(expected, opt => opt.ComparingByValue<DataTable, DBNull>());

By special handling, do you think of adding an explicit DBNullEquivalencyStep?

I'm wondering whether adding special handling for DBNull scales well, i.e. if there are other types that would need the same handling? 🤔

@dennisdoomen
Copy link
Member

ComparingByValue is not currently available on IDataEquivalencyAssertionOptions<T> which is what DataTableAssertions.BeEquivalentTo uses, but it could easily be exposed.

True. I was basing my analysis on your DbNull_is_not_equivalent_to_something_else

By special handling, do you think of adding an explicit DBNullEquivalencyStep?

To add it as a well-known type to the HasValueSemantics type.

I'm wondering whether adding special handling for DBNull scales well, i.e. if there are other types that would need the same handling? 🤔

I haven't seen them up to now, but that doesn't mean anything.

@jnyrup
Copy link
Member

jnyrup commented Jul 25, 2022

To add it as a well-known type to the HasValueSemantics type.

Ahh, had thought about that one.
Sounds reasonable.

@jnyrup
Copy link
Member

jnyrup commented Jul 31, 2022

Another workaround for now is to hook into the default AssertionOptions.

MSTest has [AssemblyInitialize].
For xUnit it can be done using a custom framework. https://fluentassertions.com/tips/#using-global-assertionoptions

For .NET 5 or greater one can use the [ModuleInitializer]

Dump this class in the test project

internal static class Initializer
{
    [ModuleInitializer]
    public static void SetDefaults()
    {
        AssertionOptions.AssertEquivalencyUsing(options => options
            .Using<DBNull>(ctx => ctx.Subject.Should().Be(ctx.Expectation))
            .WhenTypeIs<DBNull>());
    }
}

and the provided test now fails as expected with

Expected actual.Rows[0][Column2] from subject to be a System.DBNull, but found a System.String.

@Corniel
Copy link

Corniel commented Jun 8, 2023

Although I personally hardly use DbNull.Value at all (anymore), I think the types deserves a special treatment. You could even argue that objects (including new object()) that do not have properties should always be treated as value types.

@IT-VBFK
Copy link

IT-VBFK commented Oct 14, 2023

Should this be moved to FluentAssertions.DataSets?

@dennisdoomen dennisdoomen transferred this issue from fluentassertions/fluentassertions Oct 14, 2023
@jnyrup
Copy link
Member

jnyrup commented Oct 15, 2023

Just a note: DbNull is also used for database stuff unrelated to the System.Data types in this repository, e.g. npgsql, SqlClient and probably other.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants