Add System.Drawing.Region tests #20901
Conversation
| @@ -0,0 +1,1930 @@ | |||
| // Licensed to the .NET Foundation under one or more agreements. | |||
| // The .NET Foundation licenses this file to you under the MIT license. | |||
There was a problem hiding this comment.
I believe the middle line here needs to come out if it's Mono code:
1. Keep the existing copyright headers in place
○ Note: If you are porting just portions of the file over, you still need to bring over all the existing copyright headers.
2. Add the following copyright headers – NOTE this is missing the middle line of the ‘regular’ CoreFx copyright headers:
// Licensed to the .NET Foundation under one or more agreements.
// See the LICENSE file in the project root for more information.
Although if only part is, I'm not sure. @AlexGhiondea did legal indicate what to do in that case?
There was a problem hiding this comment.
@danmosemsft I didn't ask them about this case in particular. Could we split the file so that whatever came from Mono is in a single file and the new tests in a separate one?
There was a problem hiding this comment.
That's going to be tricky. Basically these tests aren't really entirely from Mono - I'm just using their test data interweaved amongst tests I added to increase coverage
There was a problem hiding this comment.
Applying common sense: I would just keep the Mono "copyright" there. Nothing prevents us to extend Mono files and modify them. Even upon initial checkin AFAIK. The key thing is to acknowledge where it came from. Of course, I am not a lawyer, maybe we should ask ...
There was a problem hiding this comment.
You expect common sense of law!? 😃
But, seriously, do you mean both the net foundation header and the mono one or just mono?
There was a problem hiding this comment.
Aren't you supposed to be the law expert here? 😆
The rule I heard was having both - Mono & .NET Foundation at the top of the file. Again, we may need to check. HttpListener code came from Mono, so you might want to check there.
There was a problem hiding this comment.
@karelz the question is not whether to have both, it's whether to remove this line // The .NET Foundation licenses this file to you under the MIT license.
@AlexGhiondea since you had the original thread with legal, could you please ask them whether we should have that line if there's non-Mono content in there? (Presumably pretty much all the files we borrowed from Mono will eventually have non-Mono content in there)
I don't think that should hold up checking this in - either way around :)
|
@mellinoe can you please double-check we keep Mono license header? There were some rules around that ... |
| { | ||
| public class RegionTests | ||
| { | ||
| private static Graphics s_graphic = Graphics.FromImage(new Bitmap(1, 1)); |
| public void Ctor_EmptyGraphicsPath_ThrowsExternalException() | ||
| { | ||
| var region = new Region(new GraphicsPath()); | ||
| Assert.Throws<ExternalException>(() => new Region(region.GetRegionData())); |
There was a problem hiding this comment.
Could you split this into two lines so it is clear which call should be throwing an exception?
| [ConditionalFact(nameof(PlatformDetection) + "." + nameof(PlatformDetection.IsNotWindowsNanoServer))] | ||
| public void Ctor_GraphicsPath() | ||
| { | ||
| var graphics = new GraphicsPath(); |
There was a problem hiding this comment.
This variable should probably be called graphicsPath.
|
Addressed all the PR feedback |
|
The latest version looks good. You will just need to rebase because I've merged other PR's. |
|
Rebased |
A 50-50 mix of tests cleanup up from Mono, and tests I wrote myself
@mellinoe