Analyzer unit tests are returning wrong assert values

[MisinformedDNA]

Analyzer unit tests are returning wrong assert values (off by one).

I have a successful unit test with the following ExpectedDiagnostic:

using VerifyCS = MisinformedDNA.PulumiAnalyzers.Test.CSharpAnalyzerVerifier<MisinformedDNA.PulumiAnalyzers.RequiredPropertiesAnalyzer>;

VerifyCS.Diagnostic().WithSpan(9, 9, 12, 10)

But when I change an expected value to be incorrect, the assert values are off by one:

For instance, with an expected diagnostic of VerifyCS.Diagnostic().WithSpan(10, 9, 12, 10), I get:

Message: Assert.AreEqual failed. Expected:<9>. Actual:<8>. Expected diagnostic to start on line "10" was actually on line "9"

Expected diagnostic:
    // /0/Test0.cs(10,9,12,10): error PULUMI0001: Type name '{0}' is missing required properties: {1}
VerifyCS.Diagnostic().WithSpan(10, 9, 12, 10),

Actual diagnostic:
    // /0/Test0.cs(9,9): error PULUMI0001: Type name '{0}' is missing required properties: {1}
VerifyCS.Diagnostic().WithSpan(9, 9, 12, 10).WithArguments("SkuName, TenantId"),

If I change the expected diagnostic to VerifyCS.Diagnostic().WithSpan(9, 19, 12, 10), I get:

Assert.AreEqual failed. Expected:<18>. Actual:<8>. Expected diagnostic to start at column "19" was actually at column "9"

These values should be consistent.

[sharwell]

Expected:<18>. Actual:<8>

This part is a verifier failure that occurs when comparing the internal representation, which uses 0-based indexing.

Expected diagnostic to start at column "19" was actually at column "9"

This part is a human-readable error message that is reported if the comparison fails, which uses 1-based indexing to match the APIs.

We could change it to make the internal comparison based on some other number, but I'm not sure it will make that much difference.

[MisinformedDNA]

I think the actual /expected numbers should be the same numbers that the developer /tester is used to working with. Internal values should not be leaked into tests.

[jmarolf]

In this case I don't see what Assert.AreEqual failed. Expected:<18>. Actual:<8>. really adds to the failure message. Personally I think we should just omit these entirely. In this case I think we should just say:

Message:
Expected diagnostic to start on line "10" was actually on line "9"

Expected diagnostic:
    // /0/Test0.cs(10,9,12,10): error PULUMI0001: Type name '{0}' is missing required properties: {1}
VerifyCS.Diagnostic().WithSpan(10, 9, 12, 10),

Actual diagnostic:
    // /0/Test0.cs(9,9): error PULUMI0001: Type name '{0}' is missing required properties: {1}
VerifyCS.Diagnostic().WithSpan(9, 9, 12, 10).WithArguments("SkuName, TenantId"),

[sharwell]

@jmarolf We aren't adding that part of the message. The unit test library (xunit/nunit/mstest) is adding it automatically in a form specific to that library.

[jmarolf]

right but its just and exception. We could always throw our own exception in order to hide this detail.