Search CTest output for failure locations (#4418) #4420
Conversation
|
@malsyned I think that this is a useful PR and is an interesting additive feature. There is also minimal risk for regression, as by default this will not perform any additional computation because the default is without this. Before spending time reviewing, could you fix the merge conflicts? |
Fixed. I tightened up the corresponding schema in |
Adds configuration setting `cmake.ctest.failurePatterns`, a list of regular expressions and capture group indices for matching file, line, message, expected output, and actual output from failing tests. Pattern matches are used to populate `TestMessage` objects with more specific information than is available through the `DEF_SOURCE_LINE` property.
The string passed into `searchOutputForFailure()` has been normalized to CRLF, so make sure the substrings produced from it also have CRLF line endings. https://code.visualstudio.com/api/extension-guides/testing#test-output
The package.json schema for `cmake.ctest.failurePattens` was more lenient in some ways than the corresponding `FailurePatternsConfig` TypeScript type. Bring them in line with each other.
ce9c88f to
58a6a64
Compare
|
I have been doing some work to make sure that this feature is powerful enough to match GoogleTest and GoogleMock failure patterns. I think it is. I went in thinking it might be challenging to match the fairly complicated set of expected vs. actual patterns, but actually, GoogleTest and GoogleMock provide so much additional info in their messages that would be hidden if The regexp for GoogleTest/GoogleMock is So, that's increased my confidence in this with its current feature set. |
|
|
||
| const testMessage = new vscode.TestMessage(normalizeCRLF(message)); | ||
| testMessage.location = new vscode.Location( | ||
| vscode.Uri.file(file), new vscode.Position(line, 0) |
There was a problem hiding this comment.
@gcampbell-msft I have a small hesitation about this line, I would love for you to weigh in on it. It's assuming that the output file name matched by the regex is an absolute path suitable for passing to Uri.file(). It always has been in my testing, and I think due to the fact that CMake tends to generate build systems that operate exclusively on absolute paths, I'm guessing it's likely that it always will.
However, if you think otherwise, and you think there's a reasonable directory to resolve relative paths against, I would be happy to add something to this PR.
There was a problem hiding this comment.
I don't have any concerns, as the behavior when there are no patterns defined should be the same, correct?
With it being a new feature, I'm okay with the assmption that Uri.file() will work, again, as long as it still works the same if this feature isn't being used.
There was a problem hiding this comment.
Correct, with no patterns defined, we'll never get to this line of code.
My concern is that the string being passed to Uri.file() is extracted from ctest output that probably originates in some unit test framework code that uses __FILE__. That's how CppUTest and GoogleTest do it, for example. GCC-like compilers generally don't absolute-ize relative paths passed on the command line, so in general it's possible for the output of these test executables to contain relative paths, which aren't handled well by Uri.file().
However, this isn't the general case, this is in the context of CMake, which generally ensures compilers are passed absolute paths, and uses absolute paths in its output messages. That's why I think we can get away with passing the file path directly to Uri.file() without worrying about trying to make it an absolute path first, but also why I wanted a second opinion.
|
@paulmaybee @gcampbell-msft Is there anything else you all would like on this PR before you'd consider merging it? |
There was a problem hiding this comment.
I think this is a cool feature! It seems to present very little risk for regressions of other features and is purely additive, so I think this looks good, and it seems you have done good testing.
I left just a comment or two, but overall I think this looks great!
|
|
||
| const testMessage = new vscode.TestMessage(normalizeCRLF(message)); | ||
| testMessage.location = new vscode.Location( | ||
| vscode.Uri.file(file), new vscode.Position(line, 0) |
There was a problem hiding this comment.
I don't have any concerns, as the behavior when there are no patterns defined should be the same, correct?
With it being a new feature, I'm okay with the assmption that Uri.file() will work, again, as long as it still works the same if this feature isn't being used.
I'm glad you like it! Thanks for the review. I'll work on your comments today. |
| "type": "string" | ||
| } | ||
| ], | ||
| "default": [ |
There was a problem hiding this comment.
@gcampbell-msft in a few comments on this PR you've said some variation of
There is also minimal risk for regression, as by default this will not perform any additional computation because the default is without this
But this PR does add a couple of default regular expressions that will be tried against test output, so it would potentially change behavior in the absence of intentional user configuration. Should I empty out the default so that this feature is disabled by default?
|
|
||
| const testMessage = new vscode.TestMessage(normalizeCRLF(message)); | ||
| testMessage.location = new vscode.Location( | ||
| vscode.Uri.file(file), new vscode.Position(line, 0) |
There was a problem hiding this comment.
Correct, with no patterns defined, we'll never get to this line of code.
My concern is that the string being passed to Uri.file() is extracted from ctest output that probably originates in some unit test framework code that uses __FILE__. That's how CppUTest and GoogleTest do it, for example. GCC-like compilers generally don't absolute-ize relative paths passed on the command line, so in general it's possible for the output of these test executables to contain relative paths, which aren't handled well by Uri.file().
However, this isn't the general case, this is in the context of CMake, which generally ensures compilers are passed absolute paths, and uses absolute paths in its output messages. That's why I think we can get away with passing the file path directly to Uri.file() without worrying about trying to make it an absolute path first, but also why I wanted a second opinion.
This change addresses item #4418
This changes visible behavior
The following changes are proposed:
Adds configuration setting
cmake.ctest.failurePatterns, a list of regular expressions and capture group indices for matching file, line, message, expected output, and actual output from failing tests.The purpose of this change
Populate
TestMessageobjects with more specific information about test failures than is available through theDEF_SOURCE_LINEproperty.Other Notes/Information
I based the design of this feature around the design of Task Problem Matchers. It lacks some of the advanced features of Problem Matchers, like named patterns, pattern inheritance, multi-regexp patterns, and loops. I'm happy to add those to the implementation if needed, but I thought it better to start with the simplest thing that could work and grow it in response to feedback.
Screenshots