-
Notifications
You must be signed in to change notification settings - Fork 1.2k
packaging test updates and updates to packaging tests #1111
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
Conversation
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.
Sdk style projects should not have ToolsVersion
. Something’s off with the solution configuration or your VS setup if those were auto generated...
Yeah VS is auto-generating this and won't load the project without it. I'll try updating my VS2019 install to see if that helps |
...Wpf/test/XAML/DRT/DrtXaml/XamlTestClasses.FriendWithKey/XamlTestClasses.FriendWithKey.csproj
Show resolved
Hide resolved
@@ -1,9 +1,32 @@ | |||
<Project> | |||
<ItemGroup> | |||
<!-- | |||
List of Drts to run that are built out of dotnet/wpf. Note that this is incredibly | |||
List of Drts (grouped by area) to run that are built out of dotnet/wpf. Note that this is incredibly |
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.
Think about this mechanism being expanded to FeatureTests. It could get pretty unwieldy.
Maybe we won't have a similar style of things going on there, but we should probably be sure that what mechanisms we use for DRTs are very similar to FeatureTests. Maybe these should be automatically generated by a target parsing the QV files (like DrtList.xml and DiscoveryInfo.xml).
Maybe the best course of action would be using QV itself to generate the list that gets batched into Helix because then we could apply the discovery mechanism with specific variables that can trim/alter the test list.
Probably not a now thing, but might be something to think about going forward.
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 do we do for feature tests, is there a FeatureTestList.xml like DRTs? My plan is to generate this file based on that list, which should be fairly straightforward to do
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.
Unfortunately FeatureTests aren't that straightforward. In the same directory with DrtList, is DiscoveryInfo,xml. This doesn't list everything though, you need to use QV to get the full test discovery I believe, which is how I generated that excel file we had with the test lists.
I had to hack QV for that, but you can probably add an option to spit out the discovery list and we can run it as a one off.
@miguep None of this sounds completely incorrect does it? : - )
Allow test projects access to internals (but only those that require it). Arcade will generate a file like | ||
"WindowsBase.InternalsVisibleTo.cs" with the proper assembly name and PublicKey for us | ||
--> | ||
<InternalsVisibleTo Include="DrtPackagingApis" /> |
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 would recommend adding this to OtherAssemblyAttrs.cs
directly, and introducing the assembly name into BuildInfo
first under shared\RefAssemblyAttrs.cs
.
This allows us to maintain a single uniform system for tracking InternalsVisibleTo
relationships. Having this encoding inside a csproj
makes this harder to maintain, and potentially troubleshoot problems in the future.
Any assemblies requested internals access to WindowsBase
etc. should have its key fingerprint declared clearly in RefAssebmlyAttrs.cs
. Having a separate section in there for test assemblies would be preferable.
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.
That seems like a lot more maintenance and overhead. Arcade generates this all for us based on the key you are using and all you have to is add this in your .csproj. We could go about removing all of that and moving to this model, but that would be out of scope for this PR.
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.
When we tried to remove this from otherassemlyattrs.cs
early in .NET Core 3 and "clean up" this information, we broke API compat widely and spent several days chasing the mistakes and fixing problems in the build, then resurrecting this information from history.
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 elaborate more on what happened? how does changing InternalsVisibleTo
attributes break API compat?
@stevenbrix If you're making bug fixes to DRTs, please make sure there are separate issues for them (apart from the conglomerate issues we have) and that you immediately port these to the internal test repo. If this PR takes more time to get merged, then you will be leaving us with test issues in our private runs in the interim period and possibly someone will try to fix them not realizing that you've already done so. |
this allows test projects to load correclty in VS without being migrated
getting rid of code that wasn't being used before
1d06f2a
to
bc7b329
Compare
I took advantage of this unique situation to give myself a clever little title ;)
This PR does a few things:
Microsoft.DotNet.Wpf.DncEng.Test
package that contains our test infrastructure that compiles againstMicrosoft.NETCore.App
instead ofMicrosoft.WindowsDesktop.App
. This should be a fairly temporary solution until we open source the infrastructure entirely. There will be an internal PR that consumes this, and then once that is done we can update this repo to consume it.Fixes #1051
Fixes #1052
Fixes #816