Add Tests for System.Drawing.Common

[mellinoe]

This issue tracks porting some set of tests from mono's test suite, covering the portions of System.Drawing that we support on .NET Core.

Mono's test cases are in this folder: https://github.com/mono/mono/tree/master/mcs/class/System.Drawing/Test

We most likely want to convert the most useful tests from all of the sections here, with the exception of System.Drawing.Design, which we aren't going to support right now on .NET Core (it is mainly related to designer / WinForms support).

Mono's tests use NUnit, so we will need to convert them to Xunit when copying them.

Additionally, I've identified that there will need to be some functional changes made to the tests themselves, as they do not pass against the .NET Framework implementation. We consider the .NET Framework implementation to be the compatibility baseline, so we should change the tests to accomodate it, rather than the other way around. The test failures seemed mainly related to very small, subtle differences in things like floating-point precision, color values, offsets, etc. We should do the following when we have both Windows and Unix implementations up and running:

• Ensure that all tests have enough "leniency" to accomodate floating-point differences (and other minor inconsistencies) where unavoidable.
• Ensure that all minor inconsistencies are acceptable or otherwise very difficult / problematic to fix.
• Ensure that all minor inconsistencies are listed somewhere, so that we can mention it both in the tests, and in the user documentation.

@hughbe @qmfrederik @marek-safar

---

Current Status

Code coverage: Note that there is a large amount of internal and debug-only code which distorts these numbers. When the coverage is generally high, we can clean out a lot of dead code and then get more accurate data.

Date	Branch Coverage	Line Coverage
6/26/2017	33%	25%
7/11/2017	49%	54%
7/17/2017	55%	60%
8/2/2017	58%	66%
8/25/2017	62%	71%
9/21/2017	64.6%	75.3%

Namespaces and coverage, as of 9/21/2017:

• System.Drawing: 76.3%
• System.Drawing.Drawing2D: 93.8%
• System.Drawing.Imaging: 86.1%
• System.Drawing.Printing: 62.8%
• System.Drawing.Text: 96%

[norek]

Hi I can grab some part of those tests for example System.Drawing.Printing, on start. Then when all will be fine i can port next part. What do you think?

[mellinoe]

@norek Splitting up the work sounds like a good idea to me. I've created separate items above for each of the folders in mono's test bed. I've put your name next to System.Drawing.Printing.

Looking over the tests in that folder, I think they are fairly straightforward, with a couple of exceptions. I would ignore the following tests for now:

• PrintingPermissionAttributeTest
• PrintingPermissionTest

We don't care about any of this permissions stuff in .NET Core, nor do we enforce it.

• PrintingServicesUnixTest

This one isn't going to work outside of Unix or the libgdiplus implemetation, so don't bother with it for now.

[norek]

@mellinoe insln file is reference to test project but it is missing in source code. Did you forgot push it?

[mellinoe]

@norek Oops -- it was a mistake to include it in the solution file. I had a couple of tests written on my local machine, but they weren't worth merging.

[norek]

I have little question about moving tests from Nunit to Xunit. In mono tests, very often Assert is used with description,prabobly beacuse there are more than one assert in test and description provide better debug (i think). What should I do with this case. Xunit doesn't provide description for Assert.Equal. Should I remove this description?

[mellinoe]

Yes -- remove the description. If really necessary, just provide the message in a comment next to the assertion.

[hughbe]

@norek a test project has been merged so feel free to get started. Mind if I ask what you're working on so we avoid conflicts?

Note that I'm gonna put some effort into beautifying and modernising these tests using xunit's features like parameter names in exceptions and theory/member data

Keep me up to date :)

[norek]

@hughbe Thanks! Im working on System.Drawing.Printing namespace. In description of this issue You can see list of assigned items.

[norek]

How about testing PageSettings class? In mono's test i found comment:

// Check for installed printers, because we need
// to have at least one to test
if (PrinterSettings.InstalledPrinters.Count == 0)

I dont know what to do with this.

[mellinoe]

@norek We certainly don't have any printers attached to the machines that run our CI tests, if that is what you are alluding to. You can just keep that check in the test code. Are you asking about something else?

[mellinoe]

@hughbe Could you post something here as you start writing/porting tests for individual components? It would be good to have a list of "in-progress" work so that we know what's being covered.

[norek]

@mellinoe yup, this is what I meant. Thanks

[norek]

@hughbe Can you tell me what is this? public static IEnumerable<(int, Color)> SystemColors_TestData() in ColorTranslatorTests.cs build is passing, but VS highlight this syntax. I see this syntax for first time ;o

second: In my machine after pull your changes, I have failing your tests for example: do you have idea why? clean, build on root completed.

<failure exception-type="Xunit.Sdk.ThrowsException">
<message><![CDATA[Assert.Throws() Failure\r\nExpected: typeof(System.ArgumentException)\r\nActual:   typeof(System.Exception): 1,256,3 is not a valid value for Int32.]]></message>
<stack-trace><![CDATA[at System.Drawing.ColorConverterCommon.IntFromString(String text, CultureInfo culture) in C:\Projects\corefx\src\Common\src\System\Drawing\ColorConverterCommon.cs:line 131
at System.Drawing.ColorConverterCommon.ConvertFromString(String strValue, CultureInfo culture) in C:\Projects\corefx\src\Common\src\System\Drawing\ColorConverterCommon.cs:line 64
at System.Drawing.ColorTranslator.FromHtml(String htmlColor) in C:\Projects\corefx\src\System.Drawing.Common\src\System\Drawing\ColorTranslator.cs:line 271
at System.Drawing.Tests.ColorTranslatorTests.<>c__DisplayClass10_0.<FromHtml_Invalid_Throws>b__0() in C:\Projects\corefx\src\System.Drawing.Common\tests\ColorTranslatorTests.cs:line 222]]></stack-trace>
</failure>
</test>```

[mellinoe]

Those are ValueTuples, a new C#7 feature. You will need VS 2017 to edit that, I believe...

[mellinoe]

@norek As for the test failure: what language settings does your computer use? We might be assuming that English is used, and number parsing could fail with other culture settings.

[norek]

@mellinoe oh my god...of course you are right, VS17 is answer in this case. And in second case... you are right too. Its written in fail message! I work on polish settings.

That was easy... I dont know what is happening to me this night! Thanks alot.

[mellinoe]

@norek Yep, the tests fail if your computer's language is Polish. This also happens on .NET Framework:

string htmlColor = "1,256,3";
var color = ColorTranslator.FromHtml(htmlColor);
Console.WriteLine("Color: " + color);

Unhandled Exception: System.Exception: 1,256,3 is not a valid value for Int32. ---> System.FormatException: Input string was not in a correct format.

We should override the current culture in the tests that check this stuff.

[hughbe]

If it's not pressing I can do this by tomorrow

[mellinoe]

I have a fix here: https://github.com/dotnet/corefx/pull/21016

[mellinoe]

@norek No problem. I have seen enough problems exactly like this that I've come to expect it 😄

[norek]

In PrinterUnitConvert.cs -> method UnitsPerDisplay have this default block in switch statement:

                    default:
                    Debug.Fail("Unknown PrinterUnit " + unit);
                    result = 1.0;

I know that for backward compatibility it shouldn't be converted to Exception but i think we can just remove this Debug.Fail. Otherwise we cannot test this block. What do you think?

[hughbe]

We should delete Debug statements that are failing, I agree. They were written before there were any unit tests.

[hughbe]

Eric, I've also given a stab at cleaning up some of the codebase with a view to use modern C# features etc. etc.

It's a big change but fairly montonous - removing devdocs, inline variables, fixup usings etc.

If you take a look here I've done an attempt for System.Drawing.Drawing2D. Do you think I should submit a PR? https://github.com/dotnet/corefx/compare/master...hughbe:drawing2d-cleanup-example

[norek]

@hughbe it should be different PR or can I attach this change to dotnet/corefx#21015?

[norek]

@hughbe for style changes it is created separate issue dotnet/runtime#22129. Hm its this good idea to completely remove comments? I thought they should be converted from doc for <summary>

[norek]

Due waiting on dotnet/corefx#21015 feedback I can grab System.Drawing.Common.Imaging.

[hughbe]

Yeah I'll submit a PR linking to that issue

[hughbe]

@mellinoe why is SystemDrawingSection commented out in the ref? I was going to write some tests as I noticed it's a public class in the source, but couldn't find it in the ref.

namespace System.Drawing.Configuration
{
    public sealed partial class SystemDrawingSection : System.Configuration.ConfigurationSection
    {
        public SystemDrawingSection() { }
        [System.Configuration.ConfigurationPropertyAttribute("bitmapSuffix")]
        public string BitmapSuffix { get { throw null; } set { } }
        protected override System.Configuration.ConfigurationPropertyCollection Properties { get { throw null; } }
    }
}

[KostaVlev]

Hi can I give a try to the System.Drawing.Drawing2D?

[mellinoe]

@KostaVlev Sure, go ahead! Take a look at the other recent PR's for inspiration on test style. Also check which PR's are currently in progress and avoid adding tests for those components.

@hughbe @norek Could you guys post a comment here mentioning which components you are planning on adding tests for?

[KostaVlev]

I didn't see anybody working on System.Drawing.Drawing2D GraphicsPathTest.cs so it's mine now :)

And here is a question. Will be the translation:

form: Assert.AreEqual (2.99962401f, rect.X, Delta, "Bounds.X"); to: Assert.True(Math.Abs(2.99962401f - rect.X) <= Delta);

accepted or there is a better way to check the tolerance between numbers?

[mellinoe]

@KostaVlev That looks fine; I'd recommend putting it into a shared helper function if one does not already exist. We could consider promoting this method into a helper class (although there would need to be a float version). @hughbe Pointers on that front?

[mellinoe]

@hughbe I think we can just add it in. I wasn't sure what the status of the ConfigurationManager library was when I was copying the code over, so I just left it commented out. We need to add a reference System.Configuration.ConfigurationManager if it's not already there.

[mellinoe]

Hm its this good idea to completely remove comments? I thought they should be converted from doc for

I'm gong to submit a few PR's which do this, and just clean up the comments in general.

[mellinoe]

We are currently at 33% branch coverage and 25% line coverage. That is only counting the tests that have been merged into master as of now. We've made pretty good progress so far, considering we started with 0% 😄 . Thanks for the help, everyone!

[hughbe]

@hughbe Pointers on that front?

Let's add a method to AssertExtensions. Something like

public class AssertExtensions
{
    public void Equal(float[] expected, float[] actual)
    {
        try
        {
            Assert.Equal(expected.Length, actual.Length);
            for (int i = 0; i < expected.Length; i++)
            {
                Equal(expected[i], actual[i]);
            }
        }
        catch
        {
            string expectedString = string.Join(", ", expected);
            string actualString = string.Join(", ", actual);
            throw new AssertActualExpectedException(expectedString, actualString, null);
        }
    }

    public void Equal(float expected, float actual)
    {
        if (double.IsInfinity(d1))
        {
            return AreSameInfinity(d1, d2 * 10);
        }
        if (double.IsInfinity(d2))
        {
            return AreSameInfinity(d1 * 10, d2);
        }
        double diffRatio = (d1 - d2) / d1;
        diffRatio *= Math.Pow(10, 6);
        return Math.Abs(diffRatio) < 1;
    }

    private static bool AreSameInfinity(float f1, float f1)
    {
        return
            float.IsNegativeInfinity(f1) == float.IsNegativeInfinity(f2) &&
            float.IsPositiveInfinity(f1) == float.IsPositiveInfinity(f2);
    }
}

We've already got something similar in the Matrix tests as well What do you think?

[mellinoe]

I think it makes sense to have an explicit "precision" parameter to at least one overload, which lets you specify a certain allowable difference. I think some variants of our tests may need that flexibility. It would probably also be a good idea to try and scour the codebase to see how many places could share this logic.

[hughbe]

@hughbe @norek Could you guys post a comment here mentioning which components you are planning on adding tests for?

I've currently shotgunned the System.Drawing root namespace - the big ones that are left are Font and Graphics (a big one)

I know @norek has grabbed System.Drawing.Printing and System.Drawing.Imaging, so it's wise you chose System.Drawing.Drawing2D.

[hughbe]

@mellinoe I tried adding the Configuration depedency to the ref, but I get the following errors:

System.Drawing.Common.cs(1275,6): error CS0012: The type 'Object' is defined in an assembly that is not referenced. You
 must add a reference to assembly 'netstandard, Version=2.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51'. [C:
\Users\hugh\Documents\GitHub\corefx\src\System.Drawing.Common\ref\System.Drawing.Common.csproj]
System.Drawing.Common.cs(1276,56): error CS0012: The type 'Object' is defined in an assembly that is not referenced. Yo
u must add a reference to assembly 'netstandard, Version=2.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51'. [C
:\Users\hugh\Documents\GitHub\corefx\src\System.Drawing.Common\ref\System.Drawing.Common.csproj]
System.Drawing.Common.cs(1281,28): error CS0012: The type 'Object' is defined in an assembly that is not referenced. Yo
u must add a reference to assembly 'netstandard, Version=2.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51'. [C
:\Users\hugh\Documents\GitHub\corefx\src\System.Drawing.Common\ref\System.Drawing.Common.csproj]
System.Drawing.Common.cs(1279,10): error CS0012: The type 'Object' is defined in an assembly that is not referenced. Yo
u must add a reference to assembly 'netstandard, Version=2.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51'. [C
:\Users\hugh\Documents\GitHub\corefx\src\System.Drawing.Common\ref\System.Drawing.Common.csproj]
System.Drawing.Common.cs(1279,31): error CS0012: The type 'Attribute' is defined in an assembly that is not referenced.
 You must add a reference to assembly 'netstandard, Version=2.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51'.
 [C:\Users\hugh\Documents\GitHub\corefx\src\System.Drawing.Common\ref\System.Drawing.Common.csproj]

Do you know what this is?

[hughbe]

I've also got a couple of tips for adding tests.

• Take a look at the Mono tests: they're patchy in some places and really comprehensive in others. Don't be surprised it they fail running against .NET Core though: .NET Core takes preference
• Do cleanup the Mono tests as well: it's a slight effort but really rewarding as it let's you...
• Add new tests that aren't covered by Mono
• Most of the behaviour differences will be in native code. Take a look at libgdiplus and WINE for each function you're testing. If you're adept at C++, look at argument validation and especially focus on the difference between those two open source implementations of gdi+. It'll guide you when you add tests for argument validation and corner cases.

[mellinoe]

@hughbe I believe the problem is that ConfigurationManager does not have a netcoreapp configuration set:

https://github.com/dotnet/corefx/blob/master/src/System.Configuration.ConfigurationManager/src/Configurations.props

That may be why I had disabled the code initially.

[hughbe]

Is it OK if we add one, or is this something that needs approval

[KostaVlev]

Not in 100% sure what to do when I see comment like this

// GetBounds (well GdipGetPathWorldBounds) isn't implemented

I think we don't need the assertions after the comment because the checks are done in the Gdpi and in case of failure the GetBounds() throws SafeNativeMethods.Gdip.StatusException .

Is it correct to remove the unnecessary assertions and try to add something like Assert.Throws<SafeNativeMethods.Gdip.StatusException>(....); ?

[mellinoe]

@hughbe I think we can just add one.

[mellinoe]

@KostaVlev Could you link to an example of that?

[KostaVlev]

For example this mono test and here is the GetBound().

I think if there is a invalid parameter we are getting SafeNativeMethods.Gdip.StatusException(status) that's why I was thinking I can remove the assertions after the comment:

// GetBounds (well GdipGetPathWorldBounds) isn't implemented

because this why looks like I am testing the result from SafeNativeMethods.Gdip.GdipGetPathWorldBounds().

[mellinoe]

@KostaVlev I'm still having a bit of trouble understanding.

It doesn't look like the test you linked expects any exceptions, does it? You can add a separate test which validates exception behavior, but you should base it on what the Windows implementation does, rather than mono. They will often be different, but the Windows version is preferred, and we will eventually want to make the mono version match it (to the greatest extent possible).

Also note that SafeNativeMethods.Gdip.StatusException is a method, not an exception type. It actually can throw one of many types of exceptions: https://github.com/dotnet/corefx/blob/1f055788ff130aacae11fae8c15584beaa7dd3b6/src/System.Drawing.Common/src/System/Drawing/Gdiplus.cs#L2626

[KostaVlev]

@mellinoe Thanks for the answer. I was lost for a moment but I got it now...

[KostaVlev]

I have this test failing because of the returned status code Ok here on line 970. Shouldn't it be InvalidParameter?

[hughbe]

If it returns Ok in .net core then update the test accordingly I say.

Also, sorry to say thi, but you're going to need to wrap all of those instantiations of any IDisposable objects - GraphicsPath, Matrix, etc. In a using block... it sucks I know ;p