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

Cannot enable MSTest runner globally and disable it only on a subset of projects #2631

Closed
Evangelink opened this issue Mar 25, 2024 · 9 comments
Assignees
Labels

Comments

@Evangelink
Copy link
Member

Describe the bug

Cannot enable MSTest runner globally and disable it only on a subset of projects.

Steps To Reproduce

Use following repro
repro.zip

@nohwnd
Copy link
Member

nohwnd commented Apr 2, 2024

I get you maybe are filing this just for yourself, but what is the observed problem?

@Evangelink
Copy link
Member Author

We have had some requests from users who would like to enable the runner on all their test projects by default and selectively disable it on some projects. The idea being to set EnableMSTestRunner to false on the projects where to disable the new mode.

As shown in the repro zip file, this is not working. After some investigation, this is linked to how we register and define all the properties and targets (dependencies and order of resolution). I have done many variations and there is no good solution while maintaining an opt-in mechanism in MSTest.

The only alternatives I have found are:

  • using MSTest.Sdk, best solution for projects support sdk style
  • documenting a set of properties people have to set on each project where they want to disable the runner
  • providing an extra target inside of MSTest that people would import to disable it (this is hiding the complexity described in the previous point).
  • providing a different set of NuGet packages for MSTest.TestAdapter for VSTest and runner. I think this is too late for this option.

@vikukush
Copy link

vikukush commented Apr 5, 2024

As shown in the repro zip file, this is not working. After some investigation, this is linked to how we register and define all the properties and targets (dependencies and order of resolution). I have done many variations and there is no good solution while maintaining an opt-in mechanism in MSTest.

Conceptually, .props files imported before the project should not do any property calculation. I don't think going SDK will save you, since props are still going to be imported before
I would prefer you would go with 3) target running before your first custom target fixing up the properties. If you have a target like this, you should be able to move all necessary property calculation from .props to .targets file while keeping all current projects buildable via target logic.

@Evangelink
Copy link
Member Author

The problem is that msbuild package target is imported before MSTest one so all processing will already be done.

It was all conceived as alternative package and opt-in solution. For some simplicity we decided to squash it all under the adapter but that's causing an issue in the case of a global opt-in and specific opt-out.

@vikukush
Copy link

vikukush commented Apr 8, 2024

If you put the processing into your custom target that'd run before the build, you then sidestep all import order problems

@Evangelink
Copy link
Member Author

Yes that means we have to copy paste all the logic of the msbuild project and always make sure to keep it in sync.

@vikukush
Copy link

vikukush commented Apr 8, 2024

As far as I understand, you shouldn't even need the target, you just need to override two properties from msbuild project and it disables everything else from that .targets file. Check out my PR above

@vikukush
Copy link

vikukush commented Apr 8, 2024

Ok I see that there are Items the MSBuild package is defining, so although this will work, for static parsing that VS does this is probably not going to work.
I see that ProjectCapability items are something we can't cleanly override, however they apparently symbolize that the project "may contain unit tests", which is going to still be the case with mstest runner.
And remaining AssemblyMetadata item can be wiped before the build, since it should have no meaning in static evaluation

@Evangelink
Copy link
Member Author

I have been looking at the problem from various angles and there is nothing we can do without breaking change so I will move the ticket as won't fix.

@Evangelink Evangelink closed this as not planned Won't fix, can't repro, duplicate, stale Jun 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants