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 Task.execution instantiation #6533

Merged
merged 1 commit into from
Dec 12, 2019
Merged

Fix Task.execution instantiation #6533

merged 1 commit into from
Dec 12, 2019

Conversation

tolusha
Copy link
Contributor

@tolusha tolusha commented Nov 12, 2019

Signed-off-by: Anatoliy Bazko abazko@redhat.com

What it does

Fix Task.execution instantiation.
ShellExecution and ProcessExecution contain additional logic in their constructors.
That's why is it necessary to use them.

[1] https://github.com/theia-ide/theia/blob/ab/fixTaskExecution/packages/plugin-ext/src/plugin/types-impl.ts#L1461
[2] https://github.com/eclipse-theia/theia/blob/ab/fixTaskExecution/packages/plugin-ext/src/plugin/types-impl.ts#L1368

How to test

The following tasks should work:

  1. https://github.com/tolusha/vscode-test-extensions/blob/master/task-test-0.0.1.vsix?raw=true
{
    // comment
    "tasks": [
        {
            "label": "config_task#1",
            "type": "shell",
            "command": "echo",
            "args": [
                "test"
            ]
        },
        {
            "label": "config_task#2",
            "type": "shell",
            "command": "echo test"
        }        
    ]
}

Review checklist

Reminder for reviewers

@tolusha tolusha added bug bugs found in the application tasks issues related to the task system labels Nov 12, 2019
@tolusha tolusha self-assigned this Nov 12, 2019
@tolusha
Copy link
Contributor Author

tolusha commented Nov 12, 2019

@eclipse-theia/task-extension

@akosyakov akosyakov added the plug-in system issues related to the plug-in system label Nov 12, 2019
Copy link
Contributor

@RomanNikitenko RomanNikitenko left a comment

Choose a reason for hiding this comment

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

Looks like tests for converting failed:

tests_tasks

execution.command = processTaskDto.command;

return execution;
export function getProcessExecution(taskDto: TaskDto): theia.ProcessExecution {
Copy link
Contributor

Choose a reason for hiding this comment

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

The function takes TaskDto now, so unnecessary assertion in the place of call

taskDto.options || {});
}

export function getShellExecution(taskDto: TaskDto): theia.ShellExecution {
Copy link
Contributor

Choose a reason for hiding this comment

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

@RomanNikitenko
Copy link
Contributor

@tolusha
Sorry, but I think that for testing your PR it's not enough just to run tasks with type process and shell.

The changes are related to plugin system, so I would say that it's very important to test the changes using some plugin (with task resolver I guess, as a resolver uses converting mechanism).

I tested your changes for che tasks and unfortunately they are broken.
Are you going to provide the corresponding PR to che-theia to adapt execution of che tasks?

@tolusha
Copy link
Contributor Author

tolusha commented Nov 14, 2019

@RomanNikitenko
I've pushed a corresponding fix into https://github.com/eclipse/che-theia/tree/ab/fixTaskExecution
Could you check pls?

@RomanNikitenko
Copy link
Contributor

@tolusha
Thank you, I'm going to test your changes today

@RomanNikitenko
Copy link
Contributor

@tolusha
I tested your changes, after last update it works well for che tasks.
Do you know some VS Code extension/custom plugin to test it for theia project?

thanks in advance!

@tolusha
Copy link
Contributor Author

tolusha commented Dec 9, 2019

It is ready to test.
See How to testsection.
@eclipse-theia/task-extension

Copy link
Contributor

@RomanNikitenko RomanNikitenko left a comment

Choose a reason for hiding this comment

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

I tested the changes:

  • 3 detected tasks provided by the test extension from the PR description
    and
  • che tasks

work well for me!

Signed-off-by: Anatoliy Bazko <abazko@redhat.com>
@akosyakov
Copy link
Member

@tolusha @RomanNikitenko @elaihau Can this PR affect built-in VS Code extensions like npm, gulp, grant and so on? I just don't see that anyone tested against them. If you think that it is not necessary and you are sure everything is alright please go ahead to merge.

@RomanNikitenko
Copy link
Contributor

To be honest, I don't know which VS Code extension would be good for testing these changes.
I'll try it for tsc and npm tasks today and give the feedback, but it still question for me about the relevant VS Code extension for testing...

@RomanNikitenko
Copy link
Contributor

RomanNikitenko commented Dec 11, 2019

I tested npm build and watch tasks.
These tasks have type: 'shell' in the definition and commandLine in the shellExecution(if I understand correctly, the PR changes target is fix for converting shell tasks with commandLine).

So, I checked that at converting the configurations:

  • fromTask they have, for example, commandLine: 'npm run build'(from plugin side to theia configuration)
  • toTask method returns commandLine: 'npm run build' (from theia configuration to plugin config)

Looks good to me and I can run these configurations using the PR changes.

@akosyakov
Copy link
Member

To be honest, I don't know which VS Code extension would be good for testing these changes.
I'll try it for tsc and npm tasks today and give the feedback, but it still question for me about the relevant VS Code extension for testing...

I hope you and @elaihau become experts in this area :) please merge, i don't think we will get better opinion

@tolusha tolusha merged commit e3abdde into master Dec 12, 2019
@tolusha tolusha deleted the ab/fixTaskExecution branch December 12, 2019 07:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug bugs found in the application plug-in system issues related to the plug-in system tasks issues related to the task system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants