Adding System.Drawing.Common.Drawing2D GraphicsPath tests. #20711 #21866

KostaVlev · 2017-07-04T18:37:11Z

Converting mono GraphicsPath tests and adding few new.

dnfclas · 2017-07-04T18:37:15Z

@KostaVlev,
Thanks for your contribution.
To ensure that the project team has proper rights to use your work, please complete the Contribution License Agreement at https://cla2.dotnetfoundation.org.

It will cover your contributions to all .NET Foundation-managed open source projects.
Thanks,
.NET Foundation Pull Request Bot

dnfclas · 2017-07-04T18:50:59Z

@KostaVlev, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request.

Thanks, .NET Foundation Pull Request Bot

…WindowsNanoServer. (dotnet#21866)

danmoseley · 2017-07-05T02:55:06Z

src/System.Drawing.Common/tests/Drawing2D/GraphicsPathTest.cs

@@ -0,0 +1,3294 @@
+//


@KostaVlev thanks for the contribution. Please prefix with these two lines:

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

danmoseley · 2017-07-05T02:55:51Z

src/System.Drawing.Common/tests/Drawing2D/GraphicsPathTest.cs

+
+        private static IEnumerable<object[]> GetBounds_TestData()
+        {
+            yield 


remove dead comments?

Dan, I think the location of your comments were screwed up :P

ack, I wonder how that happened? I don't think it was me.

Something weird happened. There's quite a few random comments showing up on this line, including some of @hughbe .

danmoseley · 2017-07-05T02:56:09Z

src/System.Drawing.Common/tests/Drawing2D/GraphicsPathTest.cs

+
+        private static IEnumerable<object[]> GetBounds_TestData()
+        {
+            yield 


please remove assert.ignores.

danmoseley · 2017-07-05T02:56:48Z

src/System.Drawing.Common/tests/Drawing2D/GraphicsPathTest.cs

+
+        private static IEnumerable<object[]> GetBounds_TestData()
+        {
+            yield 


nit: 'says'

danmoseley · 2017-07-05T02:58:05Z

src/System.Drawing.Common/tests/Drawing2D/GraphicsPathTest.cs

+
+        private static IEnumerable<object[]> GetBounds_TestData()
+        {
+            yield 


All these Skips -- I assume these are from Mono, but do they fail on the Windows copy as well? If so we probably should just delete the tests.

danmoseley · 2017-07-05T02:58:58Z

src/System.Drawing.Common/tests/Drawing2D/GraphicsPathTest.cs

+
+        [ConditionalTheory(nameof(PlatformDetection) + "." + nameof(PlatformDetection.IsNotWindowsNanoServer))]
+        [InlineData(FillMode.Alternate, FillMode.Winding)]
+        public void Ctor_FillMode_Succses(FillMode alternate, FillMode winding)


Nit: "success"

and below..

danmoseley · 2017-07-05T03:00:08Z

src/System.Drawing.Common/tests/Drawing2D/GraphicsPathTest.cs

+        }
+
+        [ConditionalTheory(nameof(PlatformDetection) + "." + nameof(PlatformDetection.IsNotWindowsNanoServer))]
+        [InlineData(FillMode.Alternate, FillMode.Winding)]


I'm not sure what this InlineData achieves, the function just gets called once, right? If so it might as well be hard coded perhaps?

danmoseley · 2017-07-05T03:01:06Z

@dotnet-bot test Linux x64 Release Build please

hughbe

Nice! Good stuff here. Main comments are

Remove dead comments
Remove skips
Dispose of any places you call new GraphicsPath (sorry!)
Add param names to argument exceptions (don't forget to use AssertExtensions.Throws instead of Assert.Throws)
Use Assert.Equal(IEnumerable<T>, IEnumerable<T> instead of asserting each element one by one in an array

hughbe · 2017-07-05T03:42:37Z

src/System.Drawing.Common/tests/Drawing2D/GraphicsPathTest.cs

+    {
+        private class RunIfFontFamilyGenericMonospaceNotNull : TheoryAttribute
+        {
+            public RunIfFontFamilyGenericMonospaceNotNull()


Is there really a platform where GenericMonospace returns null? We have tests that would fail if this were the case here

I say just remove this

Agreed -- I don't think it's valid to just return "null" from GenericMonospace, so let's not consider it a possibility here.

hughbe · 2017-07-05T03:44:17Z

src/System.Drawing.Common/tests/Drawing2D/GraphicsPathTest.cs

+        [ConditionalFact(nameof(PlatformDetection) + "." + nameof(PlatformDetection.IsNotWindowsNanoServer))]
+        public void GraphicsPath_InvalidFillMode_ThrowsInvalidEnumArgumentException()
+        {
+            Assert.Throws<InvalidEnumArgumentException>(() => new GraphicsPath().FillMode = ((FillMode)int.MaxValue));


Please add the param name and use `AssertExtensions here and elsewhere for all argument exceptions

AssertExtensions.Throws<InvalidEnumArgumentException>("value", () => ...);

Also, could be now, or in the future: let's turn this type of method into a theory, and add more test cases for each side of the range

[ConditionalTheoy(nameof(PlatformDetection) + "." + nameof(PlatformDetection.IsNotWindowsNanoServer))] [InlineData(FillMode.Alternate - 1)] [InlineData(FillMode.Winding + 1)] public void GraphicsPath_InvalidFillMode_ThrowsInvalidEnumArgumentException(FillMode fillMode) { AssertExtensions.Throws<InvalidEnumArgumentException>("value", () => ...); }

Also, you need to dispose the graphics path here:

using (var graphicsPath = new GraphicsPath()) { }

hughbe · 2017-07-05T03:46:29Z

src/System.Drawing.Common/tests/Drawing2D/GraphicsPathTest.cs

+        [ConditionalFact(nameof(PlatformDetection) + "." + nameof(PlatformDetection.IsNotWindowsNanoServer))]
+        public void PathPoints_EmptyPath_ThrowsArgumentException()
+        {
+            Assert.Throws<ArgumentException>(() => Assert.Null(new GraphicsPath().PathPoints));


This is weird. Can you remove the Assert.Null?

hughbe · 2017-07-05T03:47:13Z

src/System.Drawing.Common/tests/Drawing2D/GraphicsPathTest.cs

+        [ConditionalFact(nameof(PlatformDetection) + "." + nameof(PlatformDetection.IsNotWindowsNanoServer))]
+        public void PathTypes_EmptyPath_ThrowsArgumentException()
+        {
+            Assert.Throws<ArgumentException>(() => Assert.Null(new GraphicsPath().PathTypes));


Could you add a param name, remove the assert.Null, and wrap the path in a using block?

I'll stop commenting on these

hughbe · 2017-07-05T03:47:22Z

src/System.Drawing.Common/tests/Drawing2D/GraphicsPathTest.cs

+        [InlineData(49, 157, 75, 196, 102, 209)]
+        public void AddLine_SamePoints_Success(int x1, int y1, int x2, int y2, int x3, int y3)
+        {
+


Nit: spaces at top of method

hughbe · 2017-07-05T04:02:03Z

src/System.Drawing.Common/tests/Drawing2D/GraphicsPathTest.cs

+
+        private static IEnumerable<object[]> GetBounds_TestData()
+        {
+            yield 


Woah! Long test! Maybe you could shorten it to

Assert.Equal(new int[] { 0, 1, 1, 129 ...}, gp.PathTypes);

hughbe · 2017-07-05T04:02:11Z

src/System.Drawing.Common/tests/Drawing2D/GraphicsPathTest.cs

+
+        private static IEnumerable<object[]> GetBounds_TestData()
+        {
+            yield 


hughbe · 2017-07-05T04:02:28Z

src/System.Drawing.Common/tests/Drawing2D/GraphicsPathTest.cs

+
+        private static IEnumerable<object[]> GetBounds_TestData()
+        {
+            yield 


hughbe · 2017-07-05T04:03:01Z

src/System.Drawing.Common/tests/Drawing2D/GraphicsPathTest.cs

+
+        private static IEnumerable<object[]> GetBounds_TestData()
+        {
+            yield 


This name is kind of meaningless now - can you see what the original bug was and modernize the name

hughbe · 2017-07-05T04:03:28Z

src/System.Drawing.Common/tests/Drawing2D/GraphicsPathTest.cs

+
+        private static IEnumerable<object[]> GetBounds_TestData()
+        {
+            yield 


Super duper nit: double new line

hughbe · 2017-07-05T04:05:39Z

src/System.Drawing.Common/tests/Drawing2D/GraphicsPathTest.cs

+using System.Drawing.Drawing2D;
+using Xunit;
+
+namespace Drawing2D


Namespace: System.Drawing.Drawing2D.Tests

hughbe · 2017-07-05T04:05:56Z

src/System.Drawing.Common/tests/Drawing2D/GraphicsPathTest.cs

+
+namespace Drawing2D
+{
+    public class GraphicsPathTest


Super nit: pluralize class name and file name to GraphicsPathTests

hughbe · 2017-07-05T04:06:51Z

src/System.Drawing.Common/tests/Drawing2D/GraphicsPathTest.cs

+            using (GraphicsPath gp = new GraphicsPath())
+            {
+                gp.AddRectangle(new Rectangle(1, 1, 2, 2));
+                Assert.Equal(1f, gp.PathPoints[0].X);


Maybe Assert.Equal(new PointF(1, 1), gp.PathPoints[0]) would be clearer here and elsewhere

hughbe · 2017-07-05T04:07:52Z

src/System.Drawing.Common/tests/Drawing2D/GraphicsPathTest.cs

+                gpf.AddLines(floeatPoints);
+                // all identical points are added
+                Assert.Equal(4, gpf.PointCount);
+                Assert.Equal(0, gpf.PathTypes[0]);


Could use Assert.Equal(IEnumerable<T>, IEnumerable<T>) here and elsewhere (I'll stop commenting)

e.g

Assert.Equal(new int[] { 1, 1, 1 }, gpf.PathTypes);

danmoseley · 2017-07-05T18:51:39Z

Thanks for doing the review @hughbe .

…phicsPath in using block. Adding param names to argument exceptions. Use Assert.Equal(IEnumerable<T>, IEnumerable<T> instead of asserting each element one by one in an array. dotnet#21866

KostaVlev · 2017-07-06T07:44:46Z

I think I managed to fix the suggested changes form @danmosemsft and @hughbe :)

mellinoe

Overall this is very comprehensive and well-written. Thanks for the large contribution! I made a few comments, mainly about minor nits, and some style / organization cleanup I'd like to see.

mellinoe · 2017-07-06T18:54:01Z

src/System.Drawing.Common/tests/Drawing2D/GraphicsPathTests.cs

+        }
+
+        [ConditionalFact(nameof(PlatformDetection) + "." + nameof(PlatformDetection.IsNotWindowsNanoServer))]
+        public void PathData_Says_CannotChange()


What does "Says" mean here? Can this just be PathData_CannotChange ?

mellinoe · 2017-07-06T18:57:00Z

src/System.Drawing.Common/tests/Drawing2D/GraphicsPathTests.cs

+        }
+
+        [ConditionalTheory(nameof(PlatformDetection) + "." + nameof(PlatformDetection.IsNotWindowsNanoServer))]
+        [InlineData(1, 1, 2, 2)]


It feels odd that this is a Theory with just a single InlineData -- primarily because the AssertLine calls below are assuming that only these data points are ever going to be passed in. Can AssertLine at least be changed so that these values are passed in to it?

I see that this is kind of a general pattern with much of the assertions in here. I think it's okay, but the use of InlineData is a little bit misleading, because the assertions in all these tests are very tightly coupled with these values, and they aren't parameterized. It would be a little bit cleaner if we do the following:

Don't use Theory/InlineData, and add some comments to the assertion methods so that it's obvious which hard-coded values they are expecting.

or

Parameterize the assertions and pass in these values.

This is more a comment about the readability / maintainability of the tests, not their correctness. The first option is probably better -- the second would be more complicated and we probably wouldn't get much out of it unless we added a bunch more test cases with different data.

mellinoe · 2017-07-06T18:58:01Z

src/System.Drawing.Common/tests/Drawing2D/GraphicsPathTests.cs

+        }
+
+        [ConditionalTheory(nameof(PlatformDetection) + "." + nameof(PlatformDetection.IsNotWindowsNanoServer))]
+        [InlineData(1, 1, 2, 2)]


Same comment about the InlineData and AssertLine

mellinoe · 2017-07-06T18:58:52Z

src/System.Drawing.Common/tests/Drawing2D/GraphicsPathTests.cs

+                new Point(x1, y1), new Point(x1, y1)
+            };
+
+            PointF[] floeatPoints = new PointF[]


typo: floatPoints

mellinoe · 2017-07-06T18:59:58Z

src/System.Drawing.Common/tests/Drawing2D/GraphicsPathTests.cs

+
+            PointF[] floeatPoints = new PointF[]
+            {
+                new PointF((float)x1, (float)y1), new PointF((float)x1, (float)y1),


I think these float casts are unnecessary.

mellinoe · 2017-07-06T21:10:40Z

src/System.Drawing.Common/tests/Drawing2D/GraphicsPathTests.cs

+
+            PointF[] floatPoints = new PointF[]
+            {
+                new PointF((float)x1, (float)y1), new PointF((float)x2, (float)y2),


nit: float casts unnecessary.

mellinoe · 2017-07-06T21:11:08Z

src/System.Drawing.Common/tests/Drawing2D/GraphicsPathTests.cs

+                Assert.Equal(20, gpi.PointCount);
+
+                gpf.AddClosedCurve(
+                    new PointF[3] { new PointF((float)x1, (float)y1), new PointF((float)x2, (float)y2), new PointF((float)x3, (float)y3) });


nit: float casts unnecessary. I'll stop commenting on this.

mellinoe · 2017-07-06T21:14:53Z

src/System.Drawing.Common/tests/Drawing2D/GraphicsPathTests.cs

+
+        [ConditionalTheory(nameof(PlatformDetection) + "." + nameof(PlatformDetection.IsNotWindowsNanoServer))]
+        [InlineData(1, 1, 2, 2)]
+        public void AddEllipse_Rectangl_Success(int x, int y, int width, int height)


typo: ~~Rectangl~~ Rectangle

mellinoe · 2017-07-06T21:15:18Z

src/System.Drawing.Common/tests/Drawing2D/GraphicsPathTests.cs

+        }
+
+        [ConditionalFact(nameof(PlatformDetection) + "." + nameof(PlatformDetection.IsNotWindowsNanoServer))]
+        public void AddPie_Rectangl_Success()


typo ~~Rectangl~~ Rectangle

mellinoe · 2017-07-06T21:38:51Z

src/System.Drawing.Common/tests/Drawing2D/GraphicsPathTests.cs

+            using (GraphicsPath clone = Assert.IsType<GraphicsPath>(gp.Clone()))
+            {
+                gp.Flatten(null);
+                Assert.Equal(gp.PointCount, clone.PointCount);


Hm, for these Flatten tests: won't PointCount be 0 for the entirety of this method anyways, for both GraphicsPaths? We aren't adding anything anywhere, just creating an empty path and cloning it. It seems like we might want to:

Add something to the path

Clone it

Flatten it

Then check the values of the original and clone

Does that make sense?

Hmm -- I see there are tests down below that do roughly that. This is okay, then.

… single InlineData attributes. (dotnet#21866)

…s. Test CopyData_StartEndIndexesOutOfRange_ReturnsExpeced() fails with exception System.EntryPointNotFoundException : Unable to find an entry point named 'ZeroMemory' in DLL 'kernel32.dll' , issue is tracked in (#22026). (#20711)

hughbe · 2017-07-10T09:59:33Z

@KostaVlev you have some merge conflicts:

git checkout dotnet/master
git pull
git checkout master
git rebase dotnet/master

Solve the merge conflicts, then

git add -A
git rebase --continue
git push -f

…the pull request is merged.

mellinoe · 2017-07-11T18:37:45Z

Hey @KostaVlev, your test changes look good to me now. I think your rebasing / merging went a bit wrong, though. The only remaining change in the PR is the test file change, but you'll also need to add it to the project file (which you previously had done). Can you add that change back in? I'd also like if you could rebase out some of the intermediate stuff, as well as the merge commits with master. Let me know if you're having trouble and I can try to help out.

KostaVlev · 2017-07-11T20:08:51Z

@mellinoe I am not sure how to rebase out commits..

mellinoe · 2017-07-11T20:40:30Z

@KostaVlev That is fine -- I will just squash the commits from this into a single commit before merging.

In the future, I would recommend just doing a git rebase --onto instead of merging the commits back in from master, especially for an isolated test addition like this.

…the pull request is merged.

…tnet#21866) * Adding System.Drawing.Common.Drawing2D GraphicsPath tests. #20711 * Adding ConditionalFact and ConditionalTheory. Tests will run if IsNotWindowsNanoServer. (dotnet#21866) * Removing dead comments. Removing skips, they don't work. Wrapping GraphicsPath in using block. Adding param names to argument exceptions. Use Assert.Equal(IEnumerable<T>, IEnumerable<T> instead of asserting each element one by one in an array. dotnet#21866 * Pluralizing the name GraphicsPathTest.cs dotnet#21866 * Changing the namespace to System.Drawing.Drawing2D.Tests * Some spelling corrections. Removing unnecessary float casts. Removing single InlineData attributes. (dotnet#21866) * Removing arguments from test methods that are not [Theory]. * Adding System.Drawing.Common.Drawing2D GraphicsPathIteratorTests tests. Test CopyData_StartEndIndexesOutOfRange_ReturnsExpeced() fails with exception System.EntryPointNotFoundException : Unable to find an entry point named 'ZeroMemory' in DLL 'kernel32.dll' , issue is tracked in (#22026). (#20711) * Updating System.Drawing.Common.Tests.csproj. * Removing not related change to dotnet#21866. I will commit this when the pull request is merged. * Reverting changes to System.Drawing.Common.Tests.csproj.

…efx#20711 (dotnet/corefx#21866) * Adding System.Drawing.Common.Drawing2D GraphicsPath tests. dotnet/corefx#20711 * Adding ConditionalFact and ConditionalTheory. Tests will run if IsNotWindowsNanoServer. (dotnet/corefx#21866) * Removing dead comments. Removing skips, they don't work. Wrapping GraphicsPath in using block. Adding param names to argument exceptions. Use Assert.Equal(IEnumerable<T>, IEnumerable<T> instead of asserting each element one by one in an array. dotnet/corefx#21866 * Pluralizing the name GraphicsPathTest.cs dotnet/corefx#21866 * Changing the namespace to System.Drawing.Drawing2D.Tests * Some spelling corrections. Removing unnecessary float casts. Removing single InlineData attributes. (dotnet/corefx#21866) * Removing arguments from test methods that are not [Theory]. * Adding System.Drawing.Common.Drawing2D GraphicsPathIteratorTests tests. Test CopyData_StartEndIndexesOutOfRange_ReturnsExpeced() fails with exception System.EntryPointNotFoundException : Unable to find an entry point named 'ZeroMemory' in DLL 'kernel32.dll' , issue is tracked in (dotnet/corefx#22026). (dotnet/corefx#20711) * Updating System.Drawing.Common.Tests.csproj. * Removing not related change to dotnet/corefx#21866. I will commit this when the pull request is merged. * Reverting changes to System.Drawing.Common.Tests.csproj. Commit migrated from dotnet/corefx@4d78ca0

Adding System.Drawing.Common.Drawing2D GraphicsPath tests. #20711

3eeb4b6

dnfclas added the cla-required label Jul 4, 2017

dnfclas added cla-signed and removed cla-required labels Jul 4, 2017

Adding ConditionalFact and ConditionalTheory. Tests will run if IsNot…

ec795bb

…WindowsNanoServer. (dotnet#21866)

danmoseley reviewed Jul 5, 2017

View reviewed changes

danmoseley requested a review from mellinoe July 5, 2017 03:00

hughbe reviewed Jul 5, 2017

View reviewed changes

mellinoe self-assigned this Jul 5, 2017

mellinoe added area-System.Drawing test enhancement Improvements of test source code labels Jul 5, 2017

Removing dead comments. Removing skips, they don't work. Wrapping Gra…

7a3f6d5

…phicsPath in using block. Adding param names to argument exceptions. Use Assert.Equal(IEnumerable<T>, IEnumerable<T> instead of asserting each element one by one in an array. dotnet#21866

KostaVlev added 2 commits July 6, 2017 10:58

Pluralizing the name GraphicsPathTest.cs dotnet#21866

b0193c3

Changing the namespace to System.Drawing.Drawing2D.Tests

1042c1c

karelz added this to the 2.1.0 milestone Jul 6, 2017

mellinoe suggested changes Jul 6, 2017

View reviewed changes

KostaVlev added 3 commits July 7, 2017 13:55

Some spelling corrections. Removing unnecessary float casts. Removing…

1818a80

… single InlineData attributes. (dotnet#21866)

Removing arguments from test methods that are not [Theory].

ee9945e

KostaVlev added 4 commits July 10, 2017 13:35

Updating System.Drawing.Common.Tests.csproj.

0a64e29

Merge branch 'master' into master

d5cf98e

Removing not related change to dotnet#21866. I will commit this when …

9e7f6f3

…the pull request is merged.

Merge branch 'master' of https://github.com/KostaVlev/corefx

2285f90

Reverting changes to System.Drawing.Common.Tests.csproj.

bb6557c

mellinoe merged commit 4d78ca0 into dotnet:master Jul 11, 2017

KostaVlev added a commit to KostaVlev/corefx that referenced this pull request Jul 12, 2017

Removing not related change to dotnet#21866. I will commit this when …

b8f8c2c

…the pull request is merged.

Adding System.Drawing.Common.Drawing2D GraphicsPath tests. #20711 #21866

Adding System.Drawing.Common.Drawing2D GraphicsPath tests. #20711 #21866

Conversation

KostaVlev commented Jul 4, 2017

dnfclas commented Jul 4, 2017

dnfclas commented Jul 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danmoseley commented Jul 5, 2017

hughbe left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danmoseley commented Jul 5, 2017

KostaVlev commented Jul 6, 2017 • edited

mellinoe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mellinoe Jul 6, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hughbe commented Jul 10, 2017

mellinoe commented Jul 11, 2017

KostaVlev commented Jul 11, 2017

mellinoe commented Jul 11, 2017

hughbe left a comment •

edited

KostaVlev commented Jul 6, 2017 •

edited

mellinoe Jul 6, 2017 •

edited