Add TaskProvider + integration test infrastructure #1826

TheRealPiotrP · 2017-10-31T07:12:25Z

This PR aims to address #1820 and #1608.

I originally set out to fix #1608 by adding support for VS Code's Tasks API. However, I noticed that the existing build / test infrastructure did not enable me to write meaningful integration tests for this feature. Given that much of the code here is only verifiable in the context of VS Code I decided to make an infrastructure update prior to addressing the task at hand.

Test Infrastructure Changes

My strategy in updating the test infrastructure is two-fold. First, I wanted to be able to run integration tests in the context of representative workspaces. Second, I wanted to be able to access data shared with VS Code which is not subsequently queryable via VS Code API's. Here are the key components of the change:

Test Assets

I introduced a new top-level folder called TestAssets. Within this folder you will find three representative workspaces:

A workspace with a single csproj-based project
A workspace with a sln file and several related projects
A workspace with a single project.json-based project

These workspaces are accompanied by matching .ts files which describe, from a test expectations perspective, the contents of the workspaces.

launch.json updates

The launch.json has been updated to have test execution configurations for each of these workspaces. Selecting a given configuration will open the VS Code Test Host targetted at the specified TestAsset workspace. All of the tests running within that Test Host will then be interacting directly with the target workspace, allowing them to produce predictable results while exhibiting their ability to reason about that wokspace type. Note that in order to run through all test scenarios you must iterate through all available Test Configurations.

package.json updates

package.json was updated to allow cmd-line execution of each of the new Test Configurations. The base npm test command was updated to iterate through all three configurations, thereby ensuring that a single cmd-line invocation will execute all tests.

Product Updates

Atop this test infrastructure I have implemented a basic task provider which intends to replicate the behavior of the original tasks.json implementation. Namely, it generates a build task for each identified project. The feature itself is fairly straightforward, though it does demand that we answer several questions:

[ ] should we simultaneously disable tasks.json generation?
[ ] How do we want the tasks to appear in the VS Code UI? Currently they read dotnet: build {project name}. I would prefer to make these dotnet: build {relative path to project file}. However, I'd like to hear opinions on whether that relative path should be calculated by o#-vscode [by removing the workspace folder path from the fully qualified project path] or by o#-roslyn. @DustinCampbell looking for your insights here.
[ ] How do we coordinate this PR with @WardenGnaw's #1801 work? I'm working with @WardenGnaw offline to figure this out.
[ ] Should we block on microsoft/vscode#33523 being available? Without that change we don't have a mechanism for users to provide additional task configuration data via tasks.json.

dnfclas · 2017-10-31T07:12:30Z

@TheRealPiotrP,
Thanks for your contribution.
To ensure that the project team has proper rights to use your work, please complete the Contribution License Agreement at https://cla2.dotnetfoundation.org.

It will cover your contributions to all .NET Foundation-managed open source projects.
Thanks,
.NET Foundation Pull Request Bot

TheRealPiotrP · 2017-10-31T07:42:20Z

@DustinCampbell @rchande the build failure's output is fairly sparse. A full test pass, however, does require that a project.json-supporting CLI be installed on the build agent. Do we know if 1.0.0-preview2-003231 is available on these agents?

DustinCampbell · 2017-10-31T15:32:10Z

@TheRealPiotrP : That would not be available on these build agents and would need to be installed separately.

DustinCampbell · 2017-10-31T15:32:29Z

Also, it looks like you need to sign the CLA? That's a bit weird.

rchande

Looks pretty good to me. Left a couple of questions.

rchande · 2017-10-31T15:29:27Z

src/omnisharp/extension.ts

@@ -74,6 +76,8 @@ export function activate(context: vscode.ExtensionContext, reporter: TelemetryRe
        localDisposables.push(vscode.languages.registerCodeActionsProvider(documentSelector, codeActionProvider));
        localDisposables.push(reportDiagnostics(server, reporter, advisor));
        localDisposables.push(forwardChanges(server));
+
+        taskProvider.activate(context, server);


taskProvider is disposable, should it get added to localDisposables?

rchande · 2017-10-31T15:39:17Z

test/tasks.test.ts

+        for (let task of buildTasks) {
+            await vscode
+                .commands
+                .executeCommand('workbench.action.tasks.runTask', task.name);


Thanks @DustinCampbell for this little nugget :)

rchande · 2017-10-31T15:51:42Z

src/taskProvider.ts

+		return;
+	}
+
+	taskProvider = vscode.workspace.registerTaskProvider('dotnet', {


Where did the 'dotnet' constant come from?

It's defined in package.json. I chose this constant fairly arbitrarily. Would we prefer c#, csharp, .net?

rchande · 2017-10-31T15:58:10Z

src/taskProvider.ts

+
+let taskProvider: vscode.Disposable | undefined;
+
+export function activate(_context: vscode.ExtensionContext, server: OmniSharpServer): void {


Most of the other features are registered by the extension defined in extension.ts. Does the adapter pattern mean that we will have a separate extension class for every feature we want to test through the adapter?

I'm open for comment on this one. I prefer smaller, task-oriented units of code as I find them easier to understand, test, and modify.

TheRealPiotrP · 2017-10-31T17:04:04Z

@DustinCampbell regarding CLA I went through the flow again after being prompted yesterday. Do you know how to get the system to reconsider?

WardenGnaw · 2017-10-31T17:10:48Z

Is the generated task's type field correct?

This is from selecting the gear icon from build task runner.

ghost · 2017-10-31T17:21:01Z

Hi, I am closing and re-opening this PR to bump the CLA service. Sorry for the inconvenience!

TheRealPiotrP · 2017-10-31T17:25:40Z

@WardenGnaw I believe this is due to the default tasks.json schema validator looking at the 0.1.0 schema instead of 2.0.0. The configuration here is the same as in the example: https://github.com/Microsoft/vscode-extension-samples/blob/master/task-provider-sample/src/extension.ts#L125.

TheRealPiotrP · 2017-11-01T00:43:21Z

@rchande @DustinCampbell any guesses here? I finally got myself into a state where tests are running on CI, but O#-Roslyn is discovering 0 projects where I expected to see 1. Could this be a missing dependency issue?

DustinCampbell · 2017-11-01T14:43:43Z

Out of curiosity, why are we working so hard to add support for project.json? Project.json projects are officially deprecated in C# for VS Code and the extension displays a warning to the user about migration whenever OmniSharp is launched in such a way that it loads a project.json project. I would propose that we don't bother with project.json and just target csproj.

Also, note that project.json simply does not work on macOS 10.13 (High Sierra). Old .NET Core SDKs from the project.json error just segfault on 10.13. So, any project.json tests that require restore or build will fail on newer operating systems. I'm on High Sierra already. So, I wouldn't even be able to run tests like that.

DustinCampbell

Looks good! However, I'd like to have a conversation about project.json. I'm not sure we need to expend effort implementing new features in support of that.

DustinCampbell · 2017-11-01T14:44:51Z

package.json

+					}
+				}
+			}
+		]


indent level looks different here. Was that intentional?

DustinCampbell · 2017-11-01T14:47:27Z

src/taskProvider.ts

+		for (let project of workspaceInformation.DotNet.Projects) {
+			tasks.push(provideBuildTask(project.Path, project.Path));
+		}
+	}


FWIW, I wouldn't bother adding this feature for project.json, since the support is officially deprecated.

DustinCampbell · 2017-11-01T14:48:13Z

src/taskProvider.ts

+import { TaskRevealKind } from 'vscode';
+
+export function activate(_context: vscode.ExtensionContext, server: OmniSharpServer): vscode.Disposable {
+	if (!vscode.workspace.workspaceFolders) {


Please use spaces rather than tabs. This is defined the repro's .vscode/settings.json file.

not sure why that wasn't respected... fixed.

also added a tslint rule

DustinCampbell · 2017-11-01T14:49:17Z

src/taskProvider.ts

+async function provideDotnetTasks(server: OmniSharpServer): Promise<vscode.Task[]> {
+	let tasks: vscode.Task[] = [];
+
+	let workspaceInformation = await serverUtils.requestWorkspaceInformation(server);


Thank you for the first usage of await! 😄

DustinCampbell · 2017-11-01T14:51:00Z

src/vscodeAdapter.ts

@@ -0,0 +1,62 @@
+import * as vscode from "vscode";


It feels like this test infrastructure stuff should be in a separate folder rather than at the top-level src. Should it be somewhere in the test folder? Or, does this need to be included with the extension?

DustinCampbell · 2017-11-03T14:31:49Z

.vscode/launch.json

@@ -7,6 +7,7 @@
            "request": "launch",
            "runtimeExecutable": "${execPath}",
            "args": [
+                "${workspaceRoot}/test/integrationTests/testAssets/slnWithCsproj", 


Did you intend to change the default behavior of F5 on the extension?

Yes I did. The extension typically opens in an arbitrary directory and I wanted to have it open in a directory that represents a 'kitchen sink'. I'm fine with reverting this, but found it useful for my own development.

Often, when reproducing an issue, I open the debuggee on a particular folder. Then, subsequent F5 will launch on the same directory allowing me to continue debugging. With this change, I'd need to either update launch.json first or open the folder on every debug session. I would definitely prefer not having this.

DustinCampbell · 2017-11-03T15:58:36Z

src/features/definitionMetadataDocumentProvider.ts

+/*---------------------------------------------------------------------------------------------
+ *  Copyright (c) Microsoft Corporation. All rights reserved.
+ *  Licensed under the MIT License. See License.txt in the project root for license information.
+ *--------------------------------------------------------------------------------------------*/


yay tslint!

DustinCampbell · 2017-11-03T16:05:31Z

src/vscodeTasksAdapter.ts

@@ -0,0 +1,40 @@
+/*---------------------------------------------------------------------------------------------
+ *  Copyright (c) Microsoft Corporation. All rights reserved.


Should this be in the 'src' folder or the 'test' folder?

fixed. I created this directory structure:

test

unitTests

integrationTests

testAssets

DustinCampbell · 2017-11-03T16:06:02Z

src/taskProvider.ts

+import { TaskRevealKind } from 'vscode';
+
+export function activate(_context: vscode.ExtensionContext, server: OmniSharpServer): vscode.Disposable {
+	if (!vscode.workspace.workspaceFolders) {


DustinCampbell

Looks good! However, it wasn't clear to me how we were avoiding running the project.json test. How does that work?

Also automatically selects the workspace for a given test run

dnfclas added the cla-required label Oct 31, 2017

rchande approved these changes Oct 31, 2017

View reviewed changes

ghost closed this Oct 31, 2017

ghost reopened this Oct 31, 2017

dnfclas added the cla-already-signed label Oct 31, 2017

ghost removed the cla-required label Oct 31, 2017

TheRealPiotrP force-pushed the dev/piotrpMSFT/tasksApi branch 2 times, most recently from 272863d to 78a6e93 Compare November 1, 2017 00:15

DustinCampbell requested changes Nov 1, 2017

View reviewed changes

TheRealPiotrP force-pushed the dev/piotrpMSFT/tasksApi branch 5 times, most recently from 8fbc18e to 19bce75 Compare November 3, 2017 04:53

DustinCampbell reviewed Nov 3, 2017

View reviewed changes

Piotr Puszkiewicz added 4 commits November 3, 2017 12:59

TasksProvider with integration tests

3842a09

Test refactoring

57ab33f

Add basis for PJ tests

a834ff7

Add sln test workspace

0e92615

Also automatically selects the workspace for a given test run

Piotr Puszkiewicz added 27 commits November 3, 2017 16:23

Add PJ support and increase details in msbuild tasks.

058c644

add task definition to package.json + small refactoring of taskProvider

e27ca77

Make cmd-line test execution silent

28cea2e

Add taskProvider to localDisposables

8404024

Force task list pre-population

9145996

Activate the csharp extension before running tests

3a668ea

invoke runTask to enable task discovery

d89ca3e

Wait for task provider registration

2be55c0

temporary: validate workspace directory

ef43f90

Add dotnet CLI 2.0.2 to installation

5b3dbef

rename vscodeAdapter to vscodeTasksAdapter

6bd1e04

...

5ec5918

....

a80930e

Is it flaky?

f79ebf8

Reduce diagnostic logs

c160b16

Test Suites

5bec03b

clean up testAssets code

f732022

Fix up launch.json

1aed966

fix up launch config

46d71bd

PR feedback

228ad80

Move test functionality out of product code

6899a21

Tighten down adapter, remove unnecessary index.ts

2cb24f0

clarify name of testWrapper

de7d4a6

Add unit tests for build task creation

ab99081

Add license headers + tslint rule for license headers

999f611

tslint-clean

891436e

Merge conflicts

a0e69a4

TheRealPiotrP force-pushed the dev/piotrpMSFT/tasksApi branch from 9db4d66 to a0e69a4 Compare November 4, 2017 00:23

TheRealPiotrP mentioned this pull request Nov 4, 2017

Integration Test Infrastructure #1834

Merged

TheRealPiotrP closed this Nov 8, 2017


		let taskProvider: vscode.Disposable \| undefined;

		export function activate(_context: vscode.ExtensionContext, server: OmniSharpServer): void {

		@@ -0,0 +1,40 @@
		/*---------------------------------------------------------------------------------------------
		* Copyright (c) Microsoft Corporation. All rights reserved.

Add TaskProvider + integration test infrastructure #1826

Add TaskProvider + integration test infrastructure #1826

Conversation

TheRealPiotrP commented Oct 31, 2017

Test Infrastructure Changes

Test Assets

launch.json updates

package.json updates

Product Updates

dnfclas commented Oct 31, 2017

TheRealPiotrP commented Oct 31, 2017

DustinCampbell commented Oct 31, 2017

DustinCampbell commented Oct 31, 2017

rchande left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheRealPiotrP commented Oct 31, 2017

WardenGnaw commented Oct 31, 2017 • edited Loading

ghost commented Oct 31, 2017

TheRealPiotrP commented Oct 31, 2017

TheRealPiotrP commented Nov 1, 2017

DustinCampbell commented Nov 1, 2017

DustinCampbell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DustinCampbell left a comment

Choose a reason for hiding this comment

WardenGnaw commented Oct 31, 2017 •

edited

Loading