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

GH-7838: Fixed task execution on macOS with zsh. #7889

Merged
merged 2 commits into from
May 26, 2020
Merged

GH-7838: Fixed task execution on macOS with zsh. #7889

merged 2 commits into from
May 26, 2020

Conversation

kittaakos
Copy link
Contributor

Switched to Xcode 11.4 on Travis CI to support macOS 10.15.4.

Closes #7838.

Signed-off-by: Akos Kitta [email protected]

What it does

How to test

Review checklist

Reminder for reviewers

@kittaakos kittaakos requested a review from AlexTugarev May 26, 2020 07:15
@kittaakos
Copy link
Contributor Author

@AlexTugarev, there is not much to review on this PR, if you can run the @theia/task tests without a problem, then I think we're good here. Thanks!

@akosyakov akosyakov requested a review from paul-marechal May 26, 2020 11:08
@akosyakov akosyakov added process issues related to the process extension tasks issues related to the task system labels May 26, 2020
@@ -150,8 +148,4 @@ export class ShellCommandBuilder {
return command;
}

protected buildForDefault(args: Array<string | ShellQuotedString>, cwd?: string, env?: Array<[string, string | null]>): string {
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: Can you keep this default function, but make it call the bash one? Makes it easier to override if someone needs/wants to.

Akos Kitta added 2 commits May 26, 2020 16:56
Switched to Xcode `11.4` on Travis CI to support macOS `10.15.4`.

Closes #7838.

Signed-off-by: Akos Kitta <[email protected]>
Do not expect to receive the test expectation as the first messages
from the terminal.

Signed-off-by: Akos Kitta <[email protected]>
@kittaakos
Copy link
Contributor Author

I made the requested changes. Thank you for the review!

I also did change (6822129) one of the test assertions; it's a bit more robust now. I found an issue with the expectation after switching macs. The terminal popped up with a warning (this is expected and related to nvm), and that warning was not our expectation. I changed the test to wait for multiple messages instead. Please check.

Copy link
Member

@paul-marechal paul-marechal left a comment

Choose a reason for hiding this comment

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

LGTM thanks!

@kittaakos kittaakos merged commit 1e50741 into master May 26, 2020
@kittaakos kittaakos deleted the GH-7838 branch May 26, 2020 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
process issues related to the process extension tasks issues related to the task system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[task] Task server tests are broken
3 participants