microsoft / microsoft/nodejstools
node.js Test Container Discoverer is active in non JavaScript projects
Nobody has claimed this yet.
- Dominant language
- C#
- Stars
- 1.8k
- Forks
- 355
- PR merge metrics
- No merged PRs in 30d
Description
The NodeJS Test Container Discoverer is active in non JavaScript projects. In particular, it is active in C++ projects (.vcxproj).
I am talking about this class: Microsoft.NodejsTools.TestAdapter.TestContainerDiscoverer in this file.
When this class is queried, it iterates over all files and gets their last modified timestamp.
On large projects, this results in a non-neglectable delay. On our C++ code base, it takes between 30 seconds and 4 minutes.
Expected Behavior
NodeJSTools is not active in C++ projects and thus does not search for tests.
Actual Behavior
NodeJSTools tries to discover tests, producing disk IO for each file in the solution.
This disk IO takes more time on larger projects.
Up to 300 files, it is neglectable (<0.1 seconds).
For 1500 files, it is about half a second.
For 4500 files, it is about 2 seconds.
For 7500 files (attached solution), it is 3-5 seconds.
- NTVS Version: 1.5.10610.1 (Installed as part of the Node.js development workload)
- Visual Studio Version: 2019 (16.2.0)
- Node.js Version: -not relevant-
Steps to Reproduce
- Install Visual Studio 2019 with the Node,js development workload
- Download and extract LotOfFiles.zip
- Open Visual Studio
- Set the logging level for tests to 'Diagnostic' in Tools > Options > Test > General
- Open the solution (LotOfFiles/LotOfFiles.sln)
- Open the Test Explorer if you haven't already (default shortcut: Ctrl+E, T)
- Examine the test output in the 'Output' pane with the dropdown set to 'Tests'.
- Notice the text for the nodejs TestContainerDiscoverer is printed after a 3-5 seconds delay. (You might want to copy paste the output to a text editor and use search functionality.)
Solution
I've taken a look at the code and I believe this issue is rather simple to solve.
The following code is from TestContainerDiscoverer.cs lines 257-292:
public IEnumerable<ITestContainer> GetTestContainers(IVsProject project)
{
if (!project.TryGetProjectPath(out var path))
{
yield break;
}
// --- begin changes ---
// No need to search for tests in not supported projects.
if (!TypeScriptHelpers.IsSupportedTestProjectFile(path))
{
yield break;
}
// --- end changes ---
if (this.detectingChanges)
{
this.SaveModifiedFiles(project);
}
if (!this.knownProjects.TryGetValue(path, out var projectInfo) || !TryGetProjectUnitTestProperties(projectInfo.Project, out _, out _))
{
// Don't return any containers for projects we don't know about or that we know that they are not configured for JavaScript unit tests.
yield break;
}
projectInfo.HasRequestedContainers = true;
var latestWrite = project.GetProjectItemPaths().Aggregate(
this.lastWrite,
(latest, filePath) =>
{
try
{
var ft = File.GetLastWriteTimeUtc(filePath);
return (ft > latest) ? ft : latest;
}
catch (Exception exc) when (exc is UnauthorizedAccessException || exc is ArgumentException || exc is IOException)
{
}
return latest;
});
yield return new TestContainer(this, path, latestWrite);
}
I have not been able to test this proposed change as I cannot get the test container discoverer working when I build NodeJsTools locally.
It just doesn't run when the experimental instance Visual Studio instance is launched.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start in Nodejs/Product/TestAdapterImpl/TestContainerDiscoverer.cs, at GetTestContainers, and inspect the existing TypeScriptHelpers.IsSupportedTestProjectFile check proposed in the issue. Reproduce with LotOfFiles/LotOfFiles.sln and diagnostic Test Explorer output; done means C++ projects no longer trigger discovery or per-file timestamp checks while supported JavaScript projects still do.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- csharp, node.js
- Domain
- testing-qa, tooling
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Stale
- Clarity
- Clearly specified
- Newbie friendliness
- 38/100