Add command to create an issue from within the extension #2503

akshita31 · 2018-09-07T23:39:05Z

Fixes: #2498

To do:

Try to make the component unit testable using the vscode adapter and mocking the functions

Here is a preview of all the fields added:

Issue Description

Steps to Reproduce

Expected Behaviour

Actual Behaviour

Logs

OmniSharp log

Post the output from Output-->OmniSharp log here

C# log

Post the output from Output-->C# here

Environment information

VSCode version: 1.27.1
C# Extension: 1.16.0

Mono Information

A valid mono installation could not be found. Using the built-in mono

Dotnet Info

.NET Command Line Tools (2.1.200)

Product Information:
Version: 2.1.200
Commit SHA-1 hash: 2edba8d7f1

Runtime Environment:
OS Name: ubuntu
OS Version: 18.04
OS Platform: Linux
RID: ubuntu.18.04-x64
Base Path: /usr/share/dotnet/sdk/2.1.200/

Microsoft .NET Core Shared Framework Host

Version : 2.0.7
Build : 2d61d0b043915bc948ebf98836fefe9ba942be11

Visual Studio Code Extensions

Extension	Author	Version
cpptools	ms-vscode	0.18.1
csharp	ms-vscode	1.16.0

codecov · 2018-09-07T23:47:23Z

Codecov Report

❗ No coverage uploaded for pull request base (master@a813b0d). Click here to learn what that means.
The diff coverage is 88.23%.

@@            Coverage Diff            @@
##             master    #2503   +/-   ##
=========================================
  Coverage          ?   64.74%           
=========================================
  Files             ?       93           
  Lines             ?     4173           
  Branches          ?      598           
=========================================
  Hits              ?     2702           
  Misses            ?     1298           
  Partials          ?      173

Flag	Coverage Δ
#integration	`53.28% <21.56%> (?)`
#unit	`84.31% <77.77%> (?)`

Impacted Files	Coverage Δ
src/omnisharp/loggingEvents.ts	`98.97% <ø> (ø)`
src/platform.ts	`76.13% <100%> (ø)`
src/features/commands.ts	`35.48% <100%> (ø)`
src/main.ts	`93.2% <100%> (ø)`
src/features/utils/openUrlWithBody.ts	`100% <100%> (ø)`
src/constants/CSharpExtensionId.ts	`100% <100%> (ø)`
src/features/reportIssue.ts	`83.78% <83.78%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a813b0d...b7557d7. Read the comment docs.

rchande · 2018-09-10T20:51:12Z

package.json

@@ -643,6 +643,11 @@
        "command": "csharp.listRemoteProcess",
        "title": "List processes on remote connection for attach",
        "category": "CSharp"
+      },
+      {
+        "command": "csharp.fileBugReport",


"reportIssue"

rchande · 2018-09-10T20:52:40Z

src/features/generateBugReport.ts

+}
+
+async function getMonoVersion(): Promise<string>{
+    return execChildProcess("dotnet --info", process.cwd());


"mono --version"?

What happens if mono isn't on the path (eg, they depend on the one from the run script)

akshita31 · 2018-09-11T02:33:53Z

To do:

Add the test for the issue reporter observer

rchande · 2018-09-11T22:57:34Z

package.json

+      },
+      {
+        "command": "csharp.reportIssue",
+        "title": "Upload bug report to github",


Upload implies that there's no work to done. Maybe phrase it like "Start authoring a new issue on GitHub" or something?

rchande · 2018-09-11T22:58:07Z

src/features/fileIssue.ts

+
+const issuesUrl = "https://github.com/OmniSharp/omnisharp-vscode/issues/new";
+
+export default async function fileIssue(vscode: vscode, eventStream: EventStream, execChildProcess: (command: string, workingDirectory?: string) => Promise<string>, isValidPlatformForMono: boolean) {


reportIssue

rchande · 2018-09-11T23:03:20Z

src/features/fileIssue.ts

+            monoInfo = await getMonoVersion(execChildProcess);
+        }
+        catch (error) {
+            monoInfo = "A valid mono installation could not be found. Using the built-in mono"


The extension has logic that determines the version of mono found and only using it if it's new enough. Can we try to follow the same logic here? We don't want to report mono versions that are old enough that we use the built in one instead.

rchande · 2018-09-11T23:03:52Z

src/features/fileIssue.ts

+}
+
+async function getDotnetInfo(execChildProcess: (command: string, workingDirectory?: string) => Promise<string>): Promise<string> {
+    return execChildProcess("dotnet --info", process.cwd());


I think this also needs to handle no dotnet on path.

rchande · 2018-09-11T23:04:41Z

src/constants/CSharpExtensionId.ts

+ *  Licensed under the MIT License. See License.txt in the project root for license information.
+ *--------------------------------------------------------------------------------------------*/
+
+export const CSharpExtensionId = 'ms-vscode.csharp';


If this appears in package.json, can we read it from there instead of redefining it here?

The package json has a publisher name and the name of the extension combining which gives the extension id, but to get access to the package.json we need the extension first and to get the extension we need the extension id. Hence from what I understand there is no way to get the extension id directly from the package.json.

rchande · 2018-09-11T23:06:31Z

src/observers/ReportIssueObserver.ts

+    public post = (event: BaseEvent) => {
+        switch (event.constructor.name) {
+            case ReportIssue.name:
+                let issue = <ReportIssue>event;


I'm not sure how I feel about needing to dispatch to the message bus to do this...

What I mean is that there could be a component that's basically identical to the observer youa dded, but they could be directly composed together as opposed to going over the message bus.

rchande · 2018-09-11T23:07:39Z

test/unitTests/features/fileIssue.test.ts

+import { ReportIssue } from "../../../src/omnisharp/loggingEvents";
+import { vscode } from "../../../src/vscodeAdapter";
+
+suite("File Issue", () => {


Make names consistent, whatever we land on :)

src/omnisharp/loggingEvents.ts

rchande · 2018-09-17T16:39:27Z

src/omnisharp/OmniSharpMonoResolver.ts

+    }
+}
+
+function configureCustomMono(childEnv: NodeJS.ProcessEnv, options: Options): string {


Nit: can you move this underneath the getGlobalMono method?

rchande · 2018-09-17T16:43:05Z

src/omnisharp/OmniSharpMonoResolver.ts

+    return undefined;
+}
+
+export async function getMonoVersion( environment: NodeJS.ProcessEnv): Promise<string> {


How is this one different from the one that's passed into the constructor?

This is a separate function that is used to initialise the OmniSharpMonoResolver class.

rchande · 2018-09-17T16:43:16Z

src/omnisharp/OmniSharpMonoResolver.ts

+    return new Promise<string>((resolve, reject) => {
+        let childprocess: ChildProcess;
+        try {
+            childprocess = spawn('mono', ['--version'], { env: environment });


Seems like this makes it untestable, no?

rchande · 2018-09-17T16:52:27Z

src/omnisharp/constants/IMonoResolver.ts

+import { MonoInformation } from "./MonoInformation";
+
+export interface IMonoResolver {
+    shouldUseGlobalMono(options: Options): Promise<MonoInformation>;


"should" in the name makes me think this returns a boolean. Maybe call this "getMonoInfo" or somethign?

…p-vscode into bug_generator

akshita31 added this to the 1.17 milestone Sep 7, 2018

akshita31 requested review from DustinCampbell, rchande, colombod and TheRealPiotrP and removed request for rchande September 8, 2018 00:37

rchande reviewed Sep 10, 2018

View reviewed changes

akshita31 added 8 commits September 10, 2018 19:31

Add command to create an issue

7b1c30d

use child process promise

599794b

Add

afd08ba

add mono information

6f563df

Use existing child-process

42fa329

refactoring for testing

7876468

rename

c2c2e7e

Add the unit test

63e1746

akshita31 force-pushed the bug_generator branch from 25f335f to 63e1746 Compare September 11, 2018 02:31

Remove package-lock

44d4085

add tests for the observer

e548da2

akshita31 force-pushed the bug_generator branch from dc4acfa to e548da2 Compare September 11, 2018 19:10

Merge remote-tracking branch 'upstream/master' into bug_generator

7b2ee67

akshita31 force-pushed the bug_generator branch from 6fc1171 to 7b2ee67 Compare September 11, 2018 19:49

akshita31 added 3 commits September 11, 2018 13:17

add encoding and some structure change

440b0e3

make bold

543ad48

add encoding and some structure change

3a7ac64

akshita31 force-pushed the bug_generator branch from e826370 to 3a7ac64 Compare September 11, 2018 22:39

rchande suggested changes Sep 11, 2018

View reviewed changes

Add the changes

aed6f9a

akshita31 force-pushed the bug_generator branch from 28c9e76 to 265470c Compare September 12, 2018 00:15

akshita31 added 4 commits September 12, 2018 12:43

using the openurl observer

8980a2c

Modify the tests for the observer

d47164a

Rename to report issue

c15eb7d

PR feedback

379654c

akshita31 force-pushed the bug_generator branch from b7557d7 to 379654c Compare September 12, 2018 20:06

akshita31 added 14 commits September 13, 2018 16:17

Add refactoring for the mono options

6537a8d

Some more refactoring

293c97b

Add the global mono information

3835cfb

add the mono resolver to the issue creator

1a2dcdc

small changes

834f8d1

should throw error when not valid

5c5304e

Do not use mono information interface

cb72d3f

using the mono resolver interface

5cd3196

rename

cdb2345

pass the GetMonoVersion

2eee6b7

Add the environment to the information

baed655

add tests for the mono resolver

053f326

remove file

f29f892

Merge branch 'master' into bug_generator

2d963c2

rchande suggested changes Sep 17, 2018

View reviewed changes

akshita31 added 5 commits September 17, 2018 11:50

Rename and add interfaces

8e55eb4

using more interfaces

0c543b8

refactor the test

dfc383f

soe more changes

30b3995

Merge branch 'bug_generator' of https://github.com/akshita31/omnishar…

61a2efb

…p-vscode into bug_generator

rchande approved these changes Sep 18, 2018

View reviewed changes

akshita31 merged commit 9f37c43 into dotnet:master Sep 19, 2018

akshita31 deleted the bug_generator branch September 19, 2018 18:54

bschnurr mentioned this pull request Dec 24, 2020

Add a command that fills out the issue template microsoft/pylance-release#762

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add command to create an issue from within the extension #2503

Add command to create an issue from within the extension #2503

akshita31 commented Sep 7, 2018 •

edited

Loading

codecov bot commented Sep 7, 2018 •

edited

Loading

rchande Sep 10, 2018

rchande Sep 10, 2018

rchande Sep 10, 2018

akshita31 commented Sep 11, 2018 •

edited

Loading

rchande Sep 11, 2018

akshita31 Sep 11, 2018

rchande Sep 11, 2018

rchande Sep 11, 2018

rchande Sep 11, 2018

akshita31 Sep 11, 2018

rchande Sep 11, 2018

akshita31 Sep 11, 2018

rchande Sep 11, 2018

rchande Sep 11, 2018

rchande Sep 11, 2018

rchande Sep 17, 2018

rchande Sep 17, 2018

akshita31 Sep 17, 2018

rchande Sep 17, 2018

rchande Sep 17, 2018


		const issuesUrl = "https://github.com/OmniSharp/omnisharp-vscode/issues/new";

		export default async function fileIssue(vscode: vscode, eventStream: EventStream, execChildProcess: (command: string, workingDirectory?: string) => Promise<string>, isValidPlatformForMono: boolean) {

Add command to create an issue from within the extension #2503

Add command to create an issue from within the extension #2503

Conversation

akshita31 commented Sep 7, 2018 • edited Loading

Issue Description

Steps to Reproduce

Expected Behaviour

Actual Behaviour

Logs

OmniSharp log

C# log

Environment information

codecov bot commented Sep 7, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akshita31 commented Sep 11, 2018 • edited Loading

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

akshita31 commented Sep 7, 2018 •

edited

Loading

codecov bot commented Sep 7, 2018 •

edited

Loading

akshita31 commented Sep 11, 2018 •

edited

Loading