-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Windows.Markup tests #138
Conversation
@hughbe hold on for a few days please. Looks like WPF team has plans for tests and we want to make sure your effort aligns with them. @dotnet/wpf-developers |
Okay no problem, from the readme:
These tests are aimed at getting high coverage for simple attributes and classes used around System.Xaml. They’re unit, not integration tests and take a really short time running so runtime shouldn’t be an issue |
@hughbe We're working on getting public test guidance out and having a plan for accepting great contributions like this. Apologies for not being completely prepared in this regard, we'll get there as soon as we can. Just to be clear, we're not concerned with the runtimes of these specific tests. Generally, the full CI test suite also contains a good amount of GUI end to end testing. This is necessary to validate core functionality of the product. Unfortunately, the expected runtime of such a set of tests is around 15-20 minutes. So we're hesitant to add to this test corpus until we have a better idea of what sort of impact everything has all up. That being said, our overall goal for testing is to port everything WPF has. Our feature tests are much larger (~24,000 tests total) and obviously can't be run as part of every CI build. So these will be added as a disjointed test set (we haven't decided exactly how just yet) that developers can use as we do internally to validate more technical changes and contribute new test coverage. I'll be working on this plan and broader test porting in the coming weeks and I'll update dotnet/wpf-test#105 accordingly. |
ea40a0b
to
6428bd1
Compare
Hi, everyone! Well, from this conversation I realized there is no published plan for testing, is not there? I would like to contribute to WPF test. Should I wait publishing of plans? |
@illialarka Yes, please wait on the plan to be published. We've been going over a set of plans for both getting the current tests we have from desktop framework ported and also allowing contributions in the short term from the community. In the next 2 or 3 weeks, we hope to have the guidance sorted and published. |
I'd like to move forward with this PR if there aren't any objections :) Basically what I've done here is written unit tests that cover the classes found in the System.Windows.Markup namespace. For this reason I don't think it belongs in DRT tests, but rather in another test project. Following .NET Core open source conventions, I've named it System.Xaml.Tests.csproj. I'm not sure whether this will automatically be run in the CI. I've tested it all locally, but thats something I'd appreciate a double check on that we run these unit tests in CI |
Thanks for not giving up on this @hughbe! |
Any progress with adding a unit test project for things like this? |
@@ -0,0 +1,15 @@ | |||
<Project Sdk="Microsoft.NET.Sdk"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you name this file "System.Xaml.UnitTests.csproj". i think that way it get's automatically picked up and ran by arcade.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<ProjectReference Include="..\..\src\System.Xaml\System.Xaml.csproj" /> | ||
</ItemGroup> | ||
<ItemGroup> | ||
<PackageReference Include="coverlet.msbuild" Version="2.4.0"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is this package reference for? do other dotnet repos use this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah - its used for code coverage /cc @ViktorHofer
using System.Collections.Generic; | ||
using Xunit; | ||
|
||
namespace System.Xaml.Tests.Common |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what makes these "Common"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh i see, they are test helper classes, i thought these were tests 😂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are your thoughts on using something like the Moq framework (like they use in winforms)
Would remove the need for this type of CustomXYZ class!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let me go take a look at how it's used in WinForms. just read the github page and it looks awesome!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'm starting to understand how it's used, and that does look pretty nice! I think it'll be pretty clear if you're able to make a small change and push an update? Don't change everything to it just in case, but it looks fairly righteous
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll need to push another change anyways to kick off another PR and CI run so we can see if the tests are running anyways :)
@hughbe I'm not sure, honestly this looks awesome and I love it. This is the direction we need our tests heading in. I'm going to queue up another build and will let you know if i see the tests running. |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
b6caaf9
to
a695515
Compare
Have rebased this PR and would welcome feedback & perhaps @ViktorHofer's help on the first commit to make sure I've got the new test project set up correctly |
First commit looks good to me 👌 |
|
||
namespace System.Xaml.Tests.Common | ||
{ | ||
public class CustomTypeDescriptorContext : ITypeDescriptorContext |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could look to use Moq for these instead of direct subclasses
Thanks @ViktorHofer! So nothing else is required for these tests to run automatically in CI etc? |
@hughbe, as of now your tests are not running on the CI. Since |
Apart from the ones on the CI, take a look at the artifacts :
|
Same error as dotnet/runtime#88518? /cc @ericstj |
Microsoft.Dotnet.Wpf.sln
Outdated
@@ -1910,6 +1920,22 @@ Global | |||
{AF9084C3-BF37-4A56-A851-89F3BAE731B3}.Release|x86.ActiveCfg = Release|Win32 | |||
{AF9084C3-BF37-4A56-A851-89F3BAE731B3}.Release|x86.Build.0 = Release|Win32 | |||
{AF9084C3-BF37-4A56-A851-89F3BAE731B3}.Release|x86.Deploy.0 = Release|Win32 | |||
{B2F2A89C-55C9-41B1-A645-0948609BD8BE}.Debug|Any CPU.ActiveCfg = Debug|Any CPU |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hughbe I think mapping these to the correct target platform (.sln x86 = .csproj x86, etc) should fix the error "There was a mismatch between the processor architecture of the project being built "MSIL" and the processor architecture of the reference".
/azp run |
No commit pushedDate could be found for PR 138 in repo dotnet/wpf |
f0ecaa6
to
94c2b9f
Compare
94c2b9f
to
f8dcdd8
Compare
f8dcdd8
to
bc59fdd
Compare
Looks like things are finally green!! |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
Thank you @hughbe! I apologize for the extended delay in merging this pull request. |
Woot woot! Glad this is in |
Bunch of tests for attributes, ValueSerializer, event args, converters etc in the System.Windows.Markup namespace
Assistance required: wrote these on my mac, they pass :) but not sure how to integrate into CI
Planning on writing more for the remaining stuff soon!
/cc @karelz