Runtime: 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.

<test name = "System.Drawing.Tests.ColorTranslatorTests.FromHtml_Invalid_Throws(htmlColor: \&quot;1,256,3\&quot;, exception: typeof(System.ArgumentException))" type="System.Drawing.Tests.ColorTranslatorTests" method="FromHtml_Invalid_Throws" time="0.0564322" result="Fail"> <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:

C# 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

[mellinoe]

I've written some tests for AdjustableArrowCap ( dotnet/corefx#21911 ), and will work on some tests for CustomLineCap when I have time this week.

[KostaVlev]

I will try to write tests and convert the old once for GraphicsPathIterator also. :)

[hughbe]

Awesome. I'm busy this weekend and was this week but I'm working on tests for Graphics
Once yours and @norek's PRs are in we should be getting to a good level of coverage - lots more to be done though!

[KostaVlev]

I have a little trouble here need somebody to confirm the following is correct.

The test method I wrote:

     public static IEnumerable<object[]> CopyData_StartEndIndexesOutOfRange_TestData()
    {
        yield return new object[] { new PointF[3], new byte[3], -1, 2 };
        yield return new object[] { new PointF[3], new byte[3], int.MinValue, 2 };
        yield return new object[] { new PointF[3], new byte[3], 0, 3 };
        yield return new object[] { new PointF[3], new byte[3], 0, int.MaxValue };
        yield return new object[] { new PointF[3], new byte[3], 2, 0 };
    }
    [ConditionalTheory(nameof(PlatformDetection) + "." + nameof(PlatformDetection.IsNotWindowsNanoServer))]
    [MemberData(nameof(CopyData_StartEndIndexesOutOfRange_TestData))]
    public void CopyData_StartEndIndexesOutOfRange_ReturnsExpeced(PointF[] points, byte[] types, int startIndex, int endIndex)
    {
        PointF[] resultPoints = new PointF[points.Length];
        byte[] resultTypes = new byte[points.Length];

        using (GraphicsPath gp = new GraphicsPath(points, types))
        using (GraphicsPathIterator gpi = new GraphicsPathIterator(gp))
        {
            Assert.Equal(0, gpi.CopyData(ref resultPoints, ref resultTypes, startIndex, endIndex));
        }
    }

I am testing CopyData() from GraphicsPathIterator.

According GdipPathIterCopyData()

Expected is:

resultCount = 0.

But actual is:

System.ArgumentException : Parameter is not valid.

System.EntryPointNotFoundException : Unable to find an entry point named 'ZeroMemory' in DLL 'kernel32.dll'.

Satck Trace

[norek]

@mellinoe @hughbe I need to suspend my work for 3-4 weeks ;( untill I'll buy new computer.

[KostaVlev]

Because GraphicsPathIterator was short and I got it done. I started to work on PathGradientBrush. Then I ran into weird behavior of the SetSigmaBellShape() method and I need help.

The test method I wrote:

    [ConditionalTheory(nameof(PlatformDetection) + "." + nameof(PlatformDetection.IsNotWindowsNanoServer))]
    [InlineData(1f, 1f)]
    [InlineData(0f, 1f)]
    [InlineData(0.5f, 1f)]
    // The cases below fail, actual scale is always 1.
    [InlineData(1f, 0f)]
    [InlineData(0f, 0f)]
    [InlineData(0.5f, 0f)]
    [InlineData(1f, 0.5f)]
    [InlineData(0f, 0.5f)]
    [InlineData(0.5f, 0.5f)]
    public void SetSigmaBellShape_FocusScale_Succes(float focus, float scale)
    {
        using (PathGradientBrush brush = new PathGradientBrush(_defaultFloatPoints))
        {
            brush.SetSigmaBellShape(focus);
            Assert.True(brush.Transform.IsIdentity);
            if (focus == 0f)
            {
                Assert.Equal(256, brush.Blend.Positions.Length);
                Assert.Equal(256, brush.Blend.Factors.Length);
                Assert.Equal(focus, brush.Blend.Positions[0]);
                Assert.Equal(scale, brush.Blend.Factors[0]);
                Assert.Equal(1f, brush.Blend.Positions[brush.Blend.Positions.Length - 1]);
                Assert.Equal(0f, brush.Blend.Factors[brush.Blend.Factors.Length - 1]);
            }
            else if (focus == 1f)
            {
                Assert.Equal(256, brush.Blend.Positions.Length);
                Assert.Equal(256, brush.Blend.Factors.Length);
                Assert.Equal(0f, brush.Blend.Positions[0]);
                Assert.Equal(0f, brush.Blend.Factors[0]);
                Assert.Equal(focus, brush.Blend.Positions[brush.Blend.Positions.Length - 1]);
                Assert.Equal(scale, brush.Blend.Factors[brush.Blend.Factors.Length - 1]);
            }
            else
            {
                Assert.Equal(511, brush.Blend.Positions.Length);
                Assert.Equal(511, brush.Blend.Factors.Length);
                Assert.Equal(0f, brush.Blend.Positions[0]);
                Assert.Equal(0f, brush.Blend.Factors[0]);
                //Assert.Equal(1f, brush.Blend.Positions[16]);
                //Assert.Equal(0f, brush.Blend.Factors[16]);
                Assert.Equal(1f, brush.Blend.Positions[brush.Blend.Positions.Length - 1]);
                Assert.Equal(0f, brush.Blend.Factors[brush.Blend.Factors.Length - 1]);
            }
        }
    }

To write the test I was looking at GdipSetPathGradientSigmaBlend() .

Stack trace

Is the actual value the desired one?

[hughbe]

@KostaVlev interesting.

I have a little trouble here need somebody to confirm the following is correct.

This looks like an area where Wine doesn't have the same argument validation as GDI+ on Windows. It sounds like a Wine bug. GDI+ on Windows is the de-facto standard we should be validate our tests against, so if GDI+ on Windows gives a result that Wine/libgdiplus doesn't have, then that' going to be a bug in those open source implementations.

FWIW, libgdiplus seems to have the same behaviour: https://github.com/mono/libgdiplus/blob/1cbd58ef57c208a8517a772f3c8416169d7dfd9b/src/graphics-pathiterator.c#L120-L141. It could be that Windows used to allow this but changed behaviour and the open source libraries never found out or updated.

I'm runnning the following code against netfx locally, and it does not throw an EntrypointNotFoundException. This looks like a porting bug - I can reproduce the throwing behaviour with netcoreapp. I'm opening an issue for this

using System.Drawing;
using System.Drawing.Drawing2D;

namespace Test
{
    public class Program
    {
        public static void Main()
        {
            // Throw ArgumentException.
            //CopyData_StartEndIndexesOutOfRange_ReturnsExpeced(new PointF[3], new byte[3], -1, 2);
            //CopyData_StartEndIndexesOutOfRange_ReturnsExpeced(new PointF[3], new byte[3], 0, 3);

            // Return 0
            CopyData_StartEndIndexesOutOfRange_ReturnsExpeced(new PointF[3], new byte[3], int.MinValue, 2);
            CopyData_StartEndIndexesOutOfRange_ReturnsExpeced(new PointF[3], new byte[3], 0, int.MaxValue);
            CopyData_StartEndIndexesOutOfRange_ReturnsExpeced(new PointF[3], new byte[3], 2, 0);
        }

        public static void CopyData_StartEndIndexesOutOfRange_ReturnsExpeced(PointF[] points, byte[] types, int startIndex, int endIndex)
        {
            PointF[] resultPoints = new PointF[points.Length];
            byte[] resultTypes = new byte[points.Length];

            using (GraphicsPath gp = new GraphicsPath(points, types))
            using (GraphicsPathIterator gpi = new GraphicsPathIterator(gp))
            {
                gpi.CopyData(ref resultPoints, ref resultTypes, startIndex, endIndex);
            }
        }

    }
}

[hughbe]

Is the actual value the desired one?

I reckon this is another case where Wine and libgdiplus have different behaviour than Windows GDI - it's good to get these tests written to help us start bridging the gaps between the implementations.

I'm not 100% sure I understand what the actual and desired values are in this case - could you expand?

[KostaVlev]

    private readonly PointF[] _defaultFloatPoints = new PointF[2] { new PointF(1, 2), new PointF(20, 30) };

    [ConditionalTheory(nameof(PlatformDetection) + "." + nameof(PlatformDetection.IsNotWindowsNanoServer))]
    [InlineData(1f, 1f)]
    [InlineData(0f, 1f)]
    [InlineData(0.5f, 1f)]
    // The cases below fail, actual scale is always 1.
    //[InlineData(1f, 0f)]
    //[InlineData(0f, 0f)]
    //[InlineData(0.5f, 0f)]
    //[InlineData(1f, 0.5f)]
    //[InlineData(0f, 0.5f)]
    //[InlineData(0.5f, 0.5f)]
    public void SetSigmaBellShape_FocusScale_Succes(float focus, float scale)
    {
        using (PathGradientBrush brush = new PathGradientBrush(_defaultFloatPoints))
        {
            brush.SetSigmaBellShape(focus);
            Assert.True(brush.Transform.IsIdentity);
            if (focus == 0f)
            {
                Assert.Equal(256, brush.Blend.Positions.Length);
                Assert.Equal(256, brush.Blend.Factors.Length);
                Assert.Equal(focus, brush.Blend.Positions[0]);
                Assert.Equal(scale, brush.Blend.Factors[0]); // Here expected is as follow 1, 0, 0.5 but Actual is 1, 1, 1
                Assert.Equal(1f, brush.Blend.Positions[brush.Blend.Positions.Length - 1]);
                Assert.Equal(0f, brush.Blend.Factors[brush.Blend.Factors.Length - 1]);
            }
            else if (focus == 1f)
            {
                Assert.Equal(256, brush.Blend.Positions.Length);
                Assert.Equal(256, brush.Blend.Factors.Length);
                Assert.Equal(0f, brush.Blend.Positions[0]);
                Assert.Equal(0f, brush.Blend.Factors[0]);
                Assert.Equal(focus, brush.Blend.Positions[brush.Blend.Positions.Length - 1]);
                Assert.Equal(scale, brush.Blend.Factors[brush.Blend.Factors.Length - 1]);  // Here expected is as follow 1, 0, 0.5 but Actual is 1, 1, 1
            }
            else
            {
                Assert.Equal(511, brush.Blend.Positions.Length);
                Assert.Equal(511, brush.Blend.Factors.Length);
                Assert.Equal(0f, brush.Blend.Positions[0]);
                Assert.Equal(0f, brush.Blend.Factors[0]);
                //Assert.Equal(focus, brush.Blend.Positions[255]);
                //Assert.Equal(scale, brush.Blend.Factors[255]); // Here expected is as follow 1, 0, 0.5 but Actual is 1, 1, 1
                Assert.Equal(1f, brush.Blend.Positions[brush.Blend.Positions.Length - 1]);
                Assert.Equal(0f, brush.Blend.Factors[brush.Blend.Factors.Length - 1]);
            }
        }
    }

I also was looking here.

@hughbe I think is not setting correct the start and end points. I added comments above where Asserts fails.

[KostaVlev]

I need to clear this situation and I will be pretty much ready and with the PathGradientBrush tests.

I simplified the test I wrote:

      private readonly PointF[] _defaultFloatPoints = new PointF[2] { new PointF(1, 2), new PointF(20, 30) };

      public static IEnumerable<object[]> Blend_FactorsPositions_TestData()
    {
        yield return new object[] { new float[1] { 1 }, new float[1] { 0 } };
        yield return new object[] { new float[1] { 1 }, new float[3] { 0, 3, 1 } };
    }

    [ConditionalTheory(nameof(PlatformDetection) + "." + nameof(PlatformDetection.IsNotWindowsNanoServer))]
    [MemberData(nameof(Blend_FactorsPositions_TestData))]
    public void Blend_ReturnsExpected(float[] factors, float[] positions)
    {
        int expectedSize = factors.Length;

        using (PathGradientBrush brush = new PathGradientBrush(_defaultFloatPoints, WrapMode.TileFlipXY))
        {
            brush.Blend = new Blend { Factors = factors, Positions = positions };
            Assert.Equal(new float[1] { 0 }, brush.Blend.Positions);
        }
    }

Expected is: float[1] { 0 }
Actual is: float[1]{random number} (well I think is random :)) number is in wide range. I got values from -3 to +7 running the test multiple times.

Not sure here if that inconstancy is acceptable.

The tested method

[mellinoe]

@KostaVlev It wasn't clear whether you are describing the behavior of the Windows implementation. That's the only one we are interested in validating -- any divergence from that by the other versions should be treated as a bug on those versions. If the Windows version is actually returning different values like you indicated, then the test needs to be changed (since it is asserting different behavior).

[KostaVlev]

@mellinoe I ran the test against netfx and returns expected 0 but running netcoreapp returns different numbers.

[hughbe]

Ohhh dear - can you make an issue?

[KostaVlev]

I figured out the questions I asked above. Seems like the entire time was just me :) ... I don't see anything free to take in System.Drawing.Drawing2D so I can start working in System.Drawing.Text

[mellinoe]

@KostaVlev Actually, we already have decent coverage of everything in System.Drawing.Text. Were you going to re-submit your tests for GraphicsPathIterator ? It is the last large type in Drawing2D that is untested. We could also use more test coverage for PathGradientBrush.

[KostaVlev]

@mellinoe I have the PathGradientBrush ready too. I will submitted it when we clear dotnet/corefx#22157

[mellinoe]

@KostaVlev Ah, great to hear. That will just about do it for Drawing2D, I believe.

[KostaVlev]

I don't see anybody working on System.Drawing.Imaging ImageAttributes and I think to take it...?

[hughbe]

@KostaVlev go ahead pal

[hughbe]

And thanks for jumping ahead and writing the tests. Something called life has got in the way of my contributions recently and @norek is unavailable too, so grab what you like (apart from Graphics.cs which I'm slowly working away on when I have time)

[hughbe]

I've just sent in a PR for BufferGraphics: https://github.com/dotnet/corefx/pull/22298

That's gonna be my last system.drawing pr for a couple of weeks so feel free to grab anything else including Graphics.cs which is the last big area missing tests

[KostaVlev]

I am trying to write test for ImageAttributes.SetOutputChannelColorProfile(). The test looks like this:

    [ConditionalFact(nameof(PlatformDetection) + "." + nameof(PlatformDetection.IsNotWindowsNanoServer))]
    public void SetOutputChannelColorProfile_Name_Success()
    {
        using (var bitmap = new Bitmap(_rectangle.Width, _rectangle.Height))
        using (var graphics = Graphics.FromImage(bitmap))
        using (var imageAttr = new ImageAttributes())
        {
            imageAttr.SetOutputChannel(ColorChannelFlag.ColorChannelC);
            imageAttr.SetOutputChannelColorProfile(Helpers.GetTestColorProfilePath("RSWOP.icm"));
            bitmap.SetPixel(0, 0, Color.FromArgb(255, 100, 100, 100));
            graphics.DrawImage(bitmap, _rectangle, _rectangle.X, _rectangle.Y, _rectangle.Width, _rectangle.Height, GraphicsUnit.Pixel, imageAttr);
            Assert.Equal(Color.FromArgb(255, 198, 198, 198), bitmap.GetPixel(0, 0));
        }
    }

I added hepper method GetTestColorProfilePath() and when I paste manually the RSWOP.icm file into F:\corefx\packages\system.drawing.common.testdata\1.0.2\content , the test works but how really do I add files to the packages? Am I allowed to do this?

[mellinoe]

@KostaVlev Please submit a PR to this repository, where the TestData packages are produced.

https://github.com/dotnet/corefx-testdata

All you need to do is add the file to the correct folder, and then update the nuspec: https://github.com/dotnet/corefx-testdata/blob/master/System.Drawing.Common.TestData.nuspec.

[mellinoe]

@KostaVlev I've updated a new version of System.Drawing.Common.TestData, version 1.0.3. You need to update this file in corefx in order to use the new version:

https://github.com/dotnet/corefx/blob/master/external/test-runtime/XUnit.Runtime.depproj#L77

You will also need to update the test project to point to the newer version: https://github.com/dotnet/corefx/blob/master/src/System.Drawing.Common/tests/System.Drawing.Common.Tests.csproj#L75

All of the parts that say 1.0.2 should be updated to 1.0.3. Bonus points if you define a property instead of repeating it, as well 😄

[KostaVlev]

@mellinoe Do you mean I can do something like

c# <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:

c# [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.
c# [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.