-
Notifications
You must be signed in to change notification settings - Fork 389
Add .net tool v2 document design #704
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
Conversation
There was a problem hiding this 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Late to the party! Looking this over |
@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. |
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. |
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.