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

fix: Allow MergeCoverageFormat to default to null #86

Merged
merged 2 commits into from
Oct 8, 2023

Conversation

ImmortalRat
Copy link

Without the fix the tool is always trying to merge coverage results.

I fixed the issue and also added unit tests to confirm that this setting works as expected in all supported cases.

I only manually tested cases without coverage format specified and with Coverage format, but without actually producing any coverage data - I only confirmed that the resulting command arguments for coverage merge are the same.

@ricardofslp ricardofslp self-requested a review October 6, 2023 21:11
@ImmortalRat
Copy link
Author

@ricardofslp let me know if there is a better way to communicate if I have some fixes/improvements in mind that I feel comfortable making myself.

I was looking into producing a merged TRX file where failed test pass is overridden by the successful. Otherwise dorny/test-reporter Action shows error even though there was a successful second pass.

Should I submit the feature request first ? Propose design first and have a discussion? Or just make a sudden random PR ? What's the preference ?

@ricardofslp
Copy link
Collaborator

@ricardofslp let me know if there is a better way to communicate if I have some fixes/improvements in mind that I feel comfortable making myself.

I was looking into producing a merged TRX file where failed test pass is overridden by the successful. Otherwise dorny/test-reporter Action shows error even though there was a successful second pass.

Should I submit the feature request first ? Propose design first and have a discussion? Or just make a sudden random PR ? What's the preference ?

Hi @ImmortalRat,

If you don't mind we would prefer to have an issue/feature request opened along with the PR because it would make any possible discussion on the request easier.
Regarding that requirement you've described we've also had the same need but, because we didn't want to add this to the scope of this tool, we've created another to achieve this.

Please take a look:
https://www.nuget.org/packages/dotnet-trx-merge
https://github.com/ricardofslp/dotnet-trx-merge

@ImmortalRat
Copy link
Author

ImmortalRat commented Oct 8, 2023 via email

@ricardofslp
Copy link
Collaborator

Thanks for the pointers! I did not actually find that one when I was looking for a merger. Another one that everyone is referencing is old and does not support overrides. Next time I will create an issue/feature request first.

On Fri, Oct 6, 2023, 4:11 PM Ricardo Pereira @.> wrote: @ricardofslp https://github.com/ricardofslp let me know if there is a better way to communicate if I have some fixes/improvements in mind that I feel comfortable making myself. I was looking into producing a merged TRX file where failed test pass is overridden by the successful. Otherwise dorny/test-reporter Action shows error even though there was a successful second pass. Should I submit the feature request first ? Propose design first and have a discussion? Or just make a sudden random PR ? What's the preference ? Hi @ImmortalRat https://github.com/ImmortalRat, If you don't mind we would prefer to have an issue/feature request opened along with the PR because it would make any possible discussion on the request easier. Regarding that requirement you've described we've also had the same need but, because we didn't want to add this to the scope of this tool, we've created another to achieve this. Please take a look: https://www.nuget.org/packages/dotnet-trx-merge https://github.com/ricardofslp/dotnet-trx-merge — Reply to this email directly, view it on GitHub <#86 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AFP7XG4I6LUSYVJUMOMFUDDX6CF3RAVCNFSM6AAAAAA5WKXYXCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONJRGQ4DSNZRG4 . You are receiving this because you were mentioned.Message ID: @.>

We were trying to find one but had the same issue and that's why we've worked on this one. It is still quite new so probably not easy to find but please help share it because I see there's a need for that also.

@ricardofslp ricardofslp merged commit 6bfda1d into joaoopereira:main Oct 8, 2023
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.

4 participants