-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
[vscode] fix fetchTasks to no longer filter modified configured tasks #8399
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,174 @@ | ||
/******************************************************************************** | ||
* Copyright (C) 2020 SAP SE or an SAP affiliate company and others. | ||
* | ||
* This program and the accompanying materials are made available under the | ||
* terms of the Eclipse Public License v. 2.0 which is available at | ||
* http://www.eclipse.org/legal/epl-2.0. | ||
* | ||
* This Source Code may also be made available under the following Secondary | ||
* Licenses when the conditions for such availability set forth in the Eclipse | ||
* Public License v. 2.0 are satisfied: GNU General Public License, version 2 | ||
* with the GNU Classpath Exception which is available at | ||
* https://www.gnu.org/software/classpath/license.html. | ||
* | ||
* SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0 | ||
********************************************************************************/ | ||
|
||
import {assert} from 'chai'; | ||
import {Container, injectable} from 'inversify'; | ||
import {enableJSDOM} from '@theia/core/lib/browser/test/jsdom'; | ||
|
||
const disableJSDOM = enableJSDOM(); | ||
import {ApplicationProps} from '@theia/application-package/lib/application-props'; | ||
import {FrontendApplicationConfigProvider} from '@theia/core/lib/browser/frontend-application-config-provider'; | ||
|
||
FrontendApplicationConfigProvider.set({ | ||
...ApplicationProps.DEFAULT.frontend.config | ||
}); | ||
import {TaskConfigurations} from './task-configurations'; | ||
import {TaskConfigurationManager} from './task-configuration-manager'; | ||
import {TaskDefinitionRegistry} from './task-definition-registry'; | ||
import {TaskProvider, TaskProviderRegistry} from './task-contribution'; | ||
import {TaskTemplateSelector} from './task-templates'; | ||
import {TaskSchemaUpdater} from './task-schema-updater'; | ||
import {TaskSourceResolver} from './task-source-resolver'; | ||
import {ProvidedTaskConfigurations} from './provided-task-configurations'; | ||
import {TaskConfiguration, TaskConfigurationScope, TaskCustomization, TaskScope} from '../common/task-protocol'; | ||
import {QuickPickService} from '@theia/core/lib/common/quick-pick-service'; | ||
import {ContributionProvider, Event, ILogger} from '@theia/core/lib/common'; | ||
import { | ||
ApplicationShell, | ||
LabelProviderContribution, | ||
PreferenceProvider, | ||
PreferenceScope, | ||
WidgetManager | ||
} from '@theia/core/lib/browser'; | ||
import {FileService} from '@theia/filesystem/lib/browser/file-service'; | ||
import {WorkspaceService} from '@theia/workspace/lib/browser'; | ||
import {WorkspaceVariableContribution} from '@theia/workspace/lib/browser/workspace-variable-contribution'; | ||
import {Signal} from '@phosphor/signaling'; | ||
import {EditorManager} from '@theia/editor/lib/browser'; | ||
import {PreferenceConfigurations} from '@theia/core/lib/browser/preferences/preference-configurations'; | ||
import {FileChangeType} from '@theia/filesystem/lib/common/files'; | ||
import {IJSONSchema} from '@theia/core/lib/common/json-schema'; | ||
import {MockLogger} from '@theia/core/lib/common/test/mock-logger'; | ||
import {MockPreferenceProvider} from '@theia/core/lib/browser/preferences/test'; | ||
|
||
after(() => disableJSDOM()); | ||
|
||
@injectable() | ||
class MockTaskConfigurationManager extends TaskConfigurationManager { | ||
|
||
changeTasksConfigFire(): void { | ||
this.onDidChangeTaskConfigEmitter.fire({scope: TaskScope.Global, type: FileChangeType.ADDED}); | ||
} | ||
|
||
getTasks(scope: TaskConfigurationScope): (TaskCustomization | TaskConfiguration)[] { | ||
return [ | ||
{ | ||
'type': 'echo', | ||
'label': 'task c', | ||
'text': 'Configured task c' | ||
}, | ||
{ | ||
'type': 'echo', | ||
'label': 'task a', | ||
'text': 'Detected task a' | ||
} | ||
]; | ||
} | ||
} | ||
|
||
class MockTaskProvider implements TaskProvider { | ||
async provideTasks(): Promise<TaskConfiguration[]> { | ||
const taskA: TaskConfiguration = { | ||
type: 'echo', | ||
group: 'build', | ||
_scope: '1', | ||
label: 'task a', | ||
text: 'Detected task a' | ||
}; | ||
const taskB: TaskConfiguration = { | ||
type: 'echo', | ||
group: 'build', | ||
_scope: '1', | ||
label: 'task b', | ||
text: 'Detected task b' | ||
}; | ||
return [ | ||
taskA, taskB | ||
]; | ||
} | ||
} | ||
|
||
describe('getTasks', () => { | ||
|
||
let testContainer: Container; | ||
const taskConfigurationManager = new MockTaskConfigurationManager(); | ||
|
||
it('returns all configured tasks: customized and configured', done => { | ||
const taskConfiguration = testContainer.get<TaskConfigurations>(TaskConfigurations); | ||
taskConfigurationManager.changeTasksConfigFire(); | ||
setTimeout(async () => { | ||
const res = await taskConfiguration.getTasks(1); | ||
assert.equal(res.length, 2); | ||
assert.equal(res[0].label, 'task c'); | ||
assert.equal(res[1].label, 'task a'); | ||
done(); | ||
}, 50); | ||
}); | ||
|
||
before(() => { | ||
testContainer = new Container(); | ||
testContainer.bind(ILogger).toDynamicValue(ctx => new MockLogger()); | ||
const workspaceService = new WorkspaceService(); | ||
testContainer.bind(WorkspaceService).toConstantValue(workspaceService); | ||
testContainer.bind(WorkspaceVariableContribution).toSelf().inSingletonScope(); | ||
testContainer.bind(ApplicationShell).toConstantValue({ | ||
currentChanged: new Signal({}), | ||
widgets: () => [] | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
} as any); | ||
testContainer.bind(EditorManager).toConstantValue(<EditorManager>{}); | ||
testContainer.bind(QuickPickService).toConstantValue(<QuickPickService>{}); | ||
const taskDefinitionRegistry = new TaskDefinitionRegistry(); | ||
taskDefinitionRegistry.register({ | ||
taskType: 'echo', | ||
source: 'echotaskprovider', | ||
properties: { | ||
required: ['label', 'text'], | ||
all: ['label', 'text'], | ||
schema: new class implements IJSONSchema { | ||
} | ||
} | ||
}); | ||
testContainer.bind<TaskDefinitionRegistry>(TaskDefinitionRegistry).toConstantValue(taskDefinitionRegistry); | ||
testContainer.bind<TaskConfigurationManager>(TaskConfigurationManager).toConstantValue(taskConfigurationManager); | ||
|
||
testContainer.bind<ProvidedTaskConfigurations>(ProvidedTaskConfigurations).toSelf(); | ||
testContainer.bind<TaskProviderRegistry>(TaskProviderRegistry).toSelf().inSingletonScope(); | ||
|
||
const taskProviderRegistry = testContainer.get<TaskProviderRegistry>(TaskProviderRegistry); | ||
taskProviderRegistry.register('echo', new MockTaskProvider()); | ||
|
||
testContainer.bind(TaskTemplateSelector).toSelf().inSingletonScope(); | ||
testContainer.bind(TaskSchemaUpdater).toConstantValue(<TaskSchemaUpdater>{ | ||
onDidChangeTaskSchema: Event.None, | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
validate(data?: any): boolean { | ||
return true; | ||
} | ||
}); | ||
testContainer.bind(TaskSourceResolver).toConstantValue(<TaskSourceResolver>{}); | ||
testContainer.bind<TaskConfigurations>(TaskConfigurations).toSelf().inSingletonScope(); | ||
testContainer.bind(FileService).toConstantValue(<FileService>{}); | ||
testContainer.bind(PreferenceProvider).toDynamicValue(ctx => new MockPreferenceProvider(PreferenceScope.Workspace)).inSingletonScope(); | ||
testContainer.bind(PreferenceConfigurations).toConstantValue(<PreferenceConfigurations>{}); | ||
testContainer.bind(WidgetManager).toSelf().inSingletonScope(); | ||
testContainer.bind<ContributionProvider<LabelProviderContribution>>(ContributionProvider).toDynamicValue(ctx => ({ | ||
getContributions(): LabelProviderContribution[] { | ||
return []; | ||
} | ||
})).inSingletonScope(); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -130,7 +130,7 @@ export class TaskConfigurations implements Disposable { | |
* The invalid task configs are not returned. | ||
*/ | ||
async getTasks(token: number): Promise<TaskConfiguration[]> { | ||
const configuredTasks = Array.from(this.tasksMap.values()).reduce((acc, labelConfigMap) => acc.concat(Array.from(labelConfigMap.values())), [] as TaskConfiguration[]); | ||
const configuredTasksMap = new Map(this.tasksMap); | ||
const detectedTasksAsConfigured: TaskConfiguration[] = []; | ||
for (const [rootFolder, customizations] of Array.from(this.taskCustomizationMap.entries())) { | ||
for (const cus of customizations) { | ||
|
@@ -139,9 +139,19 @@ export class TaskConfigurations implements Disposable { | |
if (detected) { | ||
// there might be a provided task that has a different scope from the task we're inspecting | ||
detectedTasksAsConfigured.push({ ...detected, ...cus }); | ||
} else { | ||
if (configuredTasksMap.has(rootFolder)) { | ||
configuredTasksMap.get(rootFolder)!.set(cus['label'], cus as TaskConfiguration); | ||
} else { | ||
const newConfigMap = new Map(); | ||
newConfigMap.set(cus['label'], cus as TaskConfiguration); | ||
configuredTasksMap.set(rootFolder, newConfigMap); | ||
} | ||
} | ||
} | ||
} | ||
const configuredTasks = Array.from(configuredTasksMap.values()).reduce((acc, labelConfigMap) => acc.concat(Array.from(labelConfigMap.values())), [] as TaskConfiguration[]); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As I understand the code, this will still remove any tasks of type "shell", etc (which don't have a task definition) that have the same label (since we're using a map by label). Is the behaviour after this change aligned with VS Code as described in #7672 (comment) ? If I add the following to a tasks.json file:
and then do "run tasks", I see two tasks, as I read the code here, we would only get one task. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are right, the purpose of this PR is not to cover all delta of the fetchTasks between Theia and VSCode. |
||
|
||
return [...configuredTasks, ...detectedTasksAsConfigured]; | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How can we be sure that the
label
field is notundefined
here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with your concern and maybe I have to add a check here.
The reason of such usage of
label
field is caused by the following:PR modifies method
getTasks
.This method processes 2 groups of tasks:
These groups are created in the method
reorganizeTasks
. As you can see, the existing code:theia/packages/task/src/browser/task-configurations.ts
Line 407 in d7f9163
uses the field
label
without additional check.If task is not detected it's assigned to tasksMap group assuming it has field
label
.Don't we miss the check already here?