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

BeEquivalentTo throws a NullReferenceException #1932

Closed
DenisSokol opened this issue May 20, 2022 · 16 comments
Closed

BeEquivalentTo throws a NullReferenceException #1932

DenisSokol opened this issue May 20, 2022 · 16 comments

Comments

@DenisSokol
Copy link

DenisSokol commented May 20, 2022

Description

while running tests in parallel ( 2 threads) and when using BeEquivalentTo() to compare objects from two lists a null reference exception is thrown.
However no NRE is thrown on using single thread

System.NullReferenceException : Object reference not set to an instance of an object.
   at FluentAssertions.Equivalency.SelfReferenceEquivalencyAssertionOptions`1.ToString()
public class Option
{
    public string Id { get; set; }
    public string Name { get; set; }
}
List<Option> expectedOptions;
List<Option> actualOptions;

Complete minimal example reproducing the issue

// Arrange
List<Option> expectedOptions = new List<Option>
{
    new Option()
    {
        Id = "4b71bcd9",
        Name = "AutoTest_123"
    }
};

// Act
List<Option> actualOptions = new List<Option>
{
    new Option()
    {
        Id = "4b71bcd9",
        Name = "AutoTest_123"
    }
};

// Assert
for (var i = 0; i < expectedOptions.Count; i++)
{
    expectedOptions [i].Should().BeEquivalentTo(actualOptions[i]);
}

Expected behavior:

The assertion should be true.

Actual behavior:

System.NullReferenceException : Object reference not set to an instance of an object.
   at FluentAssertions.Equivalency.SelfReferenceEquivalencyAssertionOptions`1.ToString()

Versions

FluentAssertions Version="6.7.0"

@jnyrup
Copy link
Member

jnyrup commented May 20, 2022

Please include a complete and minimal example that we can run, i.e. a snippet I can copy directly into an IDE an run.
I can't see what an Option is.

@DenisSokol
Copy link
Author

Please include a complete and minimal example that we can run, i.e. a snippet I can copy directly into an IDE an run. I can't see what an Option is.

Sure thing, I've updated my question

@lg2de
Copy link
Contributor

lg2de commented May 20, 2022

This is still not valid code.
Fixing the bugs, the problem is not reproducible!

@jnyrup
Copy link
Member

jnyrup commented May 20, 2022

I tried this on my machine to reproduce the error, but no errors occurred.

Parallel.For(0, 10000, new ParallelOptions { MaxDegreeOfParallelism = -1 }, _ =>
{
    List<Option> expectedOptions = new List<Option>
    {
        new Option()
        {
            Id = "4b71bcd9",
            Name = "AutoTest_123"
        }
    };

    List<Option> actualOptions = new List<Option>
    {
        new Option()
        {
            Id = "4b71bcd9",
            Name = "AutoTest_123"
        }
    };

    for (var i = 0; i < expectedOptions.Count; i++)
    {
        expectedOptions[i].Should().BeEquivalentTo(actualOptions[i]);
    }
});

Have you tried reproducing the error in a fresh project?
Are you using AssertionOptions.AssertEquivalencyUsing?
I recall someone have had concurrency problems because they used that method in a non-intended way.

The error indicates the problem lies somewhere, but nothing strikes me

public override string ToString()
{
var builder = new StringBuilder();
builder.Append("- Use ")
.Append(useRuntimeTyping ? "runtime" : "declared")
.AppendLine(" types and members");
if (ignoreNonBrowsableOnSubject)
{
builder.AppendLine("- Do not consider members marked non-browsable on the subject");
}
if (isRecursive)
{
if (allowInfiniteRecursion)
{
builder.AppendLine("- Recurse indefinitely");
}
}
builder.AppendFormat(CultureInfo.InvariantCulture,
"- Compare enums by {0}" + Environment.NewLine,
enumEquivalencyHandling == EnumEquivalencyHandling.ByName ? "name" : "value");
if (cyclicReferenceHandling == CyclicReferenceHandling.Ignore)
{
builder.AppendLine("- Ignoring cyclic references");
}
builder.AppendLine($"- Compare tuples by their properties");
builder.AppendLine($"- Compare anonymous types by their properties");
if (compareRecordsByValue)
{
builder.AppendLine("- Compare records by value");
}
else
{
builder.AppendLine("- Compare records by their members");
}
foreach (Type valueType in valueTypes)
{
builder.AppendLine($"- Compare {valueType} by value");
}
foreach (Type type in referenceTypes)
{
builder.AppendLine($"- Compare {type} by its members");
}
if (excludeNonBrowsableOnExpectation)
{
builder.AppendLine("- Exclude non-browsable members");
}
else
{
builder.AppendLine("- Include non-browsable members");
}
foreach (IMemberSelectionRule rule in selectionRules)
{
builder.Append("- ").AppendLine(rule.ToString());
}
foreach (IMemberMatchingRule rule in matchingRules)
{
builder.Append("- ").AppendLine(rule.ToString());
}
foreach (IEquivalencyStep step in userEquivalencySteps)
{
builder.Append("- ").AppendLine(step.ToString());
}
foreach (IOrderingRule rule in OrderingRules)
{
builder.Append("- ").AppendLine(rule.ToString());
}
builder.Append("- ").AppendLine(ConversionSelector.ToString());
return builder.ToString();
}

@DenisSokol
Copy link
Author

DenisSokol commented May 20, 2022

Nope, no using AssertionOptions.AssertEquivalencyUsing on my end. I'll stick with NUnit assertions as it works with no issues running tests in parallel though

@dennisdoomen
Copy link
Member

We run thousands of tests in parallel using xUnit and FluentAssertions for the last 10 years. We never experienced anything like that. If there was a concurrency issue, the problem was always with the code under test.

So to diagnose this, we need more information. It's also weird that I don't see line numbers in your stack trace.

@jnyrup
Copy link
Member

jnyrup commented May 21, 2022

We have had a few bug fixes related the concurrency issues in the library itself 😉
Events: #325, #1773
Caching: #864, #1091

Here are two cases where the problem was calling methods that are by-design not thread-safe.
Configuration: #714, #1323

I'd love if you have time to help us dig into what is causing this issue for you.
If not a bug in the library then an explanation of why it happens could help others hitting the same problem.

Is the problem reproducible for you or did it only happened once?
Do you use anything from the classes AssertionOptions, EquivalencyPlan in any tests?
Which framework does the unit test project targets?
What operation system are you running the tests on?

@DenisSokol
Copy link
Author

DenisSokol commented May 24, 2022

@jnyrup, Unfortunately the problem is still reproducible on my end while running tests in parallel. I don't use any AssertionOptions, EquivalencyPlan in my tests. To reproduce the issue i've hardcoded two lists in my test method:

[Test]
[Category(TestCategories.Regression)]
[Category(TestCategories.Medium)]
public void VIS11111()
{
    That.Given(_ => ApiSteps.TestMe())
        .BDDfy("Test #1: BlaBlaBla");
}  
public void TestMe()
{
    var expectedOptions = new List<Option>()
    {
        new Option()
        {
            Id = "123",
            Name = "Test"
        }
    };
    
    var actualOptions = new List<Option>()
    {
        new Option()
        {
            Id = "123",
            Name = "Test"
        }
    };

    for (var i = 0; i < expectedOptions.Count; i++)
    {
        expectedOptions[i].Should().BeEquivalentTo(actualOptions[i]);
    }
}
public class Option
{
    public string Id { get; set; }
    public string Name { get; set; }
}

StackTrace (FYI @dennisdoomen )

System.NullReferenceException : Object reference not set to an instance of an object.
   at FluentAssertions.Equivalency.SelfReferenceEquivalencyAssertionOptions`1.ToString()
   at FluentAssertions.Equivalency.EquivalencyValidator.AssertEquality(Comparands comparands, EquivalencyValidationContext context)
   at FluentAssertions.Primitives.ObjectAssertions`2.BeEquivalentTo[TExpectation](TExpectation expectation, Func`2 config, String because, Object[] becauseArgs)
   at FluentAssertions.Primitives.ObjectAssertions`2.BeEquivalentTo[TExpectation](TExpectation expectation, String because, Object[] becauseArgs)
   at MyTestFramework.ApiSteps.TestMe() 

Technologies:
ASP.Net Core 6.0
NUnit3
BDDfy
Log4net
Entity Framework
Unity Container

Windows Specifications:
Edition Windows 10 Pro
Version 21H2
Installed on ‎16/‎02/‎2022
OS build 19044.1706
Experience Windows Feature Experience Pack 120.2212.4170.0

TestFramework.zip

@jnyrup
Copy link
Member

jnyrup commented May 24, 2022

I'm trying to reconstruct some runnable code, but I can't see where That in That.Given come from?
If you could construct an isolated project, zip it and upload it here, that would help us.

@DenisSokol
Copy link
Author

@jnyrup I've updated the previous post and uploaded test framework skeleton

@jnyrup
Copy link
Member

jnyrup commented May 25, 2022

Thanks for the complete example, it made it a lot easier to diagnose.

ApiTestHelper.GlobalSetUp is invoked concurrently for both tests and does call AssertionOptions.AssertEquivalencyUsing which is not safe.

One way to solve it is to move the setup of Fluent Assertions into a Module Initializer which are guaranteed to only run once.

[System.Runtime.CompilerServices.ModuleInitializer]
internal static void SetupFluentAssertions()
{
    AssertionOptions.AssertEquivalencyUsing(options => options
        .Using<DateTimeOffset>(ctx => ctx.Subject.Should().BeSameDateAs(ctx.Expectation))
        .WhenTypeIs<DateTimeOffset>()
        .Using<DateTime>(ctx => ctx.Subject.Should().BeSameDateAs(ctx.Expectation))
        .WhenTypeIs<DateTime>()
    );
}

@DenisSokol
Copy link
Author

@jnyrup, Thanks a lot for digging into the problem resolving. I appreciate your help. It works now!

@dennisdoomen
Copy link
Member

Maybe we could add some detection logic that throws when you try to call the AssertionOptions concurrently....

@jnyrup
Copy link
Member

jnyrup commented May 30, 2022

We could add a lock inside AssertEquivalencyUsing to avoid concurrent calls modifying thread-unsafe collections and getting lists with null values, which I believe was one of kind of exception I saw.
It won't guard against getting a "Collection was modified; enumeration operation may not execute" exception though.

I don't think we should serialize reading AssertionOptions.defaults.

It valid to call AssertEquivalencyUsing multiple times, but requires carefulness, so we shouldn't limit invocations to a single one.

My concern about this is how much it helps, as it might just hide that it's still unsafe to call multiple times as it can change the outcome of a test.

@JakobFerdinand
Copy link

I just got the same error and it nearly drove me crazy because it occurred only randomly.
The solution with [System.Runtime.CompilerServices.ModuleInitializer] seams to solve the problem for me.
I think a generell threadsafe behavior would be be great!

@dennisdoomen
Copy link
Member

It valid to call AssertEquivalencyUsing multiple times, but requires carefulness, so we shouldn't limit invocations to a single one.

I only meant trying to detect concurrent calls. That should never happen. Successive calls are fine.

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

No branches or pull requests

5 participants