Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Cleanups post move to net462 #3856

Merged
merged 6 commits into from
Jul 18, 2022
Merged

Cleanups post move to net462 #3856

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
fd7dba1
Fix TraitCollection label
Evangelink Jul 15, 2022
130ebe9
Add build var TargetNetFxVersion
Evangelink Jul 15, 2022
d94dff4
Reuse existing variables in scripts
Evangelink Jul 15, 2022
f51d90c
Fix Traits
Evangelink Jul 18, 2022
237b859
Add Directory.Build.props for new variable
Evangelink Jul 18, 2022
c1b5382
Update condition
Evangelink Jul 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions src/Microsoft.TestPlatform.ObjectModel/TraitCollection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,9 @@ public class TraitCollection : IEnumerable<Trait>
internal const string TraitPropertyId = "TestObject.Traits";
private static readonly TestProperty TraitsProperty = TestProperty.Register(
TraitPropertyId,
#if !NET462
// TODO: Fix this with proper resourcing for UWP and Win 8.1 Apps
// Trying to access resources will throw "MissingManifestResourceException" percolated as "TypeInitialization" exception
"Traits",
#else
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I got it backwards. This should be used: Resources.Resources.TestCasePropertyTraitsLabel,

Because the condition is a compiler directive, so net451 used Resources.Resources.TestCasePropertyTraitsLabel, and net462 should to, and net(coreapp) should probably too, even thought it used the harcoded value till now. I hpe the resource is just set to Traits :D

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The resource is set to Traits for English.

Why don't we simply use the hardcoded string for WINDOWS_UWP (and whatever the condition is for Win8.1)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it different for other languages? wtf? I hope we are not using that as an identifier. (would have been broken for netcoreapp so I think we don't)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it's being translated.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't understand the original condition. Basically for .NET 451 we use resx and for everything else we use hardcoded value. BUT comment says we want to use hardcoded for UWP and Win8.1. So we should have WINDOWS_UAP for "Traits" and everything else using resx. Am I missing something?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you are missing something, but there are 2 things:

  • we probably won't build for windows_uap anymore, because we will only update winUI runner which is netstandard2.0, so adding that condition is unnecessary.
  • because the condition was not updated when netcoreapp was introduced, netcoreapp was also using the hardcoded string, so changing the condtion is potentially a breaking change, and it would be interesting to have more context on how it is actually used.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • we probably won't build for windows_uap anymore, because we will only update winUI runner which is netstandard2.0, so adding that condition is unnecessary.

It seems we are still building it:
https://github.com/microsoft/vstest/blob/main/src/Microsoft.TestPlatform.ObjectModel/Microsoft.TestPlatform.ObjectModel.csproj#L9-L11

  • because the condition was not updated when netcoreapp was introduced, netcoreapp was also using the hardcoded string, so changing the condtion is potentially a breaking change, and it would be interesting to have more context on how it is actually used.

That's possible. So using the hardcoded string always would be the more alike to what was there before. I will add a TODO and a task to fix loading from a resource so we keep track of it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • We build it, but no-one is consuming. AFAIK. UWP runtime provider is not updated with those dll, because it needs to stick to supporting older versions of UWP than what we are able to support.
  • Hopefully there is another ID that is used, and this string is used only as a description, there are imho 2 names sent for the property, you should be able to see it in json messages on any TestRunChangedEventArgs (test run update).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on offline discussion, I have decided to make the hardcoded string special cased with the WINDOWS_UWP pragma as we publish uap under ObjectModel nuget. On the netcoreapp3.1 (or the follow-up PR) we will target only netstandard2.0 and so I will remove this case.

Resources.Resources.TestCasePropertyTraitsLabel,
#endif
typeof(KeyValuePair<string, string>[]),
#pragma warning disable 618
TestPropertyAttributes.Hidden | TestPropertyAttributes.Trait,
Expand Down