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: dotnet/corefx#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?
dotnet/corefx@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 #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

[KostaVlev]

@mellinoe Do you mean I can do something like

<PropertyGroup>
    <TestsDataVersion>1.0.3</TestsDataVersion>
</PropertyGroup>

in the System.Drawing.Common.Tests.csproj file?

[mellinoe]

@KostaVlev Yes, that's right. For now, though, just go ahead and hard-code the new version. I may actually want to create a property all the way up in https://github.com/dotnet/corefx/blob/master/dependencies.props, and use that property everywhere in the repo, including the test-runtime project file. That way it would only be specified once.

[KostaVlev]

Hi, the next one which isn't tested in Imaging is Metafile. I started working on it but :) I need a little bit help here.

I wrote this test:

[ConditionalFact(nameof(PlatformDetection) + "." + nameof(PlatformDetection.IsNotWindowsNanoServer))]
public void Ctor_IntPtrWmfPlaceableFileHeader_Success()
{
    using (var bufferGraphics = Graphics.FromHwndInternal(IntPtr.Zero))
    using (var metafile = new Metafile(bufferGraphics.GetHdc(), new WmfPlaceableFileHeader()))
    {
        Assert.Equal(new Rectangle(0, 0, 0, 0), metafile.GetMetafileHeader().Bounds);
    }
}

My idea is to create blank Metafile but I am getting System.Runtime.InteropServices.ExternalException : A generic error occurred in GDI+. when calling the constructor.

The exception occurs on every platform.

Any hints how to make this to work?

[mellinoe]

@KostaVlev Which call is throwing the exception? I'm pretty sure it is not valid to call FromHwndInternal using a null handle like that, so I'm not sure such a test is really valuable. A test could be added to assert that an ExternalException is thrown, but again, I don't think it is valuable, and I don't think we should care about that level of behavior compat to emulate it on the Unix version.

[KostaVlev]

@mellinoe The graphics object is created successfully var metafile = new Metafile(bufferGraphics.GetHdc(), new WmfPlaceableFileHeader()) throws the exception.

It tried this way too but still getting exception.

[ConditionalFact(nameof(PlatformDetection) + "." + nameof(PlatformDetection.IsNotWindowsNanoServer))]
 public void Ctor_IntPtrWmfPlaceableFileHeader_Success()
{
    using (var bitmap = new Bitmap(_rectangle.Width, _rectangle.Height))
    using (var bufferGraphics = Graphics.FromImage(bitmap))
    using (var metafile = new Metafile(bufferGraphics.GetHdc(), new WmfPlaceableFileHeader()))
    {
        Assert.Equal(new Rectangle(0, 0, 0, 0), metafile.GetMetafileHeader().Bounds);
    }
}

Since we have same behavior for netfx and netcoreapp should I skip this test for now?

[mellinoe]

Since we have same behavior for netfx and netcoreapp should I skip this test for now?

Yes

[KostaVlev]

I moved on System.Drawing.Printing.

[hughbe]

<3 good stuff

[mellinoe]

Code coverage numbers are looking really good to me (updated at the top), considering there are a few large files full of dead code (or code that doesn't really need to be tested). Thanks for all of the contributions, everyone!

[norek]

How nice, a lot of work was made since my "vacation". I just bought new computer and i'm ready to go!

[mellinoe]

Some more steady progress was made in the past month. I think @qmfrederik's current effort will give us enough coverage to fill in the remaining gaps and let us close this issue out.

[ghost]

Due to lack of recent activity, this issue has been marked as a candidate for backlog cleanup. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will undo this process.

This process is part of our issue cleanup automation.

[ghost]

This issue will now be closed since it had been marked no-recent-activity but received no further activity in the past 14 days. It is still possible to reopen or comment on the issue, but please note that the issue will be locked if it remains inactive for another 30 days.