Skip to content

Conversation

@DustinCampbell
Copy link
Member

Fixes #1594

This change looks large, but it's mostly mechanical refactoring to thread the TelemetryReporter through various OmniSharp features. In addition, I took the opportunity to introduce a TestManager class that simply encapsulates all of the functionality in dotnetTest.

Copy link
Contributor

@gregg-miskelly gregg-miskelly left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM

'dotnet.test.debug',
(testMethod, fileName, testFrameworkName) => this._debugDotnetTest(testMethod, fileName, testFrameworkName));

this._telemetryIntervalId = setInterval(() =>
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should defer this until we first run a test so we don't have something polling for no reason?

Copy link
Member Author

Choose a reason for hiding this comment

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

Potentially, though I don't think it's huge deal.

I think what I really want to do here is have a shared component that sends telemetry like this. This is really similar to the telemetry reporting in the OmniSharpServer class, and I'd like to unify them. Are you OK if I leave this for now and unify later? I've filed #1693 to track unifying these in either case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Going to go ahead and merge.

@DustinCampbell DustinCampbell merged commit 2adadbd into dotnet:master Aug 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants