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

VsTest invokes the wrong ITestExecutor method when the user chooses to Run All #2474

Closed
plioi opened this issue Jul 3, 2020 · 4 comments
Closed
Labels
by-design needs-triage This item should be discussed in the next triage meeting.

Comments

@plioi
Copy link
Contributor

plioi commented Jul 3, 2020

Description

Test adapters implement ITestExecutor, which has 2 overloads for the RunTests entry point method.

void RunTests(IEnumerable<string> sources, IRunContext runContext, IFrameworkHandle frameworkHandle)
void RunTests(IEnumerable<TestCase> tests, IRunContext runContext, IFrameworkHandle frameworkHandle)

The first is intended for "Run All", while the second is intended for when the user has selected a few specific tests to run.

At some point, VsTest stopped calling the first method for Run All, and treats all runs as if the user was selecting few tests to run. In the case of the user choosing to Run All, this amounts to the tests parameter containing an item for every single test in the assembly. This means that each test framework has to cross reference that list against their own internal reflection work in order to run tests.

This is at best a memory leak, and at worst a performance hit within each test framework, as a test project grows in size.

This also had the side effect of breaking NUnit: nunit/nunit3-vs-adapter#658

Environment

Visual Studio Community 2019 v 16.5.4

@nohwnd
Copy link
Member

nohwnd commented Jul 3, 2020

@drognanar is this related to the jsonrpc overload problem you were solving recently?

@drognanar
Copy link
Member

this is related to the change how we trigger tests for run all

tagging @jfleisher as he's been looking into nunit/nunit3-vs-adapter#658

@Evangelink Evangelink added the needs-triage This item should be discussed in the next triage meeting. label Jul 26, 2022
@nohwnd
Copy link
Member

nohwnd commented Jul 27, 2022

This is by design now, we are working on making discovery faster and faster, and moving vstest.console in-process, so we would end up with just one serialization. TE sends all testcases so then can run only tests that were actually discovered, and not all. But maybe there is more nuance to it.

@nohwnd nohwnd closed this as completed Jul 27, 2022
@plioi
Copy link
Contributor Author

plioi commented Jul 27, 2022

That doesn't make sense. A test framework still has to do its own concept of discovery/vetting against the giant unbounded list, to only run things actually discoverable. Waiting for TE to do all that irrelevant prework when a simple "run all" signal would have sufficed is only slowing things down at every step.

plioi added a commit to fixie/fixie that referenced this issue Oct 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
by-design needs-triage This item should be discussed in the next triage meeting.
Projects
None yet
Development

No branches or pull requests

4 participants