-
Notifications
You must be signed in to change notification settings - Fork 485
Search CTest output for failure locations (#4418) #4420
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
base: main
Are you sure you want to change the base?
Changes from all commits
aaae1dd
d0ba74a
349f853
06cc472
7e4bd6a
8f01d29
bbb0604
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,7 @@ import { ProjectController } from '@cmt/projectController'; | |
import { extensionManager } from '@cmt/extension'; | ||
import { CMakeProject } from '@cmt/cmakeProject'; | ||
import { handleCoverageInfoFiles } from '@cmt/coverage'; | ||
import { FailurePattern, FailurePatternsConfig } from '@cmt/config'; | ||
|
||
nls.config({ messageFormat: nls.MessageFormat.bundle, bundleFormat: nls.BundleFormat.standalone })(); | ||
const localize: nls.LocalizeFunc = nls.loadMessageBundle(); | ||
|
@@ -132,6 +133,53 @@ interface ProjectCoverageConfig { | |
coverageInfoFiles: string[]; | ||
} | ||
|
||
export function searchOutputForFailures(patterns: FailurePatternsConfig, output: string): vscode.TestMessage[] { | ||
output = util.normalizeLF(output); | ||
const messages = []; | ||
patterns = Array.isArray(patterns) ? patterns : [patterns]; | ||
for (let pattern of patterns) { | ||
pattern = typeof pattern === 'string' ? { regexp: pattern } : pattern; | ||
pattern.file ??= 1; | ||
pattern.line ??= 2; | ||
pattern.message ??= 3; | ||
|
||
try { | ||
for (const match of output.matchAll(RegExp(pattern.regexp, "g"))) { | ||
if (pattern.file && match[pattern.file]) { | ||
messages.push(matchToTestMessage(pattern, match)); | ||
} | ||
} | ||
} catch (e) { | ||
console.error(e); | ||
} | ||
} | ||
return messages; | ||
} | ||
|
||
function matchToTestMessage(pat: FailurePattern, match: RegExpMatchArray): vscode.TestMessage { | ||
const file = match[pat.file as number]; | ||
const line = pat.line ? parseLineMatch(match[pat.line]) : 0; | ||
const message = pat.message && match[pat.message]?.trim() || 'Test Failed'; | ||
const actual = pat.actual ? match[pat.actual] : undefined; | ||
const expected = pat.expected ? match[pat.expected] : undefined; | ||
|
||
const testMessage = new vscode.TestMessage(util.normalizeCRLF(message)); | ||
testMessage.location = new vscode.Location( | ||
vscode.Uri.file(file), new vscode.Position(line, 0) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correct, with no patterns defined, we'll never get to this line of code. My concern is that the string being passed to 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 |
||
); | ||
testMessage.expectedOutput = expected; | ||
testMessage.actualOutput = actual; | ||
return testMessage; | ||
} | ||
|
||
function parseLineMatch(line: string | null) { | ||
const i = parseInt(line || ''); | ||
if (i) { | ||
return i - 1; | ||
} | ||
return 0; | ||
} | ||
|
||
function parseXmlString<T>(xml: string): Promise<T> { | ||
return new Promise((resolve, reject) => { | ||
xml2js.parseString(xml, (err, result) => { | ||
|
@@ -410,7 +458,13 @@ export class CTestDriver implements vscode.Disposable { | |
} else { | ||
log.info(message.message); | ||
} | ||
run.failed(test, message, duration); | ||
const outputMessages = searchOutputForFailures( | ||
this.ws.config.ctestFailurePatterns as FailurePatternsConfig, | ||
// string cast OK; never passed TestMessage with MarkdownString message | ||
message.message as string | ||
); | ||
const messages = outputMessages.length ? outputMessages : message; | ||
run.failed(test, messages, duration); | ||
} | ||
|
||
/** | ||
|
@@ -599,7 +653,7 @@ export class CTestDriver implements vscode.Disposable { | |
|
||
let output = testResult.output; | ||
// https://code.visualstudio.com/api/extension-guides/testing#test-output | ||
output = output.replace(/\r?\n/g, '\r\n'); | ||
output = util.normalizeCRLF(output); | ||
if (test.uri && test.range) { | ||
run.appendOutput(output, new vscode.Location(test.uri, test.range.end), test); | ||
} else { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gcampbell-msft in a few comments on this PR you've said some variation of
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?