Skip to content
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

Adding documetation for VSTest Integration #402

Merged

Conversation

vagisha-nidhi
Copy link
Contributor

@vagisha-nidhi vagisha-nidhi commented May 7, 2019

Adding documetation for VSTest Integration. Laying out the details for the user experience and implementation.

contributes to #395

@codecov
Copy link

codecov bot commented May 7, 2019

Codecov Report

Merging #402 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #402   +/-   ##
=======================================
  Coverage   86.96%   86.96%           
=======================================
  Files          16       16           
  Lines        2178     2178           
=======================================
  Hits         1894     1894           
  Misses        284      284

#### Default
| Option | Summary |
|-------------|------------------------------------|
|Format | Results format in which coverage output is generated. Default format is cobertura.|
Copy link
Collaborator

@MarcoRossignoli MarcoRossignoli May 7, 2019

Choose a reason for hiding this comment

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

Should we provide default CoverletOutput?
With msbuild we are using MSBuildProjectDirectory https://github.com/tonerdo/coverlet/blob/0956d1e8da6b7265b0b5e4f93af3499c4be99cc8/src/coverlet.msbuild.tasks/coverlet.msbuild.props#L14 but there is some discussion in place #152

I'm in agreement with @eerhardt comment I prefer put it in output directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We will only be getting the coverage output from coverlet.core APIs and the responsibility of sending the file will be with the datacollector.
By default, currently the files are put into the TestResults directory, Where this TestResults directory will be created is configurable via runsettings.


| Entry point | How will code coverage be enabled? | Syntax |
|-------------|------------------------------------|----------------------------------------------------------------------|
|dotnet test CLI | Through a switch to condition data collection | `dotnet test --collect:"XPlat Code Coverage"` |
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this real final name or only a placeholder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

XPlat Code Coverage is the final name at present.

<RunSettings>
<DataCollectionRunSettings>
<DataCollectors>
<DataCollector friendlyName="XPlat code coverage">
Copy link
Collaborator

Choose a reason for hiding this comment

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

Tech question, test runner search for a class with DataCollectorFriendlyName attribute named XPlat code coverage from output build folder, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It searches for the class in various folders including the output build folder and the nuget folders.
Basically we'll call a target before VSTest that would add the nuget path of the datacollectors to the search directories(it will be picked up directly from the nuget folder)



#### Scope of Enhancement
Currently, advanced options are supported via runsettings. Providing support through additional command line arguments in vstest can be taken up separately.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Providing support through additional command line arguments in vstest can be taken up separately.

Will be better user experience...but I think that it's require non trivial internal changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. This will include some internal vstest changes which we can pick up as an enhancement.

## Implementation Details
The proposed solution is implemented with the help of [datacollectors](https://github.com/Microsoft/vstest-docs/blob/master/docs/extensions/datacollector.md).
1. Outproc Datacollector : The outproc collector would always run in a separate process(datacollector.exe/datacollector.dll) than the process in which tests are being executed(testhost*.exe/testhost.dll). This datacollector would be responsible for calling into coverlet APIs for instrumenting dlls, collecting coverage results and sending the coverage output file back to test platform.
2. Inproc Datacollector : The inproc collector in the testhost process executing the tests. This collector will be needed to remove the dependency on the exit handler to flush the hit files.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see config to plug inproc datacollector...am I missing something?

Copy link
Contributor Author

@vagisha-nidhi vagisha-nidhi May 7, 2019

Choose a reason for hiding this comment

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

The inproc datacollector will be automatically plugged in with --collect:"XPlat Code Coverage" . The inproc datacollector will lie in the same nuget and get picked from there.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah ok thank's now it's more clear in/out behaviours of my past tests.

Copy link
Collaborator

@MarcoRossignoli MarcoRossignoli left a comment

Choose a reason for hiding this comment

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

Workflow is enough clear to me at this point.
Wait for @tonerdo thoughts/question.
I could try to implement collectors with MS guys supervision if we agree on it.

@vagisha-nidhi
Copy link
Contributor Author

Workflow is enough clear to me at this point.
Wait for @tonerdo thoughts/question.
I could try to implement collectors with MS guys supervision if we agree on it.

@MarcoRossignoli Sure.
We are willing to contribute to your repo these datacollectors, We will be happy to do so!

@MarcoRossignoli
Copy link
Collaborator

We are willing to contribute to your repo these datacollectors, We will be happy to do so!

No problem to me!

Copy link
Collaborator

@tonerdo tonerdo left a comment

Choose a reason for hiding this comment

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

documentation/VSTestIntegration.md Outdated Show resolved Hide resolved
@MarcoRossignoli
Copy link
Collaborator

@tonerdo before go on with this and let @vagisha-nidhi start to work on collector we should try to merge #393 so we'll use new contracts.

@MarcoRossignoli
Copy link
Collaborator

@vagisha-nidhi can you capitalize folder name Documentation to better adhere to foundation repo guidelines https://github.com/dotnet/foundation/blob/master/guidance/repo-guide.md

@vagisha-nidhi
Copy link
Contributor Author

@MarcoRossignoli I have addressed the comments here.
Since #393 have contract changes in coverlet.core, it'll be good to prioritize it.
#392 is also related to these datacollector changes. Please look into this as well.

@vagisha-nidhi
Copy link
Contributor Author

@MarcoRossignoli @tonerdo It'll be really convenient for me if you could provide an ETA on #393?

@MarcoRossignoli
Copy link
Collaborator

Got some review from Toni #393 (comment), I'll do my best to replace PR with simpler new one in a pair of days...I won't be available between Thursday-Sunday for a fast holiday.

@MarcoRossignoli
Copy link
Collaborator

@tonerdo PTAL #409

@tonerdo
Copy link
Collaborator

tonerdo commented May 13, 2019

@vagisha-nidhi #409 has been merged in

@MarcoRossignoli
Copy link
Collaborator

@tonerdo we can merge this...if during developments something change we'll update

@vagisha-nidhi
Copy link
Contributor Author

@tonerdo This can be merged, right?

@tonerdo tonerdo merged commit 6524812 into coverlet-coverage:master May 21, 2019
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