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.

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

Cleanups post move to net462 #3856

Merged
merged 6 commits into from
Jul 18, 2022
Merged

Conversation

Evangelink
Copy link
Member

@Evangelink Evangelink commented Jul 15, 2022

Follow-up from #3646 (see #3646 (comment))

@Evangelink Evangelink changed the title Fix TraitCollection label Cleanups post move to net462 Jul 15, 2022
// 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.

@Evangelink Evangelink enabled auto-merge (squash) July 18, 2022 14:49
@Evangelink Evangelink merged commit bfe5fef into microsoft:main Jul 18, 2022
@Evangelink Evangelink deleted the net462-cleanup branch July 18, 2022 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants