add "id" to ContributedTaskConfiguration interface #7670

elaihau · 2020-04-26T19:55:27Z

all detected tasks are associated with unique IDs (

theia/packages/plugin-ext/src/plugin/types-impl.ts

Lines 1786 to 1800 in bda9ff9

    
           private updateDefinitionBasedOnExecution(): void { 
        
               if (this.taskExecution instanceof ProcessExecution) { 
        
                   Object.assign(this.taskDefinition, { 
        
                       type: 'process', 
        
                       id: this.taskExecution.computeId(), 
        
                       taskType: this.taskDefinition!.type 
        
                   }); 
        
               } else if (this.taskExecution instanceof ShellExecution) { 
        
                   Object.assign(this.taskDefinition, { 
        
                       type: 'shell', 
        
                       id: this.taskExecution.computeId(), 
        
                       taskType: this.taskDefinition!.type 
        
                   }); 
        
               } 
        
           }

). This pull request adds the "id" property to the ContributedTaskConfiguration interface.

fixes Tasks dependency resolution does not work as in VS Code #7514

Signed-off-by: Liang Huang [email protected]

How to test

define detected tasks in the workspace. I added 2 into my package.json:

  "scripts": {
    "lintAAB": "eslint .",
    "list": "sleep 1 && ls"
  },

Customize one of the detected task defined in Step 1. And make it dependent on the other detected task.
This is what I added to my tasks.json:

        {
            "type": "npm",
            "script": "list",
            "label": "NPM LIST LABEL",
            "problemMatcher": [],
            "dependsOn":["NPM LINT LINT"]
        },

Check if "dependsOn" works properly on detected tasks.

Review checklist

as an author, I have thoroughly tested my changes and carefully followed the review guidelines

elaihau · 2020-04-26T19:56:35Z

packages/task/src/common/task-protocol.ts

+     * ID of the provided/detected task.
+     * This field is not supposed to be used in `tasks.json`.
+     */
+    readonly id: string;


Hi @RomanNikitenko , does this change look OK to you?

Hello, Liang!
Yes, it looks good to me.

If I understand correctly, we need these changes (run a task by id instead of by source and label) to have more reliable way of matching a task for running.
Right?

yes. the label of detected tasks can be customized by the user so it would be easier to use id instead.

- all detected tasks are associated with unique IDs (https://github.com/eclipse-theia/theia/blob/bda9ff9d4cf15b28b5ffc2e984f11834adfc8f33/packages/plugin-ext/src/plugin/types-impl.ts#L1786-L1800). This pull request adds the "id" property to the ContributedTaskConfiguration interface. - fixes #7514 Signed-off-by: Liang Huang <[email protected]>

RomanNikitenko

Tested for npm build and clean tasks, it works well for me!

For tsc tasks I have problems:

But looks like it's due to: #7684

tsmaeder · 2020-04-29T06:59:41Z

packages/task/src/browser/provided-task-configurations.ts

@@ -66,6 +66,16 @@ export class ProvidedTaskConfigurations {
        }
    }

+    async getTaskById(id: string): Promise<TaskConfiguration | undefined> {


Looking at the code, I don't think this is going to work: when we cache tasks, two contributed tasks that have the same scope and the same label will overwrite one another: the id does not factor into the key at all.

tsmaeder · 2020-04-29T07:06:27Z

Generally, we should have a conversation about what it means for two tasks (configurations/customizations) to be the same. Shooting from the hip in this area will haunt us in the long run, IMO. A simple test I run is this: I have an extension that contributes two tasks that are identical: They still both show up when doing a "run task".

tsmaeder · 2020-04-29T07:28:07Z

See also: #7446 (comment)

akosyakov · 2020-04-29T07:30:48Z

I agree with @tsmaeder I see the same for VS Code extensions APIs: #7681 (comment)

RomanNikitenko · 2020-04-29T12:15:53Z

At the moment there is run(source: string, taskLabel: string, scope?: string) method in task service and we are trying to get the corresponding configuration for running by source and label.

At the same time TaskRunQuickOpenItem contains a configuration.
So, maybe we should have run(task: TaskConfiguration) in task service and just give the corresponding configuration for running (instead of source and label). It will allow to not guess but be sure what config should be running.

I guess for these case we should reconsider way how TaskRunQuickOpen items are prepared. Maybe current state (run by source and label) was caused by some problems related to running detected/customized tasks.

@elaihau @akosyakov @tsmaeder wdyt?

tsmaeder · 2020-04-29T12:42:06Z

@RomanNikitenko public runXXX methods on TaskService go through different execution paths, sometimes, so we'd have to be careful when refactoring this. Generally, I think you're right: it does not make sense when we already have a task in hand to "throw it away" and do another lookup via scope and label (I don't think source should come into this, but that's a different topic).

RomanNikitenko · 2020-04-29T13:15:19Z

I meant run(source: string, taskLabel: string, scope?: string) as a starting point.

At this step we are trying to get a configuration for running by source and label.
I see that this method is used:

for running a configured task - the method is not used, but we already have a config
TaskRunQuickOpenItem - we already have a config
runBuildOrTestTask - we already have a config

We don't have a config:

for task run command
for runLastTask method.

elaihau · 2020-04-30T03:06:23Z

Generally, we should have a conversation about what it means for two tasks (configurations/customizations) to be the same. Shooting from the hip in this area will haunt us in the long run, IMO. A simple test I run is this: I have an extension that contributes two tasks that are identical: They still both show up when doing a "run task".

I agree with @tsmaeder I see the same for VS Code extensions APIs: #7681 (comment)

Thanks for the comments !
@akosyakov @tsmaeder
I also noticed that tasks with the same "type" and "label" can be defined in the tasks.json as well. So maybe the task config interface needs a chanage. And we need a new way to check if two tasks are identical.

I meant run(source: string, taskLabel: string, scope?: string) as a starting point.

At this step we are trying to get a configuration for running by source and label.
I see that this method is used:

for running a configured task - the method is not used, but we already have a config

TaskRunQuickOpenItem - we already have a config

runBuildOrTestTask - we already have a config

We don't have a config:

for task run command

for runLastTask method.

@RomanNikitenko
True. We should add a function to start a task from the task config object. This way we don't have to search every single time.

akosyakov · 2020-04-30T09:41:53Z

I think someone should look at VS Code as a reference implementation and trace how IDs are computed and used there for different kind of tasks, then we should add tests that it works in the same way.

Generally, I think you're right: it does not make sense when we already have a task in hand to "throw it away" and do another lookup via scope and label (I don't think source should come into this, but that's a different topic)

I am not entirely sure about it. I see that we cache provided tasks. Is it correct? Should not we collect them each time when a user want to run or a VS Code extensions wants to fetch? There is no an event saying that now provided tasks are invalid. Our cache can be invalid anytime.

akosyakov · 2020-04-30T09:44:34Z

packages/task/src/browser/provided-task-configurations.ts

@@ -111,6 +121,13 @@ export class ProvidedTaskConfigurations {
        return matchedTask;
    }

+    /** checks if the config is a detected / contributed task */
+    isDetectedTask(task: TaskCustomization | TaskConfiguration | TaskCustomization): task is ContributedTaskConfiguration {


We should be consistent with naming. Either call them provided, detected or contributed. But please not mix.

tsmaeder · 2020-04-30T11:37:55Z

There is no an event saying that now provided tasks are invalid. Our cache can be invalid anytime.

For user-configured tasks, we detect changes via notifications. For contributed tasks, we have to assume some scope of validity: otherwise we'd have to constantly refetch tasks from providers.
My approach in #7633 is to clear the cache each time the user initiates a task-related action.
We could still check if the task is valid once we do something with it (like "run" or "configure"). But since the task is still formally valid (even if it would not be contributed any longer if we asked the provider), we can do without that validation (IMO). If the preconditions of the task are no longer valid, we can just let the task fail.

elaihau · 2020-05-02T21:06:05Z

I did some investigation with a vscode extension.

same tasks, or tasks with identical labels, can be contributed by task providers more than once. duplications are not removed.
same tasks, or tasks with identical labels, can be added to the same tasks.json more than once. duplications are not removed.
However, a detected task, if configured in tasks.json, neither shows up in the list populated by Run Tasks, nor is returned by vscode.fetchTasks(). From the users' stand point it is "replaced" by its configured version.

akosyakov · 2020-05-04T07:49:34Z

@elaihau Is there a way to turn your investigation in tests, similarly how it was done for launch configurations: https://github.com/akosyakov/vscode-launch/blob/master/src/test/extension.test.ts

Think about different variables which can affect results of fetchTasks. What values can such variables have? Can be such values grouped to test a group only once? Test different combinations of values, one for each combination is enough. We will translate such tests in integration tests for Theia.

Look at source code as well. VS Code have CommonTask and then 3 subclasses ConfiguredTask, CustomizedTask and ContributedTask. All of them have id property. How ID is computed for each kind for different code paths and why? Could you come up with more tests which will be broken if we compute ID differently, i.e. looking at the code they factor in properties of tasks as well, not only labels.

elaihau · 2020-05-06T03:24:09Z

@elaihau Is there a way to turn your investigation in tests, similarly how it was done for launch configurations: https://github.com/akosyakov/vscode-launch/blob/master/src/test/extension.test.ts

Think about different variables which can affect results of fetchTasks. What values can such variables have? Can be such values grouped to test a group only once? Test different combinations of values, one for each combination is enough. We will translate such tests in integration tests for Theia.

@akosyakov Thank you for the guidance !
are you suggesting me to write integration tests before writing code? or maybe you are reminding me to add integration tests after the change?

I think i would be able to translate what i described above into tests for $fetchTasks(). Do we have any tests for Theia's plugin API? I went through the tests in examples/api-tests, and found they are the ones to test classes in Theia extensions.

akosyakov · 2020-05-06T07:44:43Z

are you suggesting me to write integration tests before writing code? or maybe you are reminding me to add integration tests after the change?

Whatever suits you. But I don't see a point to write code If we don't know what is the proper behaviour. For a start just creating a VS Code extension testing fetchTasks under different conditions and preserving it as tests will help.

I think i would be able to translate what i described above into tests for $fetchTasks(). Do we have any tests for Theia's plugin API? I went through the tests in examples/api-tests, and found they are the ones to test classes in Theia extensions.

You don't need a VS Code extension to extract and test fetchTasks function? You can do whatever VS Code extensions do with Theia APIs directly?

elaihau · 2020-06-07T20:50:15Z

Thank you all for the feedback.
The goal and scope of the pull request have changed quite a bit with your suggestions.
I opened a separate PR #7975 for your review, and am closing this one.

elaihau commented Apr 26, 2020

View reviewed changes

elaihau force-pushed the Liang/addIdToTaskConfig branch from 6324664 to caf6fc5 Compare April 26, 2020 19:59

akosyakov requested review from RomanNikitenko and tsmaeder April 27, 2020 08:00

akosyakov added tasks issues related to the task system vscode issues related to VSCode compatibility labels Apr 27, 2020

akosyakov requested a review from vince-fugnitto April 27, 2020 08:01

RomanNikitenko approved these changes Apr 28, 2020

View reviewed changes

tsmaeder requested changes Apr 29, 2020

View reviewed changes

akosyakov reviewed Apr 30, 2020

View reviewed changes

akosyakov mentioned this pull request Apr 30, 2020

manage cache lifetime explicitly #7633

Closed

1 task

elaihau closed this Jun 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add "id" to ContributedTaskConfiguration interface #7670

add "id" to ContributedTaskConfiguration interface #7670

elaihau commented Apr 26, 2020

elaihau Apr 26, 2020

RomanNikitenko Apr 28, 2020 •

edited

Loading

elaihau Apr 28, 2020

RomanNikitenko left a comment •

edited

Loading

tsmaeder Apr 29, 2020

tsmaeder commented Apr 29, 2020

tsmaeder commented Apr 29, 2020

akosyakov commented Apr 29, 2020

RomanNikitenko commented Apr 29, 2020 •

edited

Loading

tsmaeder commented Apr 29, 2020

RomanNikitenko commented Apr 29, 2020

elaihau commented Apr 30, 2020

akosyakov commented Apr 30, 2020 •

edited

Loading

akosyakov Apr 30, 2020

tsmaeder commented Apr 30, 2020

elaihau commented May 2, 2020 •

edited

Loading

akosyakov commented May 4, 2020

elaihau commented May 6, 2020 •

edited

Loading

akosyakov commented May 6, 2020 •

edited

Loading

elaihau commented Jun 7, 2020 •

edited

Loading

	private updateDefinitionBasedOnExecution(): void {
	if (this.taskExecution instanceof ProcessExecution) {
	Object.assign(this.taskDefinition, {
	type: 'process',
	id: this.taskExecution.computeId(),
	taskType: this.taskDefinition!.type
	});
	} else if (this.taskExecution instanceof ShellExecution) {
	Object.assign(this.taskDefinition, {
	type: 'shell',
	id: this.taskExecution.computeId(),
	taskType: this.taskDefinition!.type
	});
	}
	}

add "id" to ContributedTaskConfiguration interface #7670

add "id" to ContributedTaskConfiguration interface #7670

Conversation

elaihau commented Apr 26, 2020

How to test

Review checklist

elaihau Apr 26, 2020

Choose a reason for hiding this comment

RomanNikitenko Apr 28, 2020 • edited Loading

Choose a reason for hiding this comment

elaihau Apr 28, 2020

Choose a reason for hiding this comment

RomanNikitenko left a comment • edited Loading

Choose a reason for hiding this comment

tsmaeder Apr 29, 2020

Choose a reason for hiding this comment

tsmaeder commented Apr 29, 2020

tsmaeder commented Apr 29, 2020

akosyakov commented Apr 29, 2020

RomanNikitenko commented Apr 29, 2020 • edited Loading

tsmaeder commented Apr 29, 2020

RomanNikitenko commented Apr 29, 2020

elaihau commented Apr 30, 2020

akosyakov commented Apr 30, 2020 • edited Loading

akosyakov Apr 30, 2020

Choose a reason for hiding this comment

tsmaeder commented Apr 30, 2020

elaihau commented May 2, 2020 • edited Loading

akosyakov commented May 4, 2020

elaihau commented May 6, 2020 • edited Loading

akosyakov commented May 6, 2020 • edited Loading

elaihau commented Jun 7, 2020 • edited Loading

RomanNikitenko Apr 28, 2020 •

edited

Loading

RomanNikitenko left a comment •

edited

Loading

RomanNikitenko commented Apr 29, 2020 •

edited

Loading

akosyakov commented Apr 30, 2020 •

edited

Loading

elaihau commented May 2, 2020 •

edited

Loading

elaihau commented May 6, 2020 •

edited

Loading

akosyakov commented May 6, 2020 •

edited

Loading

elaihau commented Jun 7, 2020 •

edited

Loading