jest-editor-support: Add support for Windows Subsystem for Linux in Runner #6941

mojadev · 2018-09-03T20:31:11Z

Add useWsl configuration flag to project worksapce
Translate windows paths in call args to POSIX paths valid in WSL

Summary

See jest-community/vscode-jest#331 for motivation.
When running vscode in Windows but executing jest in WSL (WIndows Subsystem for Linux), additional steps have to be performed:

project_workspace now has a useWsl flag (like the built-in flag for vscode launch options) which can be set to true
The wsl command has to be prepended to the shell command if useWsl is true
The jest output path is a windows path, but wsl is running in a different filesystem with POSIX paths (usually C:/ is /mnt/c), this has to be translated.
The last step is done in a own package wsl-path, as this is also required in vscode-jest and I don't really see it in the scope of either jest or vscode jest (see https://github.com/mojadev/wsl-path)

Test plan

See the following command being executed on a Windows 10 Host - normally the path would be C:..

codecov-io · 2018-09-03T21:16:55Z

Codecov Report

Merging #6941 into master will increase coverage by 0.03%.
The diff coverage is 89.47%.

@@            Coverage Diff             @@
##           master    #6941      +/-   ##
==========================================
+ Coverage   67.25%   67.28%   +0.03%     
==========================================
  Files         248      249       +1     
  Lines        9641     9614      -27     
  Branches        3        3              
==========================================
- Hits         6484     6469      -15     
+ Misses       3156     3144      -12     
  Partials        1        1

Impacted Files	Coverage Δ
...kages/jest-editor-support/src/project_workspace.js	`100% <100%> (ø)`	⬆️
packages/jest-editor-support/src/Runner.js	`88.09% <100%> (ø)`	⬆️
packages/jest-editor-support/src/Process.js	`88.88% <85.71%> (-4.45%)`	⬇️
...ackages/jest-editor-support/src/readTestResults.js	`90.47% <90.47%> (ø)`
packages/jest-runtime/src/helpers.js	`50% <0%> (-40.48%)`	⬇️
packages/babel-jest/src/index.js	`39.62% <0%> (-1.12%)`	⬇️
packages/jest-runtime/src/index.js	`76.64% <0%> (-0.53%)`	⬇️
packages/jest-validate/src/index.js	`0% <0%> (ø)`	⬆️
packages/jest-message-util/src/index.js	`80.91% <0%> (ø)`	⬆️
packages/jest-jasmine2/src/index.js	`0% <0%> (ø)`	⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 814c929...9f62a27. Read the comment docs.

SimenB · 2018-09-03T22:21:56Z

@orta @seanpoulter @connectdotz mind reviewing this?

seanpoulter · 2018-09-03T22:56:27Z

I'll have some time towards the end of the week to really get into it if the other two don't. Looks great at a glance though. Nicely done Jannis!

SimenB · 2018-09-17T11:53:49Z

@orta @seanpoulter @connectdotz ping 🙂

connectdotz · 2018-09-18T04:12:28Z

I don't have a window machine to verify this, if @seanpoulter didn't have time to review it, I suggest @stephtr who actually reviewed the original jest-community/vscode-jest#331

stephtr · 2018-09-22T10:17:52Z

packages/jest-editor-support/src/Process.js


  // If a path to configuration file was defined, push it to runtimeArgs
  if (workspace.pathToConfig) {
    runtimeArgs.push('--config');
    runtimeArgs.push(workspace.pathToConfig);
  }

+  if (workspace.useWsl) {
+    runtimeArgs = [command, ...runtimeArgs.map(convertWslPath)];


Just for consistency I would also convert command such that we also support absolute commands.

stephtr · 2018-09-22T10:39:29Z

At first thank you for your work!
One thing I noticed is that if you have several Linux distros installed in parallel (which is perfectly supported by Microsoft), you have no chance of specifying in which one the command should be executed. One way of solving it would be to not automatically prepend wsl as a command but have the user to explicitly set for example bash npm test --.
Beside that, is it intended that we transform windows -> linux paths in jest-editor-support but do the backtransformation in vscode-jest instead of within Runner.js?

mojadev · 2018-09-24T10:08:16Z

Hey,
very good points - I'll rework the PR this week.

Regarding multiple wsl environments -> I basically mirrored the behaviour of the launch.json configuration, which always uses the default distro. Originally I also didn't have the prepend command magic in there, but I didn't like having to set two settings (one useWsl + an updated path) - detecting the environment from the path is not really an option, I think. How about useWsl: string | boolean, e.g.

    useWsl: true // default wsl env
    useWsl: 'ubuntu' // use ubuntu
    useWsl: 'bash;  // use bash

as possible config options?

Having the backtransformation in vscode-jest would be definetly best (also because I don't need two PRs :) ), but where would you see that happening right? When I get it right, editor-support is only providing the output as is, reading the actual results is done in vscode-jest. I could see two options to support mapping only in one place:

Move reading the json file to editor-support and make an implicit mapping when in wsl
Move wsl support deeper into jest, so the file is already mapped, which I would not really consider an option
I didn't want to change too much in the existing code for this PR, but I'm willing to :)

stephtr · 2018-09-24T10:34:52Z

How about useWsl: string | boolean

In my opinion that would be perfect!

When I get it right, editor-support is only providing the output as is, reading the actual results is done in vscode-jest.

You initially added the translateWslPathsToWindowsPaths to updateWithData, which itself is called within jestProcess.onJestEditorSupportEvent('executableJSON', (data: JestTotalResults) => {this.updateWithData(data)}), which in turn gets triggered by _parseOutput within jest-editor-support/Runner.js.

mojadev · 2018-09-24T10:50:01Z

You're right, I somehow had in mind vscode-jest reads the file by itself. Thanks!

Then I'll

update wsl-path to support multiple environments
update useWsl to (string | boolean)
move the translate logic out of vscode-jest and into _parseOuptut
also translate the command, as noted in your code review

I hope that I manage to do it in the next few days

stephtr · 2018-09-24T21:19:30Z

Sounds good to me.
@connectdotz or @seanpoulter: is that ok for you?

seanpoulter · 2018-09-25T00:38:47Z

I'm unable to comment for a few weeks, sorry.

mojadev · 2018-10-07T20:00:22Z

@stephtr : Sorry for the delay here (I didn't find too much time for reworking it, and my main system is Fedora again). One question regarding file naming: I will add a result_reader module which contains a readTestResults. Now I'm a bit confused because some modules are uppercase (Process.js) and others are lower / snakecase (test_reconciler). Is there some kind of pattern behind this? Should I use result_reader.js or ResultReader.js?

SimenB · 2018-10-07T21:31:44Z

See #4969

…unner * Add useWsl configuration flag to project worksapce * Translate windows paths in call args to POSIX paths valid in WSL see jest-community/vscode-jest#331

…upport, support multiple wsl environments - The useWsl config in ProjectWorkspace can now be either 'true' or the actual wsl command (e.g. 'ubuntu run') - Resolving the path is done after resolving the json file, so for projects using jest-editor-support no extra effort is required to support useWsl

mojadev · 2018-10-14T19:30:17Z

@stephtr I updated the PR as discussed, maybe you find some time to look over it

SimenB · 2018-10-16T07:55:08Z

packages/jest-editor-support/index.d.ts

what's up? lf vs crlf?

Strange - in my local dev environment it's LF (and I disabled autoCLRF), I'll verify that

In the master this file is CLRF while all other files are LF - is that intended?

file index.d.ts index.d.ts: ASCII text, with CRLF line terminators

if so, I switch back to CLRF for this file only

I'm quite sure that this wasn't intended, but it's probably better to change line endings in a separate PR.

SimenB · 2018-10-16T07:56:17Z

packages/jest-editor-support/src/Runner.js

@@ -40,7 +42,7 @@ export default class Runner extends EventEmitter {
    this._createProcess = (options && options.createProcess) || createProcess;
    this.options = options || {};
    this.workspace = workspace;
-    this.outputPath = tmpdir() + '/jest_runner.json';
+    this.outputPath = tmpdir() + sep + 'jest_runner.json';


path.join instead

SimenB · 2018-10-22T11:23:56Z

@mojadev could you open this up against https://github.com/jest-community/jest-editor-support (See #7232).

Thanks, and sorry for the inconvenience!

mojadev · 2018-10-22T16:56:38Z

No problem! Actually the big jest setup causes real issues for me under WSL (mostly yarn and npm related), so I appreciate having a dedicated repo. From: Simen Bekkhus Sent: Monday, October 22, 2018 1:30 PM To: facebook/jest Cc: Jannis Moßhammer; Mention Subject: Re: [facebook/jest] jest-editor-support: Add support for WindowsSubsystem for Linux in Runner (#6941) @mojadev could you open this up against https://github.com/jest-community/jest-editor-support (See #7232). Thanks, and sorry for the inconvenience! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

stephtr

Sorry for the delay.
In principle it looks good to me, once we have the PR in the new repository, we will test it and hopefully ship it soon.

stephtr · 2018-10-25T06:25:42Z

packages/jest-editor-support/index.d.ts

I'm quite sure that this wasn't intended, but it's probably better to change line endings in a separate PR.

stephtr · 2018-10-25T06:30:15Z

packages/jest-editor-support/src/readTestResults.js

+ *
+ * @param coverageMap The coverage map to rewrite
+ */
+const translateWslPathCoverateToWindowsPaths = (


"Coverate" is probably a spelling mistake?

stephtr · 2018-10-25T06:35:04Z

packages/jest-editor-support/src/Process.js

+  if (workspace.useWsl) {
+    // useWsl can be either true for the default ('wsl' or the explicit
+    // wsl call to use, e.g. 'ubuntu run')
+    const wslCommand = workspace.useWsl === true ? 'wsl' : workspace.useWsl;


In order to not duplicate your code, I would use your getWslCommand function from readTestResults.js (even though I would then place the function in another file, maybe we need a helper.js file or similar).

github-actions · 2021-05-12T06:07:45Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

mojadev requested a review from orta as a code owner September 3, 2018 20:31

facebook-github-bot added the cla signed label Sep 3, 2018

mojadev mentioned this pull request Sep 3, 2018

Fix #331 - Implement uswWsl flag and POSIX <-> Windows path mapping jest-community/vscode-jest#375

Closed

mojadev force-pushed the wsl-support-in-runner branch from 62135b3 to e46ab29 Compare September 3, 2018 20:48

stephtr reviewed Sep 22, 2018

View reviewed changes

stephtr mentioned this pull request Sep 28, 2018

No way to use jest installed within Windows Subsystem for Linux jest-community/vscode-jest#331

Closed

mojadev force-pushed the wsl-support-in-runner branch from e46ab29 to 8e188b6 Compare October 14, 2018 17:43

jest-editor-support: Add support for Windows Subsystem for Linux in R…

f76ae0c

…unner * Add useWsl configuration flag to project worksapce * Translate windows paths in call args to POSIX paths valid in WSL see jest-community/vscode-jest#331

mojadev force-pushed the wsl-support-in-runner branch 2 times, most recently from 2361f16 to 83573c0 Compare October 14, 2018 18:19

mojadev force-pushed the wsl-support-in-runner branch from 83573c0 to cd11442 Compare October 14, 2018 18:56

SimenB reviewed Oct 16, 2018

View reviewed changes

jest-editor-support: Use join() instead of path for tempPath

9f62a27

mojadev force-pushed the wsl-support-in-runner branch from b4d2799 to 9f62a27 Compare October 19, 2018 07:22

SimenB closed this Oct 22, 2018

stephtr reviewed Oct 25, 2018

View reviewed changes

github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

jest-editor-support: Add support for Windows Subsystem for Linux in Runner #6941

jest-editor-support: Add support for Windows Subsystem for Linux in Runner #6941

mojadev commented Sep 3, 2018

codecov-io commented Sep 3, 2018 •

edited

Loading

SimenB commented Sep 3, 2018

seanpoulter commented Sep 3, 2018

SimenB commented Sep 17, 2018

connectdotz commented Sep 18, 2018

stephtr Sep 22, 2018

stephtr commented Sep 22, 2018

mojadev commented Sep 24, 2018

stephtr commented Sep 24, 2018

mojadev commented Sep 24, 2018 •

edited

Loading

stephtr commented Sep 24, 2018

seanpoulter commented Sep 25, 2018

mojadev commented Oct 7, 2018

SimenB commented Oct 7, 2018

mojadev commented Oct 14, 2018

SimenB Oct 16, 2018

mojadev Oct 16, 2018

mojadev Oct 19, 2018 •

edited

Loading

stephtr Oct 25, 2018

SimenB Oct 16, 2018

SimenB commented Oct 22, 2018

mojadev commented Oct 22, 2018 via email

stephtr left a comment

stephtr Oct 25, 2018

stephtr Oct 25, 2018

stephtr Oct 25, 2018

github-actions bot commented May 12, 2021

jest-editor-support: Add support for Windows Subsystem for Linux in Runner #6941

jest-editor-support: Add support for Windows Subsystem for Linux in Runner #6941

Conversation

mojadev commented Sep 3, 2018

Summary

Test plan

codecov-io commented Sep 3, 2018 • edited Loading

Codecov Report

SimenB commented Sep 3, 2018

seanpoulter commented Sep 3, 2018

SimenB commented Sep 17, 2018

connectdotz commented Sep 18, 2018

Choose a reason for hiding this comment

stephtr commented Sep 22, 2018

mojadev commented Sep 24, 2018

stephtr commented Sep 24, 2018

mojadev commented Sep 24, 2018 • edited Loading

stephtr commented Sep 24, 2018

seanpoulter commented Sep 25, 2018

mojadev commented Oct 7, 2018

SimenB commented Oct 7, 2018

mojadev commented Oct 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mojadev Oct 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimenB commented Oct 22, 2018

mojadev commented Oct 22, 2018 via email

stephtr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 12, 2021

codecov-io commented Sep 3, 2018 •

edited

Loading

mojadev commented Sep 24, 2018 •

edited

Loading

mojadev Oct 19, 2018 •

edited

Loading