-
Notifications
You must be signed in to change notification settings - Fork 294
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
improve failure reporting for the whole project #146
Conversation
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.
👍 looks great to me, I won't merge until the next jest patch release though (when someone installs the extension it will get deps from NPM, so the dep needs to exist in npm (not just on my computer for deploys)
/cc @cpojer
status.success() | ||
} else { | ||
status.failed() | ||
status.failed(` (${failedFileCount} test suite${failedFileCount > 1 ? 's' : ''} failed)`) |
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.
nice move 👍
import { TestFileAssertionStatus } from 'jest-editor-support' | ||
import { TestReconciliationState } from './TestReconciliationState' | ||
|
||
export function updateDiagnositics(testResults: TestFileAssertionStatus[], diagnostics: vscode.DiagnosticCollection) { |
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.
updateDiagnositics
, typo: updateDiagnostics
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.
Oops, indeed... Looks like you already merged it, I will submit a new PR then
export function failedSuiteCount(diagnostics: vscode.DiagnosticCollection): number { | ||
let sum = 0 | ||
diagnostics.forEach(() => sum++) | ||
return sum |
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.
interesting, I didn't know reduce
was not a part of the JS core library
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.
Array.prototype.reduce() is indeed part of JS core, unfortunately diagnostics is a vscode.DiagnosticCollection
, which only exposes forEach() iterator... I know it looked stupid but we have no choice here...
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.
👍
setTimeout(() => initial(), 2000) | ||
} | ||
|
||
function updateStatus(message: string) { | ||
function updateStatus(message: string, details?: string) { |
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.
nice move
Published 21.3.0-alpha.eff7a1cf for you guys. |
ohhh yeahhh, thanks! |
improve failure reporting for the whole project
This PR addressed the issues discussed in #129, mainly to make managing tests easier (especially for monorepo projects) by providing the test state for the "whole-project" in addition to the "changed", plus a few usability/stability enhancement.
change outline
Note
Follow up