-
Notifications
You must be signed in to change notification settings - Fork 290
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
Add usage of shell when creating Settings #297
Changes from all commits
398527a
3233f8b
6aae593
e6b0a8b
47a3f2c
2065fc5
45d0f1c
c426c27
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 |
---|---|---|
|
@@ -22,6 +22,7 @@ import { DecorationOptions } from './types' | |
import { hasDocument, isOpenInMultipleEditors } from './editor' | ||
import { CoverageOverlay } from './Coverage/CoverageOverlay' | ||
import { JestProcess, JestProcessManager } from './JestProcessManagement' | ||
import { platform } from 'os' | ||
import { isWatchNotSupported, WatchMode } from './Jest' | ||
|
||
export class JestExt { | ||
|
@@ -68,7 +69,8 @@ export class JestExt { | |
this.failingAssertionDecorators = {} | ||
this.failDiagnostics = vscode.languages.createDiagnosticCollection('Jest') | ||
this.clearOnNextInput = true | ||
this.jestSettings = new Settings(workspace) | ||
const useShell = platform() === 'win32' | ||
this.jestSettings = new Settings(workspace, { shell: useShell }) | ||
this.pluginSettings = pluginSettings | ||
|
||
this.coverageMapProvider = new CoverageMapProvider() | ||
|
@@ -259,7 +261,8 @@ export class JestExt { | |
this.workspace.pathToJest = pathToJest(updatedSettings) | ||
this.workspace.pathToConfig = pathToConfig(updatedSettings) | ||
|
||
this.jestSettings = new Settings(this.workspace) | ||
const useShell = platform() === 'win32' | ||
this.jestSettings = new Settings(this.workspace, { shell: useShell }) | ||
|
||
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. Seeing the same thing twice makes me think we should move this into one spot: updateSettings(workspace) {
this.jestSettings = new Settings(workspace, { shell: platform() === 'win32' })
} 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 at first just thought of moving the settings out into its own property (a la |
||
this.coverageOverlay.enabled = updatedSettings.showCoverageOnLoad | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,7 +20,8 @@ export function registerSnapshotCodeLens(enableSnapshotPreviews: boolean) { | |
|
||
class SnapshotCodeLensProvider implements vscode.CodeLensProvider { | ||
public provideCodeLenses(document: vscode.TextDocument, _token: vscode.CancellationToken) { | ||
const snapshots = new Snapshot() | ||
// temporarily use `undefined` as parser argument, until Jest/#6212 is published | ||
const snapshots = new Snapshot(undefined) | ||
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. How do you feel about adding another line to the comment explaining when we can remove the argument? It'd be quick for someone who sees it to test it and see if the type updated type definition with the optional arg has been published with jestjs/jest#6212. 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. My plan was to remove it by myself as soon as there is a new release of Jest (I had it written to my calendar), but since jest-editor-supportv23 already includes that change, the |
||
return snapshots.getMetadata(document.uri.fsPath).map(snapshot => { | ||
const { line } = snapshot.node.loc.start | ||
const range = new vscode.Range(line - 1, 0, line - 1, 0) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
import { platform } from 'os' | ||
import { existsSync, readFileSync } from 'fs' | ||
import { normalize, join } from 'path' | ||
import * as vscode from 'vscode' | ||
|
||
import { IPluginSettings } from './IPluginSettings' | ||
|
||
|
@@ -62,22 +63,19 @@ function hasNodeExecutable(rootPath: string, executable: string): boolean { | |
*/ | ||
export function pathToJest(pluginSettings: IPluginSettings) { | ||
if (pluginSettings.pathToJest) { | ||
if (isBootstrappedWithCRA(pluginSettings.rootPath)) { | ||
return 'npm test --' | ||
} | ||
return normalize(pluginSettings.pathToJest) | ||
} | ||
|
||
const platform = process.platform | ||
if (platform === 'win32' && existsSync(join(pluginSettings.rootPath, 'node_modules', '.bin', 'jest.cmd'))) { | ||
return normalize(join(pluginSettings.rootPath, 'node_modules', '.bin', 'jest.cmd')) | ||
} else if ( | ||
(platform === 'linux' || platform === 'darwin') && | ||
existsSync(join(pluginSettings.rootPath, 'node_modules', '.bin', 'jest')) | ||
) { | ||
return normalize(join(pluginSettings.rootPath, 'node_modules', '.bin', 'jest')) | ||
if (isBootstrappedWithCRA(pluginSettings.rootPath)) { | ||
return 'npm test --' | ||
} | ||
|
||
const defaultPath = normalize('node_modules/.bin/jest') | ||
if (existsSync(join(pluginSettings.rootPath, defaultPath))) { | ||
return defaultPath | ||
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. This would remove the check for 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. This wouldn't fail since on Windows both files, 'node_modules/.bin/jest' and 'node_modules/.bin/jest.cmd' are present. But if we already check for the existence of the file, we probably really should keep caring about the extension. |
||
} | ||
|
||
vscode.window.showWarningMessage("Jest couldn't be found. Maybe you forgot to run `npm install`?") | ||
return 'jest' | ||
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. Neat idea. Should we worry about how noisy this could end up being for users who have installed Jest globally or are working with a package in a monorepo where 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. Probably, especially because users using a global Jest install at the moment also see the "This extension relies on Jest 20+ features…" error message. |
||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,21 +1,68 @@ | ||
jest.unmock('../src/helpers') | ||
jest.mock('fs') | ||
|
||
import { pathToJestPackageJSON, isCRATestCommand } from '../src/helpers' | ||
import { existsSync } from 'fs' | ||
jest.mock('vscode', () => ({ | ||
window: { | ||
showWarningMessage: () => {}, | ||
}, | ||
})) | ||
|
||
import { pathToJestPackageJSON, isCRATestCommand, pathToJest } from '../src/helpers' | ||
import { existsSync, readFileSync } from 'fs' | ||
import * as path from 'path' | ||
|
||
const existsMock = existsSync as jest.Mock<boolean> | ||
const defaultPathToJest = 'node_modules/.bin/jest' | ||
const readFileMock = readFileSync as jest.Mock<string> | ||
|
||
describe('ModuleHelpers', () => { | ||
describe('pathToJest', () => { | ||
it('should return the set value', () => { | ||
const workspace: any = { | ||
pathToJest: 'abcd', | ||
} | ||
expect(pathToJest(workspace)).toBe(workspace.pathToJest) | ||
}) | ||
|
||
it('should recognize CRA apps', () => { | ||
readFileMock.mockReturnValueOnce('{"scripts":{"test":"react-scripts test"}}') | ||
|
||
const workspace: any = { | ||
rootPath: '', | ||
pathToJest: '', | ||
} | ||
expect(pathToJest(workspace)).toBe('npm test --') | ||
}) | ||
|
||
it('should default to `node_modules/.bin/jest`', () => { | ||
readFileMock.mockReturnValueOnce('{}') | ||
existsMock.mockReturnValueOnce(true) | ||
|
||
const defaultPath = path.normalize('node_modules/.bin/jest') | ||
const workspace: any = { | ||
rootPath: '', | ||
pathToJest: '', | ||
} | ||
expect(pathToJest(workspace)).toBe(defaultPath) | ||
}) | ||
|
||
it('should fallback to `jest` if everything other fails', () => { | ||
readFileMock.mockReturnValueOnce('{}') | ||
existsMock.mockReturnValueOnce(false) | ||
|
||
const workspace: any = { | ||
rootPath: '', | ||
pathToJest: '', | ||
} | ||
expect(pathToJest(workspace)).toBe('jest') | ||
}) | ||
}) | ||
|
||
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. Nice. This makes me sad we duplicated this effort. 😭 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. At least we now can keep the best of both PRs 😉. Sorry that I didn't mention it already when discussing 324. |
||
describe('pathToJestPackageJSON', () => { | ||
it('should return null when not found', () => { | ||
existsMock.mockReturnValueOnce(false).mockReturnValueOnce(false) | ||
|
||
const workspace: any = { | ||
rootPath: '', | ||
pathToJest: defaultPathToJest, | ||
pathToJest: '', | ||
} | ||
expect(pathToJestPackageJSON(workspace)).toBe(null) | ||
}) | ||
|
@@ -28,7 +75,7 @@ describe('ModuleHelpers', () => { | |
|
||
const workspace: any = { | ||
rootPath: '', | ||
pathToJest: defaultPathToJest, | ||
pathToJest: '', | ||
} | ||
expect(pathToJestPackageJSON(workspace)).toBe(expected) | ||
}) | ||
|
@@ -39,7 +86,7 @@ describe('ModuleHelpers', () => { | |
|
||
const workspace: any = { | ||
rootPath: '', | ||
pathToJest: defaultPathToJest, | ||
pathToJest: '', | ||
} | ||
expect(pathToJestPackageJSON(workspace)).toBe(expected) | ||
}) | ||
|
@@ -52,7 +99,7 @@ describe('ModuleHelpers', () => { | |
|
||
const workspace: any = { | ||
rootPath: path.join('..', '..'), | ||
pathToJest: defaultPathToJest, | ||
pathToJest: '', | ||
} | ||
expect(pathToJestPackageJSON(workspace)).toBe(expected) | ||
}) | ||
|
@@ -63,7 +110,7 @@ describe('ModuleHelpers', () => { | |
|
||
const workspace: any = { | ||
rootPath: path.join('..', '..'), | ||
pathToJest: defaultPathToJest, | ||
pathToJest: '', | ||
} | ||
expect(pathToJestPackageJSON(workspace)).toBe(expected) | ||
}) | ||
|
This file was deleted.
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.
One of us is going to have a failing test if this changes. 😉