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

Fix the problem that the test projects are not being enriched by VsTest. #2751

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MischaVreeburg
Copy link
Contributor

For the enrichment, an alternative datasource has been chosen along with some clever parsing. It now falls back for C# files to the previously scanned test source files and tries to map the test full name with the name in the file and uses that info to get the test file path, along with the method declaration.

This PR fixes issue: #2750

For the enrichment, an alternative datasource has been chosen along with some clever parsing.
It now falls back for C# files to the previously scanned test source files and tries to map the test full name with the name in the file and uses that info to get the test file path, along with the method declaration.
@dupdob
Copy link
Member

dupdob commented Nov 4, 2023

Thanks for this PR. This is not a review but a bunch of general comments:

  1. you need to add unit tests for this. I suggest isolating the source code searching/parsing algorithm so writing test is easier
  2. note that this approach provides only partial support: unit tests can be renamed via framework specific attributes and theory-like tests may include data as part of their name.

Overall, I think this is a good move, but I would like feedback from the core team regarding if this is an approach they support (i.e. this simply improving and supporting it until a fix from VsTest teams is made available)

@rouke-broersma
Copy link
Member

@dupdob sounds good to me, I have no objection and I agree with your review feedback as well.

Copy link
Member

@dupdob dupdob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test detection logic should be moved to a specific class, or methods.
You need to add unit tests for this

Comment on lines +72 to 115
//Test if you can find the file by scanning the sources for testcase name
var qualifiedNameArray = unitTest.FullyQualifiedName.Split('.');
var methodName = qualifiedNameArray[^1];
var className = qualifiedNameArray[^2];
var nameSpace1 = new ArraySegment<string>(qualifiedNameArray, 0, qualifiedNameArray.Length - 2);
var nameSpace = $"namespace {string.Join('.', nameSpace1)}";

testFile = testProjectsInfo.TestFiles.Where(tFile => !tFile.FilePath.EndsWith("GlobalSuppressions.cs")).SingleOrDefault(tFile =>
tFile.Source.Contains(className) && tFile.Source.Contains(methodName) && tFile.Source.Contains(nameSpace));
if (testFile is not null)
{
var testDescriptions =
initialTestRun.Result.VsTestDescriptions.Where(td => td.Description.Name == unitTest.FullyQualifiedName);
foreach (var testDescription in testDescriptions)
{
testDescription.Description.TestFilePath = testFile.FilePath;
}

var lineNumber = unitTest.LineNumber;
if (lineNumber < 1)
{
var lines = testFile.Source.Split("\r\n");
foreach (var line in lines)
{
if (line.Contains(methodName) && !line.Contains("class"))
{
lineNumber = Array.IndexOf(testFile.Source.Split("\r\n"),
testFile.Source.Split("\r\n").First(sourceLine => sourceLine.Contains(methodName) && !sourceLine.Contains("class")));
break;
}
}
}

var lineSpan = testFile.SyntaxTree.GetText().Lines[lineNumber].Span;
var nodesInSpan = testFile.SyntaxTree.GetRoot().DescendantNodes(lineSpan);
var node = nodesInSpan.First(n => n is MethodDeclarationSyntax);
testFile.AddTest(unitTest.Id, unitTest.FullyQualifiedName, node);
}
else
{
_logger.LogDebug(
"Could not locate unit test in any testfile. This should not happen and results in incorrect test reporting.");
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be please move this logic to a dedicated class ? at least dedicated methods?

This code needs unit tests.

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

Successfully merging this pull request may close these issues.

3 participants