parroty / excoveralls

Coverage report tool for Elixir with coveralls.io integration.
MIT License
820 stars 178 forks source link

Minimum threshold support for json output #286

Closed RomanKotov closed 1 year ago

RomanKotov commented 2 years ago

Hi,

This pull request allows mix coveralls.json task return non-zero code, when the coverage is below minimum threshold.

I use this task in my CI pipeline. Now it passes, though coverage is below minimum threshold.

I have also noticed, that running tests via mix test printed some data to stdout. I have captured the output in that test.

parroty commented 2 years ago

Thank you for the PR and sorry being late to respond. :bow:. How do you think about separating out typo and test fix part? I don't have specific concerns and I think I can merge the change.

Regarding the mix coveralls.json part, at least it would be a breaking change, so I am wondering wether it requires additional flag, etc..). Currently, only some tasks enforce this minimum threshold.

Also, it's not the best option..., but running mix coveralls could trigger expected error?

RomanKotov commented 2 years ago

Thank you for your response. I have created a separate PR for typo and test part: https://github.com/parroty/excoveralls/pull/287 .

I have thought that every task respects the coveralls.json file. I run mix coveralls in pre-commit hook locally and it fails on low coverage. It was unexpected for me to see, that CI did not fail for the same coverage with the same settings. I thought it is a bug, so created this PR to fix this behaviour.

Thank you for pointing to the documentation. I see, that only mix coveralls and mix coveralls.html respect minimum_coverage option. I did not read it carefully, sorry.

You are right, adding this feature to mix coveralls.json will be a breaking change.

Regarding running mix coveralls in CI. Yes, this will trigger the expected error. I need a json output to upload it to Codecov, to show a coverage stats and badge. Only mix coveralls.json produces this kind of output. Both commands run a test suite. So this approach will require me to run a test suite twice:

Thank you for your library! I am quite happy with how it works now, though was confused about inconsistent behaviour between tasks. Possibly it would be helpful to state explicitly, that other tasks do not support it.

Should I close this PR?

parroty commented 1 year ago

Sorry being late again 🙇 . Thank you for the PR and detailed descriptions. Can we close this part at the moment? (though if there's more requests are coming up, it may be good to revisit this item)

RomanKotov commented 1 year ago

Yes, sure.