Add usage of shell when creating Settings by stephtr · Pull Request #297 · jest-community/vscode-jest

stephtr · 2018-04-05T09:27:50Z

This change will finally close #274. We still have to wait until jest/#5658 and jest/#6212 get released to npm, but in the meantime I created this PR so that we don't forget about it.
#trivial

coveralls · 2018-04-05T09:30:56Z

Pull Request Test Coverage Report for Build 389

9 of 9 (100.0%) changed or added relevant lines in 2 files are covered.
2 unchanged lines in 1 file lost coverage.
Overall coverage increased (+3.3%) to 78.952%

Files with Coverage Reduction	New Missed Lines	%
src/JestExt.ts	2	53.47%

Totals
Change from base Build 383:	3.3%
Covered Lines:	661
Relevant Lines:	811

💛 - Coveralls

stephtr · 2018-05-21T07:21:04Z

I updated jest-editor-support to the last beta (23.0.0-charlie.2), since there haven't been made any large changes except our fixes.

seanpoulter · 2018-05-22T04:22:34Z

I hate to be asking it at this point in your PR but can you explain why we need another shell option in jest-editor-support? Is it explained in the issues you linked? Having that in two places seems like a code smell that we should clean up in jest-editor-support.

stephtr · 2018-05-22T06:33:29Z

No problem. You actually introduced the second occurence with Jest#5658 😉. The issue is that if we would like to omit the .cmd extension, we have to run the command in a shell every time. jest-editor-support creates a Jest process in Runner#L83, Runner#L136 and Settings#L66. With Jest#5340 and vscode-jest#248 we fixed the first two occurrences, but forgot about the third one (which resulted in #274).

Since Runner and Settings are independent from each other, a cleaner solution than supplying shell two times would be hard. The only option I could think of would be to move shell: platform() === 'win32' directly into Process#L48 such that we don't have to care about it in vs-code anymore.

seanpoulter · 2018-05-23T03:05:37Z

Sweet, thanks for those links and details. Since they're all using this._createProcess from the Process we could wrap that to set our common shell option on our end of Process, but those still all end up in the constructor options and aren't any cleaner. 🤔

seanpoulter

Two blocking comments:

We're rolling back @ThomasRooney's Windows support. I can see how that might not be a problem if we get this in and v23 doesn't explode.
The warning message might be a bit much when Jest is globally installed or running from a monorepo

seanpoulter · 2018-05-23T03:15:28Z

  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)


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.

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 undefined parameter won't be necessary anymore.

seanpoulter · 2018-05-23T03:16:11Z

+            "The path to the Jest binary, or an npm command to run tests prefixed with `--` e.g. `node_modules/.bin/jest` or `npm test --`",
          "type": "string",
-          "default": null
+          "default": ""


One of us is going to have a failing test if this changes. 😉

seanpoulter · 2018-05-23T03:19:06Z

-    this.jestSettings = new Settings(this.workspace)
+    const useShell = platform() === 'win32'
+    this.jestSettings = new Settings(this.workspace, { shell: useShell })



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' }) }

I at first just thought of moving the settings out into its own property (a la this.settingsOptions = { shell: platform() === 'win32' }, but your approach is tidier than that.

seanpoulter · 2018-05-23T03:21:06Z

+
+  const defaultPath = normalize('node_modules/.bin/jest')
+  if (existsSync(join(pluginSettings.rootPath, defaultPath))) {
+    return defaultPath


This would remove the check for jest.cmd and fail on Windows systems.

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.

seanpoulter · 2018-05-23T03:26:13Z

  }

+  vscode.window.showWarningMessage("Jest couldn't be found. Maybe you forgot to run `npm install`?")
  return 'jest'


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 node_modules isn't in the workspace root (like Jest)? I think we can build on this and lead users to helpful documentation about how to configure the extension. 👍

Probably, especially because users using a global Jest install at the moment also see the "This extension relies on Jest 20+ features…" error message.
It was just that opening VS Code before npm install finished happened to me more often than thinking about global Jest installs.

seanpoulter · 2018-05-23T03:27:53Z

+      expect(pathToJest(workspace)).toBe('jest')
+    })
+  })
+


Nice. This makes me sad we duplicated this effort. 😭

At least we now can keep the best of both PRs 😉. Sorry that I didn't mention it already when discussing 324.

stephtr · 2018-05-26T15:52:33Z

I will wait for #332 being merged and base my PR on that one (once it is merged).

stephtr · 2018-07-01T15:21:48Z

Closing in favor of #340.

add usage of shell when creating Settings

398527a

stephtr added the state: in progress label Apr 5, 2018

stephtr mentioned this pull request May 17, 2018

Fix decorators on windows by adjusting implicit jest path based on OS #324

Merged

stephtr added 2 commits May 20, 2018 21:10

use original typescript definition of jest-editor-support

3233f8b

update packages

6aae593

stephtr removed the state: in progress label May 21, 2018

stephtr added 5 commits May 21, 2018 09:53

Merge branch 'master-recent' into fix-spawn

e6b0a8b

add comment

47a3f2c

revert/update pathToJest

2065fc5

update tests

45d0f1c

show warning message if pathToJest is falling back

c426c27

stephtr mentioned this pull request May 21, 2018

Start Jest with the --no-color CLI argument #293

Merged

stephtr added the stage: needs review label May 21, 2018

stephtr mentioned this pull request May 22, 2018

Fix regression detecting create-react-app #332

Merged

seanpoulter requested changes May 23, 2018

View reviewed changes

seanpoulter added stage: in progress and removed stage: needs review labels May 23, 2018

stephtr removed the stage: in progress label Jul 1, 2018

stephtr mentioned this pull request Jul 1, 2018

Add usage of shell when gathering Jest settings #340

Merged

stephtr closed this Jul 1, 2018

stephtr deleted the fix-spawn branch July 1, 2018 15:44

Conversation

stephtr commented Apr 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Apr 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 389

💛 - Coveralls

Uh oh!

stephtr commented May 21, 2018

Uh oh!

seanpoulter commented May 22, 2018

Uh oh!

stephtr commented May 22, 2018

Uh oh!

seanpoulter commented May 23, 2018

Uh oh!

seanpoulter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephtr commented May 26, 2018

Uh oh!

stephtr commented Jul 1, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

stephtr commented Apr 5, 2018 •

edited

Loading

coveralls commented Apr 5, 2018 •

edited

Loading