Skip to content
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

Fix mapping from Native Windows to WSL Paths #361

Closed

Conversation

mojadev
Copy link

@mojadev mojadev commented Aug 15, 2018

This is a naive fix that allows jest runs in WSL (Windows Subsystem for Linux)
to be executed and also fixes mapping wsl paths to the windows paths (which
are required by the vscode instance that is not running in WSL). Parts
of this fix should be moved to jest-editor-support.

The idea was to touch as little logic as possible and only introduce wsl specific logic in the spawning of the process and the parsing of resultpaths.

  • JestProcess (I have no clue why the diff says everything has changed): The
    // If Windows Subsystem for Linux is used, spawn in wsl
    if (this.projectWorkspace.pathToJest && this.projectWorkspace.pathToJest.startsWith('wsl')) {
      options.createProcess = createProcessInWSL
    }

snippet is a really dirty workaround which I would try to circumvent by updating the jest-editor-support in the aftermath. Having a useWSL Flag would really make sense in the ProjectWorkspace, even if that means it becomes an even bigger data structure

  • WslProcess would also end up in jest-editor-support, or the process implementation there.

  • TestResult has been updated to rewrite the WSL (/mnt/c) paths to Windows paths (C:)

This is a naive fix that allows jest runs in WSL (Windows Subsystem for Linux)
to be executed and also fixes mapping wsl paths to the windows paths (which
are required by the vscode instance that is not running in WSL). Parts
of this fix should be moved to jest-editor-support.w
@DangerCI
Copy link

Fails
🚫

No CHANGELOG added. If this is a small PR, or a bug-fix for an unreleased bug add #trivial to your PR message and re-run CI.

Generated by 🚫 dangerJS

@coveralls
Copy link

Pull Request Test Coverage Report for Build 432

  • 67 of 69 (97.1%) changed or added relevant lines in 2 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.05%) to 83.961%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/JestProcessManagement/JestProcess.ts 43 44 97.73%
src/JestProcessManagement/WslProcess.ts 24 25 96.0%
Files with Coverage Reduction New Missed Lines %
src/TestResults/TestResult.ts 2 91.23%
Totals Coverage Status
Change from base Build 428: -0.05%
Covered Lines: 724
Relevant Lines: 843

💛 - Coveralls

@stephtr
Copy link
Member

stephtr commented Aug 16, 2018

At first thanks for this PR!
Beside that, as you said, some of that code should probably be moved to the jest-editor-support package. Additionally maybe some path translations could also be done with wslpath.
Another idea would be to remove the automatic WSL detection and instead add a new WSL setting to the plugin (or somehow combine it with the jest.windowsShell setting](#340 (comment)) @connectdotz proposed.
And just as a side note: It seems like in JestProcess.ts the line endings got changed (thus the large diff).

@mojadev
Copy link
Author

mojadev commented Aug 16, 2018

Thanks for looking over it - I will move parts to jest-editor-support as soon as I find some minutes - then I can also use a dedicated flag as I can pass it down using the project workspace, hence getting rid of this ugly wsl detection hack.

Regarding wslpath: I didn't use it as I expect it hit performance rather hard if I do it for every path, but I think it would be sufficient to only convert the projects base path once and then use that one - I'll give that a try. Right now it feels there's a lot of guesswork involved in the path handling and I too would like to get rid of that..

@mojadev
Copy link
Author

mojadev commented Aug 27, 2018

To give a short update here: I moved all the wsl path resolution out to a own package https://github.com/mojadev/wsl-path) and hope to find some tme this evening to wrap up the rest of the PR, as now it's only about detecting wsl (I would use an own setting useWsl, as this already exists in launch configurations of vscode) and calling the path mapping before calling wsl and when parsing he results

@mojadev mojadev closed this Sep 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants