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

Test methods with generic parameters don't show source locations in Microsoft.Testing.Platform mode in Test Explorer #3863

Closed
bradwilson opened this issue Sep 25, 2024 · 9 comments

Comments

@bradwilson
Copy link

bradwilson commented Sep 25, 2024

When xUnit.net v3 test projects are in Microsoft Testing Platform mode, source information is missing when the test method parameter is generic.

Recording.2024-09-24.210427.mp4

Reproduced with:

  • VS 2022 17.12 Preview 2
  • NuGet package xunit.v3 version 0.4.0-pre.20
@bradwilson bradwilson changed the title Generic test methods don't show source locations in Microsoft.Testing.Platform mode in Test Explorer Test methods with generic parameters don't show source locations in Microsoft.Testing.Platform mode in Test Explorer Sep 25, 2024
@bradwilson
Copy link
Author

bradwilson commented Sep 28, 2024

On a hunch, I brought back my implementation of https://github.com/microsoft/vstest/blob/main/docs/RFCs/0017-Managed-TestCase-Properties.md and now everything is working as expected.

I don't remember what issue it was in, but someone from the VSTest team told me this was outdated and no longer needed. That information appeared to be incorrect.

So...what's the story here? Is this working right now because Test Explorer hasn't been updated yet? Or is this working right now because the advice to just use .FullName was incorrect?

/cc @MarcoRossignoli

Here's 2022 17.11 in VSTest mode:

image

Here's 2022 17.12 Preview 2 in MTP mode:

image

@bradwilson
Copy link
Author

I set up a branch for the changes: https://github.com/xunit/xunit/tree/testfx-3863

@bradwilson
Copy link
Author

I don't remember what issue it was in, but someone from the VSTest team told me this was outdated and no longer needed.

Found it: #3478 (comment)

@bradwilson
Copy link
Author

Also, if this is the answer:

Is this working right now because Test Explorer hasn't been updated yet?

Then what is the migration path forward? Will Test Explorer support both styles into the future so as to preserve backward compatibility with test frameworks that are emitting the "VSTest" style vs. the "FullName" style? If so, for how long? We obviously don't want to unnecessarily degrade users who are stuck on older builds of Visual Studio that might still be supported.

@MarcoRossignoli
Copy link
Contributor

MarcoRossignoli commented Sep 30, 2024

So...what's the story here? Is this working right now because Test Explorer hasn't been updated yet? Or is this working right now because the advice to just use .FullName was incorrect?

I suppose that use the full name in this case was not enough, so for now I think you can keep the fix you did, the second image attached is what I would expect.

Then what is the migration path forward? Will Test Explorer support both styles into the future so as to preserve backward compatibility with test frameworks that are emitting the "VSTest" style vs. the "FullName" style? If so, for how long? We obviously don't want to unnecessarily degrade users who are stuck on older builds of Visual Studio that might still be supported.

Test explorer will support both styles I expect forever. VSTest is supported and will be for long time. So for adapters that won't support the VSTestBridge we use and evolve the TestMethodIdentifierProperty that's used in the new protocol workflow for VS too.


About TestMethodIdentifierProperty I had already a discussion ongoing to make it more useful and complete. We were thinking to have an object descriptor(or 2 different object props) that can be used in 2 way:

  • descriptive information that VS can use to "show" to user the test method, something done for pure UX reason.
  • a round trip object that let to the adapter to "precisely" point to a method info to allow quick discovery and invocation, tailored made on the current reflection api without the need of parsing and string manipulation.

cc: @drognanar @Evangelink

@nohwnd
Copy link
Member

nohwnd commented Sep 30, 2024

I don't remember what issue it was in, but someone from the VSTest team told me this was outdated and no longer needed.

Found it: #3478 (comment)

I am not sure if the is still any confustion, but: That specification is still valid when you are implementing a VSTest based test adapter. When Marco said this is the old VSTest, he meant that it is not relevant to your TestingPlatform implementation, not that it is not relevant to VSTest.

@bradwilson
Copy link
Author

bradwilson commented Sep 30, 2024

When Marco said this is the old VSTest, he meant that it is not relevant to your TestingPlatform implementation, not that it is not relevant to VSTest.

Except it's affecting both.

For VSTest (via xunit.runner.visualstudio), that means setting TestCase.ManagedMethod and TestCase.ManagedType. For MTP, that means setting TestMethodIdentifierProperty. In both cases, I must use the "old VSTest" names or else Test Explorer is broken.

  • descriptive information that VS can use to "show" to user the test method, something done for pure UX reason.
  • a round trip object that let to the adapter to "precisely" point to a method info to allow quick discovery and invocation, tailored made on the current reflection api without the need of parsing and string manipulation.

I'm definitely not going to have both of these sitting on our object model. Having both would be both space-wasteful (they're serialized) and confusing for end users, assuming anybody ever uses this besides xunit.runner.visualstudio (which is pretty much guaranteed to be the only consumer, given that the format of it is hand-crafted solely to make Test Explorer work). I'm not going to change what it means and how it's rendered later, because it's the only thing that works today and into the future, unless Test Explorer decides to stop supporting it (or you break me by changing the contract of TestMethodIdentifierProperty in the future).

It's hard to foresee a situation in which I'll ever provide anything other than the "old VSTest" names, given where we are today.

(To put in clearer terms: I think you've already defined what TestMethodIdentifierProperty means. You can't change it now without breaking me, and maybe others. If you want something different, add something new. Don't break us.)

@bradwilson
Copy link
Author

Solidified that this is about VSTest: xunit/xunit@b4686ac

@bradwilson bradwilson closed this as not planned Won't fix, can't repro, duplicate, stale Sep 30, 2024
@MarcoRossignoli
Copy link
Contributor

MarcoRossignoli commented Sep 30, 2024

For VSTest (via xunit.runner.visualstudio), that means setting TestCase.ManagedMethod and TestCase.ManagedType. For MTP, that means setting TestMethodIdentifierProperty. In both cases, I must use the "old VSTest" names or else Test Explorer is broken.

Ok I think you can use the "old VSTest" names, I don't see changes on parsing logic on TE side in near future.

cc: @drognanar as owner of TE side.

bradwilson added a commit to xunit/visualstudio.xunit that referenced this issue Oct 2, 2024
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

No branches or pull requests

3 participants