[System.Drawing] Add unit tests based on Mono's test suite by qmfrederik · Pull Request #23797 · dotnet/corefx

qmfrederik · 2017-09-05T15:14:16Z

When consolidating the Unix and Windows codebase for System.Drawing, I noticed some gaps in code coverage. For example, the various Graphics.*Clip* methods and properties did not have any coverage.

As a result, I tried to port the Mono test suite for System.Drawing to corefx. This PR is the result of that.

The tests uncovered a couple of bugs in the current implementation of System.Drawing:

The getter for PathGradientBrush.Blend returned inconsistent results across calls due to invalid P/Invoke code - 3a6f130
The P/Invoke declaration for GdipSaveImageToFile_delegate was missing a MarshalAs attribute for the filename parameter, causing the filenames to be passed in ANSI format to a function which expected Unicode, resulting in unexpected filenames.
The InvalidEnumArgumentException exception class exists in both System.Drawing and System.ComponentModel

qmfrederik · 2017-09-05T15:17:15Z

/cc @mellinoe @hughbe

In general, I think it's worth adding these tests to the test suite for System.Drawing as they were written when Mono's System.Drawing was written, and were used to assert that Mono's System.Drawing exposed the same behavior as the on on .NET.

In addition to that, they have the advantage of testing the System.Drawing + GDI+ layer, and tend to be "high level" tests of the framework. For example, lots of these tests load images or draw lines, arcs,... and then do a near-pixel-level validation of the result.

I'm aware there's still a lot of work to be done on these tests to get them in a fairly stable state, that would include

Code formatting standards
Storing blobs used as test asset in a NuGet package rather than git
...

It would make sense to conslidate these unit tests. A way forward would be to finish & merge this PR first, and then do the consolidation in separate PRs, just like we are doing for the actual codebase.

Thoughts?

ViktorHofer · 2017-09-05T16:57:56Z

Storing blobs used as test asset in a NuGet package rather than git

Once you commit sth into a git repository the files will stay there forever (because of history). If the files which are used here are large then I definitely suggest putting them into the accompanying repo for blobs before merging.

qmfrederik · 2017-09-05T17:29:20Z

The Linux builds fail with this error:

09:48:15 CSC : error CS1504: Source file '/mnt/j/workspace/dotnet_corefx/master/linux-TGroup_netcoreapp+CGroup_Release+AGroup_x64+TestOuter_false_prtest/src/System.Drawing.Common/tests/mono/System.Drawing.Imaging/IconCodecTest.cs' could not be opened
 -- Unable to load DLL 'api-ms-win-core-localization-l1-2-0.dll':
 The specified module or one of its dependencies could not be found. 
[/mnt/j/workspace/dotnet_corefx/master/linux-TGroup_netcoreapp+CGroup_Release+AGroup_x64+TestOuter_false_prtest/src/System.Drawing.Common/tests/System.Drawing.Common.Tests.csproj]

Does anyone know why CSC would try to load api-ms-win-core-localization-l1-2-0.dll on Linux?

qmfrederik · 2017-09-05T22:35:17Z

@dotnet-bot test Windows x64 Debug Build

qmfrederik · 2017-09-06T10:46:37Z

OK - the tests are passing on Windows which seems like a good start.

For Linux, some tests fail on some distros only: perhaps they have different versions of libgdiplus?
For Linux tests that fail on all Linux distros: these may be actual regressions on Linux as a result of the consolidation push. I'll have a closer look.

akoeplinger · 2017-09-06T14:33:49Z

For Linux, some tests fail on some distros only: perhaps they have different versions of libgdiplus?

If I remember correctly, at least the Bitmap2bit* and Save_* tests were due to some bugfix in newer libgdiplus.

One idea I had about how to handle cases like these would be to add an API to libgdiplus which returns the version numbers, so we could skip tests on broken versions.

qmfrederik · 2017-09-06T14:37:35Z

One idea I had about how to handle cases like these would be to add an API to libgdiplus which returns the version numbers, so we could skip tests on broken versions.

+1 for this one, we could also consider using it to prevent some code from executing if we know it would trigger issues in the underlying libgdiplus implementation.

akoeplinger · 2017-09-06T14:45:51Z

@@ -0,0 +1,29 @@
+#!/bin/sh


this file can be deleted

akoeplinger · 2017-09-06T14:46:17Z

@@ -0,0 +1,28 @@
+#!/bin/sh


this file can be deleted

akoeplinger · 2017-09-06T14:47:23Z

@@ -0,0 +1,2 @@
+/TestColorMatrix.cs -crlf


can probably be deleted as well

akoeplinger · 2017-09-06T14:48:05Z

@@ -0,0 +1 @@
+/tests-ms.sh -crlf


akoeplinger · 2017-09-06T14:48:34Z

+created by tests in the directory called MsNet.
+
+
+


can be deleted

qmfrederik · 2017-09-06T15:02:54Z

@akoeplinger Thanks! - I removed those files.

qmfrederik · 2017-09-06T16:01:57Z

According to pkgs.org, CentOS 7.3 and RHEL 7.3 are on version 2.10 of libgdiplus; Ubuntu 14.04 uses version 2.11.

libgdiplus 2.10 dates from Jan 13, 2011 (with the last commit on Oct 15, 2010), so that seems rather outdated :-).

@mellinoe Any way we can double-check which version of libgdiplus is installed on the build servers? The Mono project hosts package feeds for libgdiplus for most Linux distro's, perhaps it makes sens to use libgdiplus from that feed?

qmfrederik · 2017-09-07T06:25:48Z

Current status: 84 unit test failures. A fair share of them are on systems which I suspect are running old versions of libgdiplus (CentOS, RHEL and Ubuntu 14.04) - I'll go ahead & try to disable these tests on those systems.

qmfrederik · 2017-09-12T06:21:01Z

The last build saw 3 unit test failures in System.Drawing.Tests.BrushesTests/Brushes_Get_ReturnsExpected for YellowGreen. It appears Mono's unit tests actually break YellowGreen by a destructive test; I'll fix that.

The fixes for bugs uncovered by these unit tests have landed separately - via #23956, #23957 and #23958.

Work to keep the blobs out of git is also on it's way.

qmfrederik · 2017-09-13T22:36:57Z

@dotnet-bot test code coverage please

mellinoe · 2017-09-14T21:42:30Z

    <Reference Include="System.Runtime.InteropServices" />
    <Reference Include="System.ComponentModel" />
    <Reference Include="System.ComponentModel.Primitives" />
+    <Reference Include="System.ComponentModel.TypeConverter" />


We actually don't want this dependency. It was previously there and we removed it due to layering concerns.

And you are correct that we have duplicate InvalidEnumArgumentException types, but that is going to be okay for now. The one defined in System.Drawing.Common is internal, and the real type is being moved down into System.ComponentModel.Primitives, but that won't be available until .NET Core 2.1. In the meantime, we are building against .NET Core 2.0 and will just include an internal version of the exception.

OK, I'll revert the change. It causes one unit test to fail, I'll disable that test for now.

qmfrederik · 2017-09-17T20:13:09Z

@mellinoe & others, quick update:

I removed the change for InvalidEnumArgumentException and disabled the tests which check for this exception.
There's one bitmap file missing in the test data NuGet package, I sent out a PR for that.
Other than that, I have hope we can stabilize this PR this week, as in all unit tests passing.

What else do you need for this PR to merged? From a code style standpoint, I haven't change much yet to keep the diff with the upstream Mono tests fairly simple, but once the tests are stable I can tend to that.
Other than opening each file in VS and having VS auto-format the document (*), anything else to watch out for?

(*): When I make that change, I'll remember all the spaces I carefully put between the function name and the argument list, as well as the conversions from spaces to tabs and the countless times @akoeplinger had to remind me of Mono's code style ;-)

qmfrederik · 2017-09-18T18:21:44Z

@mellinoe Thanks, I've bumped the System.Drawing test data package, I'll keep an eye on the CI results as they come in.

mellinoe · 2017-09-18T18:34:53Z

Other than opening each file in VS and having VS auto-format the document (*), anything else to watch out for?

That will be a good start. Realistically, I don't care as much about the other stuff like variable names, etc. but the braces and tabs will be problematic if anyone formats the document with VS. Can we do that before we merge?

qmfrederik · 2017-09-18T18:38:47Z

@mellinoe Yup, I'll make sure all documents use the default Visual Studio code formatting. I'll do it once the CI is green, shouldn't be long before we get there.

qmfrederik · 2017-09-18T19:34:49Z

@mellinoe Two quick questions:

The NETFX x86 Release Build tests fail, but I don't seem to be able to see detailed test results (as in, which tests failed). Is there any way to retrieve them?
It also seems the System.Drawing tests are not running on macOS. Do you know whether libgdiplus has been installed on the macOS build servers?

It looks like the System.Threading.Tasks.Tests crashed on suse.422.amd64.Open, I'll relaunch the Linux tests.

qmfrederik · 2017-09-18T19:35:04Z

@dotnet-bot test Linux x64 Release Build please

mellinoe · 2017-09-18T19:37:06Z

It also seems the System.Drawing tests are not running on macOS. Do you know whether libgdiplus has been installed on the macOS build servers?

Yeah, I just noticed this in another PR as well. It is supposed to be installed on them via homebrew. I'll see if I can figure out what's wrong.

mellinoe · 2017-09-18T19:38:41Z

You should be able to see the full output of the netfx tests here:

https://ci3.dot.net/job/dotnet_corefx/job/master/job/windows-TGroup_netfx+CGroup_Release+AGroup_x86+TestOuter_false_prtest/3411/consoleText

There are quite a few tests that are failing in Drawing. If any of the mono tests are asserting behavior that doesn't match the .NET Framework behavior, we need to remove them.

qmfrederik · 2017-09-18T20:11:40Z

@mellinoe Thanks, some of the results are indeed consistent between Desktop & Core (oddly enough, also on Windows) so I disabled those tests for now.
Other tests were checking for ArgumentExceptions where the Core implementation has been changed to throw an ArgumentNullException. I've reverted that change for the time being (9c37c93) to keep the behavior consistent.

safern · 2017-09-18T20:38:23Z

MonoTests.System.Drawing.IconTest.Constructor_StreamNull
MonoTests.System.Drawing.IconTest.Constructor_StreamNull_Size

Are checking for ArgumentNullException also.

The remaining failures in NETFX are in PathGradientBrushTest class. You can repro them locally by running:

build.cmd -framework:netfx
msbuild <csproj> /t:rebuildandtest /p:targetgroup=netfx

danmoseley · 2017-09-19T18:22:04Z

@dotnet/dnceng is the InterruptedException above caused by the official build issue going on?

markwilkie · 2017-09-19T18:28:39Z

Only VSTS official builds are affected by the hotfix that broke our builds.

qmfrederik · 2017-09-19T18:32:32Z

@dotnet-bot test NETFX x86 Release Build please

mmitche · 2017-09-19T18:39:33Z

@danmosmsft This might have been caught by my restart this morning. Let me know if you see it again.

mellinoe · 2017-09-19T18:49:16Z

@@ -0,0 +1,170 @@
+//


We need to include a 2-line header in all of these new files.

// Licensed to the .NET Foundation under one or more agreements. // See the LICENSE file in the project root for more information.

Just to be clear, do you need this header in addition to the existing header or does it replace the existing header?

It is in addition to the existing header -- it just goes at the very top.

The files under System.Drawing.Common/src/Unix show what it should look like. Those are files pulled in from the mono codebase.

mellinoe · 2017-09-19T18:52:52Z

+        [ConditionalFact(Helpers.GdiplusIsAvailable)]
+        public void Ctor_Null()
+        {
+            using (GraphicsPathIterator gpi = new GraphicsPathIterator(null))


This is identical to an existing test that we already have checked in

https://github.com/dotnet/corefx/blob/master/src/System.Drawing.Common/tests/Drawing2D/GraphicsPathIteratorTests.cs#L61

mellinoe · 2017-09-19T18:54:55Z

+        }
+
+        [ConditionalFact(Helpers.GdiplusIsAvailable)]
+        public void NextMarker_Null()


This is identical to an existing test.

mellinoe · 2017-09-19T18:55:25Z

+        }
+
+        [ActiveIssue(20844)]
+        public void NextSubpath_Null()


This is identical to an existing test

mellinoe · 2017-09-19T18:55:53Z

+        }
+
+        [ConditionalFact(Helpers.GdiplusIsAvailable)]
+        public void CopyData_NullPoints()


This is identical to an existing test.

mellinoe · 2017-09-19T18:57:11Z

+        }
+
+        [ConditionalFact(Helpers.GdiplusIsAvailable)]
+        public void CopyData_NullTypes()


This is basically covered by an existing test. https://github.com/dotnet/corefx/blob/master/src/System.Drawing.Common/tests/Drawing2D/GraphicsPathIteratorTests.cs#L351

mellinoe · 2017-09-19T19:03:12Z

@qmfrederik I have started to look through the test code, and I'm concerned that there is a very large amount of overlap with existing tests. Before adding 30,000+ lines of code, we should be sure that it's adding real value to un-tested parts of the library. Have we done any de-duplication with what exists in corefx already? A big part of the problem here is that a majority of the test cases we have are already ported from mono's codebase -- just significantly cleaned up and improved. I've looked at the GraphicsPathIterator tests and I believe that most or all of the test cases are duplicates. In the cases I looked at, the code we have in corefx is better formatted and has more exhaustive coverage, as well.

I know this is a difficult problem, so I'm not sure how best to proceed. I think 30,000 lines of test code is probably too much for a single pull request, especially when there is such duplication. Can we split this up into a few individual PR's and try to do some de-duplication along the way?

danmoseley · 2017-09-19T19:39:48Z

Aside from @mellinoe 's feedback, note that our license header would need merging in to each of these files before the change could be merged. I'll mark it no merge for now, for that reason.

qmfrederik · 2017-09-19T19:49:56Z

@mellinoe Sure, we can split this up in multiple PRs. As to the value - yes, I believe there is value in this; #24140, #23958, #23957, #23956 for example are fixes for regressions that these tests caught.

My only concern is that some of the refactoring/consolidation we're doing impacts the codebase without us noticing (see the list of PRs before). On the other hand, we can keep this PR open for the time being - it would catch regressions, if any, whenever this is rebased.

In general, I think the Mono's test have the advantage of also testing the GDI+ code and how this library interacts with it.

mellinoe · 2017-09-19T20:22:05Z

@qmfrederik I definitely agree -- there's value in here, and you've already found and fixed issues because of it. I'd just like to do at least a basic scan of each class to make sure there's not excessively duplicated stuff. I think a little bit of overlap will be fine; like you mentioned we did a lot of refactoring so some of the new tests are not strictly equivalent to the old tests. Looking at the GraphicsPathIterator tests, I was able to spot a lot of duplicates just by looking at the method names. I think just a simple scan like that should be good enough to get rid of most of the duplication, and will leave us with a more manageable test bed.

If we separate this into a few PR's, I'd be happy to take one of the namespaces and do the de-duplication from your branch. That way we could split up the extra work I'm creating by asking for this 😄

qmfrederik · 2017-09-19T20:48:22Z

Sounds good :-). There are some fairly large files in there - Graphics and Bitmap come to mind. I'll split those off (as well as Metafile, which has very little code coverage at this point) and we can start with those.

Feel free to leave comments on the PR, send a PR to my branch, fork & create a separate PR, ... .

mellinoe · 2017-09-25T20:30:25Z

@qmfrederik I'm going to work on pulling out the Printing tests from here.

mellinoe · 2017-09-25T20:45:38Z

Actually, I looked through all of the Printing tests, and I don't think there's anything valuable there. Everything is a duplicate of what we already have, except for PrintingServicesUnixTest.cs. That test might be useful, but it won't work as-is (it uses direct PInvokes to libcups).

mellinoe · 2017-09-26T20:46:23Z

@qmfrederik As noted in #24263 , I looked at both of the System.Drawing.Text files you have here (there are only two) and found that our coverage is equal or better. I put up that PR to re-enable our existing tests everywhere.

qmfrederik · 2017-09-26T20:48:33Z

Thanks for letting me know! I'll remove that file from this PR - may help as a kind of bookkeeping as to which tests are redundant and which test we may want to consider porting over.

mellinoe · 2017-09-26T20:50:36Z

@qmfrederik Yep, sounds good. I'm trying to pick off the outer stuff since it will be quick and I don't want to conflict with your current PR's.

I just looked at ImageFormatTests.cs, and it is also duplicate coverage. Our version is pretty much identical in function, just refactored a little bit to use [Theory].

mellinoe · 2017-09-26T20:53:45Z

WmfPlaceableFileHeaderTest.cs is the same thing as our tests/Imaging/WmfPlaceableFileHeaderTests.cs, ours is just refactored slightly (again with [Theory]).

karelz · 2017-10-11T18:31:23Z

@safern what is the remaining work here?

safern · 2017-10-11T19:19:18Z

@safern what is the remaining work here?

@qmfrederik broke this PR into multiple PRs to find test duplication and make reviewing easier. So I will just wait for his confirmation that we can close this PR.

karelz · 2017-10-11T19:39:11Z

In that case I would recommend to close it right away as we do not plan to merge it ever. The content can be easily harvested and reused even if it is closed ... @qmfrederik what do you think?

safern · 2017-10-11T19:42:00Z

This is why we kept it open: #23797 (comment)

karelz · 2017-10-11T19:44:49Z

we can keep this PR open for the time being - it would catch regressions, if any, whenever this is rebased

Is anyone planning to do that anytime soon? If yes, sure ... if not, let's close it.

…Drawing\Test) * Remove CAS-related attributes * NUnit to Xunit - Replace using statements - [Test] -> [Fact] - Assert.Equals => Assert.Equal - Assert.IsTrue => Assert.True - Assert.IsFalse => Assert.False - Assert.IsNull => Assert.Null - Assert.IsNotNull => Assert.NotNull - Port Assert.Fail - Assert fixes - Replace [TestFixture] attribute - Fix calls to Assert.*: remove the user message - Various fixes - Remove TestFixture attribute - Fix Assertion * Remove outdated tests - Remove tests in the NotWorking category - Remove Ignored tests - Remove CAS tests - Delete PrintingPermissionTests as PrintingPermission/CAS is gone. - Remove ColorConverterTests * Fix precision on unit tests * Disable tests which fail due to behavioral differences * Disable ToLogFont_Int * Disable tests which require printers to be installed * Disable 24bpp and 32bpp tests on Linux distros with an old version of libgdiplus. * Don't dispose the YellowGreen brush as part of the tests * Use test bitmaps from the NuGet package * Disable tests which check for InvalidEnumArgumentException, as this class is internal to System.Drawing.Common * Use standard Visual Studio code formatting. * Expect ArgumentException on Desktop and ArgumentNullException on Core * Remove duplicate tests - Remove duplicate printing tests - Remove duplciate ImageFormat tests - Remove duplicate System.Drawing.Text tests - Remove duplicate WmfPlaceableFileHeaderTest.cs * Remove codec tests

qmfrederik · 2017-10-12T19:49:22Z

I just rebased to see whether these tests are still in a good shape ;-).

I'll try to go over the PR again this weekend/early next week to see if there are obvious tests that we can split off in separate PRs or if it's really just duplicates at this point and we can close the PR.

danmoseley · 2017-10-12T19:55:01Z

Thanks @qmfrederik we appreciate your work here.

qmfrederik · 2017-10-25T19:29:48Z

Closing for now, I hope to be able to come back to this in the near future.

dnfclas added the cla-already-signed label Sep 5, 2017

akoeplinger reviewed Sep 6, 2017

View reviewed changes

karelz added the area-System.Drawing label Sep 6, 2017

karelz assigned qmfrederik, mellinoe and safern Sep 6, 2017

qmfrederik mentioned this pull request Sep 11, 2017

Add additional test assets for System.Drawing based on Mono's unit tests dotnet/runtime-assets#19

Merged

mellinoe reviewed Sep 14, 2017

View reviewed changes

mellinoe reviewed Sep 19, 2017

View reviewed changes

danmoseley added the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Sep 19, 2017

karelz unassigned mellinoe Oct 11, 2017

qmfrederik closed this Oct 25, 2017

karelz added this to the 2.1.0 milestone Oct 28, 2017

Conversation

qmfrederik commented Sep 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qmfrederik commented Sep 5, 2017

Uh oh!

ViktorHofer commented Sep 5, 2017

Uh oh!

qmfrederik commented Sep 5, 2017

Uh oh!

qmfrederik commented Sep 5, 2017

Uh oh!

qmfrederik commented Sep 6, 2017

Uh oh!

akoeplinger commented Sep 6, 2017

Uh oh!

qmfrederik commented Sep 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qmfrederik commented Sep 6, 2017

Uh oh!

qmfrederik commented Sep 6, 2017

Uh oh!

qmfrederik commented Sep 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qmfrederik commented Sep 12, 2017

Uh oh!

qmfrederik commented Sep 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qmfrederik commented Sep 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qmfrederik commented Sep 18, 2017

Uh oh!

mellinoe commented Sep 18, 2017

Uh oh!

qmfrederik commented Sep 18, 2017

Uh oh!

qmfrederik commented Sep 18, 2017

Uh oh!

qmfrederik commented Sep 18, 2017

Uh oh!

mellinoe commented Sep 18, 2017

Uh oh!

mellinoe commented Sep 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qmfrederik commented Sep 18, 2017

Uh oh!

safern commented Sep 18, 2017

Uh oh!

danmoseley commented Sep 19, 2017

Uh oh!

markwilkie commented Sep 19, 2017

Uh oh!

qmfrederik commented Sep 19, 2017

Uh oh!

mmitche commented Sep 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

qmfrederik commented Sep 5, 2017 •

edited

Loading

qmfrederik commented Sep 7, 2017 •

edited

Loading

qmfrederik commented Sep 17, 2017 •

edited

Loading

mellinoe commented Sep 18, 2017 •

edited

Loading

mellinoe commented Sep 26, 2017 •

edited

Loading

safern commented Oct 11, 2017 •

edited

Loading