-
Notifications
You must be signed in to change notification settings - Fork 382
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
[Collectors]Output multiple formats #533
[Collectors]Output multiple formats #533
Conversation
cc: @vagisha-nidhi for help on review. |
src/coverlet.collector/DataCollection/CoverletCoverageCollector.cs
Outdated
Show resolved
Hide resolved
src/coverlet.collector/DataCollection/CoverletCoverageCollector.cs
Outdated
Show resolved
Hide resolved
src/coverlet.collector/DataCollection/CoverletSettingsParser.cs
Outdated
Show resolved
Hide resolved
src/coverlet.collector/DataCollection/CoverletSettingsParser.cs
Outdated
Show resolved
Hide resolved
test/coverlet.collector.tests/CoverletCoverageDataCollectorTests.cs
Outdated
Show resolved
Hide resolved
@daveMueller can you also do some real test and share the output, we need also some integration testing, at the moment manual because we don't have in place a way to simulate all 3 drivers(msbuild, collector, console) and verify conditions, we should implement one day to keep also in sync the features between drivers cc: @tonerdo @petli To use local build follow this guide https://github.com/tonerdo/coverlet/blob/master/Documentation/Troubleshooting.md#use-local-build let me know if you're in throuble. |
Oh one more thing...you should also update documentation adding a simple comma/format to be clear that is supported https://github.com/tonerdo/coverlet/blob/master/Documentation/VSTestIntegration.md#advanced-options-supported-via-runsettings but not fundamental, thank's for your effort here. |
Sorry somehow I can't get my local build running. After the test execution nothing else happens. Any idea how I could troubleshoot this? |
Yes I will update the documentation after the code is 'clean'. |
@MarcoRossignoli I can't figure out why the linux (release) build failed. Can you help out here? |
@daveMueller I'll take a look in the afternoon or tomorrow! |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
Seem a tmp issue on azp |
@daveMueller my fault we need to take another approach to debug/run local collectors code I've updated the guide, let me know if you're able to test! |
src/coverlet.collector/DataCollection/CoverletCoverageCollector.cs
Outdated
Show resolved
Hide resolved
Update fork
# Conflicts: # test/coverlet.collector.tests/CoverletCoverageDataCollectorTests.cs
Now I've also update the documentation. |
It seems there is still something going wrong. I made some tests on windows and linux. First everything seemed to work fine. Then I tried a 3th format on linux and suddenly the |
@daveMueller I added some code to try to solve Attachment issue...can you retry on linux? |
I'd like a review by some of vstest team if possible |
@@ -19,31 +20,31 @@ internal class AttachmentManager : IDisposable | |||
private readonly DataCollectionContext _dataCollectionContext; | |||
private readonly IFileHelper _fileHelper; | |||
private readonly IDirectoryHelper _directoryHelper; | |||
private readonly string _reportFileName; | |||
private readonly ICountDownEvent _countDownEvent; |
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.
Just curious. Why is this being used? Waiting for all reports to get generated?
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.
Because with multifile we get error on directory cleanup on Linux also if we create one AttachmentManager per file, like tmp directory returned was the same. Now we use only one AttachmentManager and wait on dispose for all sends(max wait 30 sec to avoid hang) this should resolve your old issue with sync between async send and dispose, what do you think?
@MarcoRossignoli @daveMueller This looks like a great change! |
Co-Authored-By: Marco Rossignoli <marco.rossignoli@gmail.com>
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.
LGTM thank's a lot @daveMueller for the great effort here!
I did some test on ubuntu and win and seem to work well!
closes #490