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

Add .net tool v2 document design #704

Closed

Conversation

MarcoRossignoli
Copy link
Collaborator

@MarcoRossignoli MarcoRossignoli commented Jan 25, 2020

Follow up discussion #683

cc: @ViktorHofer because coverlet .net tool is used on dotnet runtime repo. Feel free to cc others if needed and this is a breaking change.

I'm not a native speaker/writer so feel free to correct my typos, thank's to all.

@MarcoRossignoli MarcoRossignoli added documentation driver-console Issue related to dotnet net tool driver labels Jan 25, 2020
@MarcoRossignoli MarcoRossignoli added the breaking-change Issue or PR that represents a breaking change in features or functional. label Jan 25, 2020
Copy link
Collaborator

@petli petli left a comment

Choose a reason for hiding this comment

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

Sorry for the delay on this, but here are finally some thoughts on the tool design. Looks good!

The content of this report is releated to internal structure and was not born to be "portable" or "stable" over time.

```
coverlet mergereports --coverletreport .\**\coverlet.json --targetdir .\Report\coverlet.xml --format cobertura
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's better that the filenames are command line arguments, rather than having to specify the --coverletreport flag. Also rely on bash filename expansion primarily, but perhaps have support for ** globbing in the tool.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it's better that the filenames are command line arguments,

Can you explain better, it's not clear to me....can you type a command sample?

Copy link
Collaborator

Choose a reason for hiding this comment

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

coverlet mergereports --targetdir .\Report\coverlet.xml --format cobertura .\**\coverlet.json previously-merged.json ...

Validate could support in future different type of validations, for now we support threshold.

```
coverlet validate --type threshold --... --Fail or --Warn
Copy link
Collaborator

Choose a reason for hiding this comment

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

This need to specify which coverlet JSON files to process, which could be command line arguments in the same way as for mergereports. If multiple files are listed (or globbed) they should be merged before calculating the validation (but without the merge result being saved) to avoid having to first run a merge-to-coverlet followed by validate.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yep you're right good catch...we could merge in memory and do the check

This will support current instrumentation workflow

```
coverlet run /path/to/test-assembly.dll --target "dotnet" --targetargs "test /path/to/test-project --no-build"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggestion: also add --coverlet-batch ID and save the ID in the JSON coverage file. Then the same --coverlet-batch ID can be provided to mergereports and validate to select which files to include, if the result directory isn't cleared between runs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My ideas was not "touch" old feature, to avoid new behaviour for current user...I mean we could add new feature to current .net tool, but it wasn't a goal of this tool.
Again...this run tool today will output a formant different than our json...so where we put that id?

Copy link
Collaborator

Choose a reason for hiding this comment

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

My comment was assuming the output here is coverlet json. Having this flag, which only would work if also selecting that format, would allow people to build a script that runs a series of specific test projects and then merges/validates the results based on the batch ID. (Though, in such a script, it's not too difficult to clean out the result dir or work in a temporary dir to ensure that only current report files are included)

```
coverlet test solution.sln --format cobertura --threshold 80 --threshold-type line --output .\Report
```
At the moment it's not super clear in my mind how to find and merge report files, especially if user specify `--results-directory`. Also because a user can specify also every other `dotnet test` driver switch.
Copy link
Collaborator

Choose a reason for hiding this comment

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

A batch ID can be applied to each individual run (see --coverlet-batch suggestion above) and use that to find out which files were produced in a specific run.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also in this case the resulting file is not a coverlet format...I mean no always depends on --format parameter

Copy link
Collaborator Author

@MarcoRossignoli MarcoRossignoli Mar 7, 2020

Choose a reason for hiding this comment

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

use that to find out which files were produced in a specific run.

This is interesting...in general your comment concern about repetitive runs, we could add a pair of more flat to commands(here and also where we take files and produce new one) like --cleansource --appendtimestamp yyyMMddhhmmss[default] --cleantarget to cleanup source target and add a timestamp to final generated file, for easy re-runs

What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm assuming this would translate into running each individual test project, producing coverlet json files, and then merging them in the end once they all have run. Then having a batch ID in the coverlet files (not the merged file in cobertura or whatever format) would make it easy for this command to pick up the files produced by running the tests in the SLN, without the user having to think about this possibly picking up results from older runs.

@tonerdo
Copy link
Collaborator

tonerdo commented Mar 5, 2020

Late to the party! Looking this over

@MarcoRossignoli
Copy link
Collaborator Author

MarcoRossignoli commented Mar 6, 2020

@tonerdo @petli I'm a bit blocked at the moment on improvements because I "need"(or we agree on "go on with your idea and in case well'fix because otherwise product improvements slow down" but this is a @tonerdo product so he is the boss here 😄 ) some double check on my ideas to be sure that we've the same goal in mind.
Next version will have only fixes.
I know that this is OSS and we spent on it extra time when we have it.

@MarcoRossignoli
Copy link
Collaborator Author

After some discussion with @tonerdo we prefer for now work on current drivers and allow merge and other functionality without external tool to preserve a good usability.

@MarcoRossignoli MarcoRossignoli deleted the designdoc branch May 3, 2020 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Issue or PR that represents a breaking change in features or functional. documentation driver-console Issue related to dotnet net tool driver
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants