-
Notifications
You must be signed in to change notification settings - Fork 326
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
Fixed bugs in ManagedMethod parsing, and updated hierarchies. #3704
Fixed bugs in ManagedMethod parsing, and updated hierarchies. #3704
Conversation
333a48b
to
e4650ed
Compare
e4650ed
to
94960c3
Compare
Can you add more info to the PR description please? |
...Microsoft.TestPlatform.AdapterUtilities/ManagedNameUtilities/ManagedNameHelper.Reflection.cs
Show resolved
Hide resolved
...Microsoft.TestPlatform.AdapterUtilities/ManagedNameUtilities/ManagedNameHelper.Reflection.cs
Outdated
Show resolved
Hide resolved
...Microsoft.TestPlatform.AdapterUtilities/ManagedNameUtilities/ManagedNameHelper.Reflection.cs
Outdated
Show resolved
Hide resolved
...Microsoft.TestPlatform.AdapterUtilities/ManagedNameUtilities/ManagedNameHelper.Reflection.cs
Outdated
Show resolved
Hide resolved
94960c3
to
af50d49
Compare
b6f3ecb
to
4daa7eb
Compare
4daa7eb
to
3827a8e
Compare
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.
My main concern is about breaking backwards compatibility. What is the consequences of that?
const Microsoft.TestPlatform.AdapterUtilities.HierarchyConstants.Levels.NamespaceIndex = 0 -> int | ||
const Microsoft.TestPlatform.AdapterUtilities.HierarchyConstants.Levels.TotalLevelCount = 2 -> int | ||
const Microsoft.TestPlatform.AdapterUtilities.HierarchyConstants.Levels.ContainerIndex = 0 -> int | ||
const Microsoft.TestPlatform.AdapterUtilities.HierarchyConstants.Levels.NamespaceIndex = 1 -> int |
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.
So how does this work when adapter utilities on testhost side and on IDE side are different? How do we keep compatibility with MSTest that is currently using this, like 2.2.9?
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.
We'll handle those on the test window side. No one except us uses this, so in older versions we just fallback to old approach for display purposes.
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.
Based on what will you handle it? The array size or the actual version of the mstest?
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.
Yes, we will only enable this on next release. (That part is not baked in TE yet.) Also if an adapter returns something not supported we will fallback to current behavior.
src/Microsoft.TestPlatform.AdapterUtilities/Resources/Resources.Designer.cs
Show resolved
Hide resolved
...Microsoft.TestPlatform.AdapterUtilities/ManagedNameUtilities/ManagedNameHelper.Reflection.cs
Show resolved
Hide resolved
d568fa6
to
2089b4c
Compare
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.
One last comment.
PS: It'd be better to have a separate PR for the assemblyinfo changes as they do not relate to this change.
...Microsoft.TestPlatform.AdapterUtilities/ManagedNameUtilities/ManagedNameHelper.Reflection.cs
Outdated
Show resolved
Hide resolved
2089b4c
to
749aac1
Compare
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.
Approving, because I am promised we can iterate on the design. And hopefully make the values separate properties that will be output only when they have non-default values. And same for the FQN properties, because keeping them custom (in property bag) adds a lot of overhead.
Our FQN API is finalized, this PR makes it compliant with Visual Studio.
Test group
,class name
,namespace
, andcontainer
.Test group
by default is managed method name without adapters.Hierarchies are for display purposes; we need to change how we generate those:
TestMethod<string>
instead ofTestMethod`1
.Test group
if any, do not append()
if not.Namespace
can bestring.Empty
in case of no namespace. Make sure that works as expected, and we're able to resolve when provided.Breaking changes
Hierarchy
since those versions were officially released and never used by anyone - support will start with the first version we release. It will be bound to TP version, and adapter version. Check is done inside Test Explorer.