NUnit 2045: Assert.Multiple fixer might be slightly too greedy due to the lack of lookahead

[danielmarbach]

For example

            var configuration = new Configuration();
            Assert.That(configuration, Is.Not.Null);
            Assert.That(configuration.Value1, Is.EqualTo(0));
            Assert.That(configuration.Value2, Is.EqualTo(0.0));
            Assert.That(configuration.Value11, Is.EqualTo(string.Empty));
            Assert.That(configuration.Values, Is.Not.Empty);
            Assert.That(configuration.Values.ElementAt(1), Is.EqualTo(string.Empty));

will be rewritten to

            var configuration = new Configuration();
            Assert.That(configuration, Is.Not.Null);
            Assert.Multiple(() =>
            {
                Assert.That(configuration.Value1, Is.EqualTo(0));
                Assert.That(configuration.Value2, Is.EqualTo(0.0));
                Assert.That(configuration.Value11, Is.EqualTo(string.Empty));
                Assert.That(configuration.Values, Is.Not.Empty);
            });
            Assert.That(configuration.Values.ElementAt(1), Is.EqualTo(string.Empty));

while my intuition tells me it should do

            var configuration = new Configuration();
            Assert.That(configuration, Is.Not.Null);
            Assert.Multiple(() =>
            {
                Assert.That(configuration.Value1, Is.EqualTo(0));
                Assert.That(configuration.Value2, Is.EqualTo(0.0));
                Assert.That(configuration.Value11, Is.EqualTo(string.Empty));
            });
            Assert.That(configuration.Values, Is.Not.Empty);
            Assert.That(configuration.Values.ElementAt(1), Is.EqualTo(string.Empty));

or if Values would contain more complex objects

            var configuration = new Configuration();
            Assert.That(configuration, Is.Not.Null);
            Assert.That(configuration.Value1, Is.EqualTo(0));
            Assert.That(configuration.Value2, Is.EqualTo(0.0));
            Assert.That(configuration.Value11, Is.EqualTo(string.Empty));
            Assert.That(configuration.Values, Is.Not.Empty);
            Assert.That(configuration.Values[0].Value1, Is.EqualTo(string.Empty));
            Assert.That(configuration.Values[0].Value2, Is.EqualTo(string.Empty));
            Assert.That(configuration.Values[0].Value11, Is.EqualTo(string.Empty));

will be rewritten to

            var configuration = new Configuration();
            Assert.That(configuration, Is.Not.Null);
            Assert.Multiple(() =>
            {
                Assert.That(configuration.Value1, Is.EqualTo(0));
                Assert.That(configuration.Value2, Is.EqualTo(0.0));
                Assert.That(configuration.Value11, Is.EqualTo(string.Empty));
                Assert.That(configuration.Values, Is.Not.Empty);
            });
            Assert.Multiple(() =>
            {
                Assert.That(configuration.Values[0].Value1, Is.EqualTo(string.Empty));
                Assert.That(configuration.Values[0].Value2, Is.EqualTo(string.Empty));
                Assert.That(configuration.Values[0].Value11, Is.EqualTo(string.Empty));
            });

while my intuition would tell me it should do

            var configuration = new Configuration();
            Assert.That(configuration, Is.Not.Null);
            Assert.Multiple(() =>
            {
                Assert.That(configuration.Value1, Is.EqualTo(0));
                Assert.That(configuration.Value2, Is.EqualTo(0.0));
                Assert.That(configuration.Value11, Is.EqualTo(string.Empty));
            });
             Assert.That(configuration.Values, Is.Not.Empty);
            Assert.Multiple(() =>
            {
                Assert.That(configuration.Values[0].Value1, Is.EqualTo(string.Empty));
                Assert.That(configuration.Values[0].Value2, Is.EqualTo(string.Empty));
                Assert.That(configuration.Values[0].Value11, Is.EqualTo(string.Empty));
            });

Or with nested objects

            var configuration = new Configuration();
            Assert.That(configuration, Is.Not.Null);
            Assert.That(configuration.Value1, Is.EqualTo(0));
            Assert.That(configuration.Value2, Is.EqualTo(0.0));
            Assert.That(configuration.Value11, Is.EqualTo(string.Empty));
            Assert.That(configuration.Inner, Is.Not.Null);
            Assert.That(configuration.Inner.Value1, Is.EqualTo(0));
            Assert.That(configuration.Inner.Value2, Is.EqualTo(0.0));
            Assert.That(configuration.Inner.Value11, Is.EqualTo(string.Empty));

it currently does

var configuration = new Configuration();
Assert.That(configuration, Is.Not.Null);
Assert.Multiple(() =>
{
    Assert.That(configuration.Value1, Is.EqualTo(0));
    Assert.That(configuration.Value2, Is.EqualTo(0.0));
    Assert.That(configuration.Value11, Is.EqualTo(string.Empty));
    Assert.That(configuration.Inner, Is.Not.Null);
});
Assert.Multiple(() =>
{
    Assert.That(configuration.Inner.Value1, Is.EqualTo(0));
    Assert.That(configuration.Inner.Value2, Is.EqualTo(0.0));
    Assert.That(configuration.Inner.Value11, Is.EqualTo(string.Empty));
});

while my intuition tells me to

var configuration = new Configuration();
Assert.That(configuration, Is.Not.Null);
Assert.Multiple(() =>
{
    Assert.That(configuration.Value1, Is.EqualTo(0));
    Assert.That(configuration.Value2, Is.EqualTo(0.0));
    Assert.That(configuration.Value11, Is.EqualTo(string.Empty));
});
Assert.That(configuration.Inner, Is.Not.Null);
Assert.Multiple(() =>
{
    Assert.That(configuration.Inner.Value1, Is.EqualTo(0));
    Assert.That(configuration.Inner.Value2, Is.EqualTo(0.0));
    Assert.That(configuration.Inner.Value11, Is.EqualTo(string.Empty));
});

I do realize this can be highly depending on "taste". I think my intuition comes from the explanation of Assert.Multiple that an argument is not suffixed later and that is an indication that individual subtrees should be groupe together

[manfred-brands]

@danielmarbach Thanks for your report.

The current CodeFix uses common prefix to determine if something is independent. So in your first example: configuration.Values is independent of the earlier and hence is grouped inside the Assert.Multiple. The configuration.Values.ElementAt(1) has the same prefix as that statement and hence is not independent.

Logically you might say that these two statements belong together, but the CodeFix looks at maximum grouping ability and is technically correct. Maybe we can add a configuration option to not put a prefix inside the last group and always leave it on its own. As you said, it is a matter of "taste".

[danielmarbach]

@manfred-brands Agreed it is technically correct. I was first even hesitant to raise it as an issue. I think what caused me to raise it was that the prefix can be seen as "start of a hierarchy level" and every new prefix is a different "hierarchy". I went down on this path of thinking because when I saw

var configuration = new Configuration();
Assert.That(configuration, Is.Not.Null);
Assert.Multiple(() =>
{
    Assert.That(configuration.Value1, Is.EqualTo(0));
    Assert.That(configuration.Value2, Is.EqualTo(0.0));
    Assert.That(configuration.Value11, Is.EqualTo(string.Empty));
});

I immediately thought "kind of smart because essentially every . or descending into subtrees is a good candidate for grouping". But when I look at the application of the fix through our more than 20 plus repositories, we got quite surprising groupings. My hunch that the originally intent of the feature was exactly what my intuition told me is the following rule

https://github.com/nunit/nunit.analyzers/blob/master/src/nunit.analyzers/UseAssertMultiple/UseAssertMultipleAnalyzer.cs#L49-L50 which not only looks at the prefix but whether a new hierarchy is started (or at least that was my interpretation of that check and it might be biased 🤣 ), especially because it almost achieves that except in the samples I provided it tends to be slightly too greedy.

Here are a few live example https://github.com/Particular/ServiceControl/pull/4377/commits/60c8d48e9140cfa54817ae638124567d468767bf

[manfred-brands]

@danielmarbach The lines you referred to say that instance.Property.Nested is not independent from instance.Property because it starts with the same prefx. It does say that instance.Values is independent from instance.Property and hence can be grouped.

What you want is looking at 3 items. Only if instance.Values is not followed by instance.Values[0] it is independent.

That would require maintaining more previous arguments in the loop and check something like assert[-1].IsIndependent(assert[0]) && assert[-2].IsIndependent(assert[-1])