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 issue 241 (handle case where bash is not installed) #242

Merged
merged 4 commits into from
May 23, 2020

Conversation

nikita-skobov
Copy link
Contributor

closes #241

execShellScript needs to check for errors in addition to close signals, otherwise it never rejects.

I added tests for resolving and rejecting, but they are pretty bad IMO. Im not well versed in typescript, so I couldn't figure out how to use jest mock functions other than adding // @ts-ignore, so let me know if there's a better way to accomplish this

@nikita-skobov nikita-skobov mentioned this pull request May 21, 2020
@nikita-skobov
Copy link
Contributor Author

@skovhus I got a linting error from using ts-ignore on my tests. Can you review, and tell me what the proper typescript way is to mock child_process?

Copy link
Collaborator

@skovhus skovhus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Just to clarify: the current code handles normal exits fine, but it doesn’t handle if bash doesn’t exist on the path.

There are a few workaround for the Jest mock issue, the best one probably being https://github.com/kulshekhar/ts-jest/blob/master/docs/user/test-helpers.md#mocked

I would avoid mocking for all these cases we can avoid it. This can be done by calling with a command that doesn’t exist for example. For the cases we cannot avoid to mock, then you can use the ts-jest mocked helper.

@nikita-skobov
Copy link
Contributor Author

I would avoid mocking for all these cases we can avoid it. This can be done by calling with a command that doesn’t exist for example. For the cases we cannot avoid to mock, then you can use the ts-jest mocked helper.

The line within execShellScript

const process = ChildProcess.spawn('bash', args)

contains a child process spawn call of the string 'bash', if we wish to test the function execShellScript we either have to mock child process spawn, or otherwise temporarily delete the bash executable from the users system. If you want to avoid mocks, should I just remove those test cases?

Otherwise a slightly easier approach would be to modify the execShellScript function to optionally take in a command to run instead of just bash, that way when we test it, we can pass in some command that doesnt exist like: dr43hgvfe or whatever.

@skovhus
Copy link
Collaborator

skovhus commented May 22, 2020

Yes, the case bash is not on the path we would still need to mock it. But I like your suggestion of make it parameterized. Then you can make bash the default value of that parameter. 👍

@codecov
Copy link

codecov bot commented May 22, 2020

Codecov Report

Merging #242 into master will increase coverage by 0.46%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #242      +/-   ##
==========================================
+ Coverage   73.30%   73.77%   +0.46%     
==========================================
  Files          18       18              
  Lines         547      549       +2     
  Branches       87       87              
==========================================
+ Hits          401      405       +4     
+ Misses        126      125       -1     
+ Partials       20       19       -1     
Impacted Files Coverage Δ
server/src/util/sh.ts 88.46% <100.00%> (+4.46%) ⬆️

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 016291a...689decb. Read the comment docs.

@nikita-skobov nikita-skobov requested a review from skovhus May 23, 2020 00:22
@skovhus skovhus merged commit 72ca9de into bash-lsp:master May 23, 2020
@skovhus skovhus changed the title Fix issue 241 Fix issue 241 (handle case where bash is not installed) May 23, 2020
akurtakov added a commit to akurtakov/shellwax that referenced this pull request May 27, 2020
Language server changelog:
1.16.1

    Fix brace expansion bug
(bash-lsp/bash-language-server#240)
    Do not crash if bash is not installed
(bash-lsp/bash-language-server#242)

1.16.0

    Improved completion handler for parameter expansions
(bash-lsp/bash-language-server#237)

1.15.0

    Use comments above symbols for documentation
(bash-lsp/bash-language-server#234,
bash-lsp/bash-language-server#235)

1.14.0

    onHover and onCompletion documentation improvements
(bash-lsp/bash-language-server#230)
    support 0/1 as values for HIGHLIGHT_PARSING_ERRORS
(bash-lsp/bash-language-server#231)

1.13.1

    Gracefully handle glob failures
(bash-lsp/bash-language-server#224,
bash-lsp/bash-language-server#226)
    Maintenance
(bash-lsp/bash-language-server#222,
bash-lsp/bash-language-server#225)


Signed-off-by: Alexander Kurtakov <[email protected]>
akurtakov added a commit to eclipse-shellwax/shellwax that referenced this pull request May 27, 2020
Language server changelog:
1.16.1

    Fix brace expansion bug
(bash-lsp/bash-language-server#240)
    Do not crash if bash is not installed
(bash-lsp/bash-language-server#242)

1.16.0

    Improved completion handler for parameter expansions
(bash-lsp/bash-language-server#237)

1.15.0

    Use comments above symbols for documentation
(bash-lsp/bash-language-server#234,
bash-lsp/bash-language-server#235)

1.14.0

    onHover and onCompletion documentation improvements
(bash-lsp/bash-language-server#230)
    support 0/1 as values for HIGHLIGHT_PARSING_ERRORS
(bash-lsp/bash-language-server#231)

1.13.1

    Gracefully handle glob failures
(bash-lsp/bash-language-server#224,
bash-lsp/bash-language-server#226)
    Maintenance
(bash-lsp/bash-language-server#222,
bash-lsp/bash-language-server#225)


Signed-off-by: Alexander Kurtakov <[email protected]>
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.

Plugin crash
2 participants