Clean up jest process management #215
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,7 @@ | ||
| import * as vscode from 'vscode' | ||
| import * as path from 'path' | ||
| import * as fs from 'fs' | ||
| import { Settings, ProjectWorkspace, JestTotalResults } from 'jest-editor-support' | ||
| import { matcher } from 'micromatch' | ||
|
|
||
| import * as decorations from './decorations' | ||
|
|
@@ -21,10 +21,10 @@ import { DebugCodeLensProvider } from './DebugCodeLens' | |
| import { DecorationOptions } from './types' | ||
| import { hasDocument, isOpenInMultipleEditors } from './editor' | ||
| import { CoverageOverlay } from './Coverage/CoverageOverlay' | ||
| import { JestProcess, JestProcessManager } from './JestProcessManagement' | ||
|
|
||
| export class JestExt { | ||
| private workspace: ProjectWorkspace | ||
| private jestSettings: Settings | ||
| private pluginSettings: IPluginSettings | ||
|
|
||
|
|
@@ -50,8 +50,10 @@ export class JestExt { | |
| // We have to keep track of our inline assert fails to remove later | ||
| failingAssertionDecorators: { [fileName: string]: vscode.TextEditorDecorationType[] } | ||
|
|
||
| private jestProcessManager: JestProcessManager | ||
| private jestProcess: JestProcess | ||
|
|
||
| private clearOnNextInput: boolean | ||
|
|
||
| constructor(workspace: ProjectWorkspace, outputChannel: vscode.OutputChannel, pluginSettings: IPluginSettings) { | ||
| this.workspace = workspace | ||
|
|
@@ -68,119 +70,111 @@ export class JestExt { | |
| this.testResultProvider = new TestResultProvider() | ||
| this.debugCodeLensProvider = new DebugCodeLensProvider(this.testResultProvider, pluginSettings.enableCodeLens) | ||
|
|
||
| this.jestProcessManager = new JestProcessManager({ | ||
| projectWorkspace: workspace, | ||
| runAllTestsFirstInWatchMode: this.pluginSettings.runAllTestsFirst, | ||
| }) | ||
|
|
||
| this.getSettings() | ||
| // The theme stuff | ||
| this.setupDecorators() | ||
| // The bottom bar thing | ||
| this.setupStatusBar() | ||
| //reset the jest diagnostics | ||
| resetDiagnostics(this.failDiagnostics) | ||
|
|
||
| // If we should start the process by default, do so | ||
| if (this.pluginSettings.autoEnable) { | ||
| this.startProcess() | ||
| } else { | ||
| this.channel.appendLine('Skipping initial Jest runner process start.') | ||
| } | ||
| } | ||
|
|
||
| private handleStdErr(error: Buffer) { | ||
| const message = error.toString() | ||
|
|
||
| if (this.shouldIgnoreOutput(message)) { | ||
| return | ||
| } | ||
|
|
||
| // The "tests are done" message comes through stdErr | ||
| // We want to use this as a marker that the console should | ||
| // be cleared, as the next input will be from a new test run. | ||
| if (this.clearOnNextInput) { | ||
| this.clearOnNextInput = false | ||
| this.parsingTestFile = false | ||
| this.channel.clear() | ||
| } | ||
| // thanks Qix, http://stackoverflow.com/questions/25245716/remove-all-ansi-colors-styles-from-strings | ||
| const noANSI = message.replace(/[\u001b\u009b][[()#;?]*(?:[0-9]{1,4}(?:;[0-9]{0,4})*)?[0-9A-ORZcf-nqry=><]/g, '') | ||
| if (noANSI.includes('snapshot test failed')) { | ||
| this.detectedSnapshotErrors() | ||
| } | ||
|
|
||
|
There was a problem hiding this comment. We're missing this behavior in the PR. When the Jest process in watch mode fails:
There was a problem hiding this comment. This is good point. The user should be notified. I will add this behaviour. There was a problem hiding this comment. Working on it - when the restart threshold has been reached the callback will be called so that we can update the channel and the status bar appropriately. There was a problem hiding this comment. Ok, so I suggest the following. For processes in watch mode I call exit callback only if they failed for a number of times ( There was a problem hiding this comment. Hmm...there is still one issue with this. When stopping Jest explicitly, we will get a misleading message in the channel. I suppose I will need to add an argument to the stop handler to mark that it is the result of explicit user action. There was a problem hiding this comment. Ok, maybe there is a better way to accomplish that without adding any extra argument to the handlers. Let me check. There was a problem hiding this comment. One more small thing. Just to see how it feels, on a separate branch I created a version where There was a problem hiding this comment. @seanpoulter I am running it now for two days using the version where There was a problem hiding this comment. Sounds good. I'll wrap up my own dev efforts then focus on the review. I'm pretty short on spare time these days, so sorry in advance for it taking a while. If anyone else has the bandwidth, feel free to take the lead. |
||
| this.channel.appendLine(noANSI) | ||
| } | ||
|
|
||
| private assignHandlers(jestProcess) { | ||
| jestProcess | ||
| .onJestEditorSupportEvent('executableJSON', (data: JestTotalResults) => { | ||
| this.updateWithData(data) | ||
| }) | ||
| .onJestEditorSupportEvent('executableOutput', (output: string) => { | ||
| if (!this.shouldIgnoreOutput(output)) { | ||
| this.channel.appendLine(output) | ||
| } | ||
| }) | ||
| .onJestEditorSupportEvent('executableStdErr', (error: Buffer) => this.handleStdErr(error)) | ||
| .onJestEditorSupportEvent('nonTerminalError', (error: string) => { | ||
| this.channel.appendLine(`Received an error from Jest Runner: ${error.toString()}`) | ||
| }) | ||
| .onJestEditorSupportEvent('exception', result => { | ||
| this.channel.appendLine(`\nException raised: [${result.type}]: ${result.message}\n`) | ||
| }) | ||
| .onJestEditorSupportEvent('terminalError', (error: string) => { | ||
| this.channel.appendLine('\nException raised: ' + error) | ||
| }) | ||
| } | ||
|
|
||
| public startProcess() { | ||
| if (this.jestProcessManager.numberOfProcesses > 0) { | ||
| return | ||
| } | ||
|
|
||
| if (this.pluginSettings.runAllTestsFirst) { | ||
| this.testsHaveStartedRunning() | ||
| } | ||
|
|
||
| this.jestProcess = this.jestProcessManager.startJestProcess({ | ||
| watch: true, | ||
| keepAlive: true, | ||
| exitCallback: (jestProcess, jestProcessInWatchMode) => { | ||
| if (jestProcessInWatchMode) { | ||
| this.jestProcess = jestProcessInWatchMode | ||
|
|
||
| this.channel.appendLine('Finished running all tests. Starting watch mode.') | ||
| status.running('Starting watch mode') | ||
|
|
||
| this.assignHandlers(this.jestProcess) | ||
| } else { | ||
| status.stopped() | ||
| if (!jestProcess.stopRequested) { | ||
| this.channel.appendLine( | ||
| 'Starting Jest in Watch mode failed too many times and has been stopped. Please check your system configuration.' | ||
| ) | ||
| } | ||
| } | ||
| }, | ||
| }) | ||
|
|
||
| this.assignHandlers(this.jestProcess) | ||
| } | ||
|
|
||
| public stopProcess() { | ||
| this.channel.appendLine('Closing Jest') | ||
| this.jestProcessManager.stopAll() | ||
| status.stopped() | ||
| } | ||
|
|
||
| private getSettings() { | ||
|
|
@@ -191,13 +185,6 @@ export class JestExt { | |
| ) | ||
| } | ||
| this.workspace.localJestMajorVersion = jestVersionMajor | ||
| }) | ||
|
|
||
| // Do nothing for the minute, the above ^ can come back once | ||
|
|
@@ -219,7 +206,14 @@ export class JestExt { | |
| // No response == cancel | ||
| if (response) { | ||
| this.jestProcess.runJestWithUpdateForSnapshots(() => { | ||
| if (this.pluginSettings.restartJestOnSnapshotUpdate) { | ||
| this.jestProcessManager.stopJestProcess(this.jestProcess).then(() => { | ||
| this.startProcess() | ||
| }) | ||
| vscode.window.showInformationMessage('Updated Snapshots and restarted Jest.') | ||
| } else { | ||
| vscode.window.showInformationMessage('Updated Snapshots. It will show in your next test run.') | ||
| } | ||
| }) | ||
| } | ||
| }) | ||
|
|
@@ -345,11 +339,7 @@ export class JestExt { | |
| } | ||
|
|
||
| private setupStatusBar() { | ||
| status.initial() | ||
| } | ||
|
|
||
| private setupDecorators() { | ||
|
|
@@ -360,13 +350,13 @@ export class JestExt { | |
| } | ||
|
|
||
| private shouldIgnoreOutput(text: string): boolean { | ||
| // this fails when snapshots change - to be revised - returning always false for now | ||
| return text.includes('Watch Usage') | ||
| } | ||
|
|
||
| private testsHaveStartedRunning() { | ||
| this.channel.clear() | ||
| status.running('initial full test run') | ||
| } | ||
|
|
||
| private updateWithData(data: JestTotalResults) { | ||
|
|
@@ -418,7 +408,7 @@ export class JestExt { | |
| } | ||
|
|
||
| public deactivate() { | ||
| this.jestProcessManager.stopAll() | ||
| } | ||
|
|
||
| private getJestVersion(version: (v: number) => void) { | ||
|
|
@@ -514,8 +504,8 @@ export class JestExt { | |
| } | ||
|
|
||
| public runTest = (fileName: string, identifier: string) => { | ||
| const restart = this.jestProcessManager.numberOfProcesses > 0 | ||
| this.jestProcessManager.stopAll() | ||
| const program = this.resolvePathToJestBin() | ||
| if (!program) { | ||
| console.log("Could not find Jest's CLI path") | ||
|
|
||
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.
Should this be the default behavior @marcinczenko? Based on your description in #179 it seems like we'll always want to restart Jest and don't need the option.
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.
I wanted this change to be not invasive so that current users do not get something they do not expect. I am open to making it a default if there is agreement.