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

Minimum threshold support for json output #286

Closed
wants to merge 3 commits into from

Conversation

RomanKotov
Copy link
Contributor

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
Copy link
Owner

parroty commented Aug 28, 2022

Thank you for the PR and sorry being late to respond. 🙇.
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
Copy link
Contributor Author

RomanKotov commented Aug 28, 2022

Thank you for your response.
I have created a separate PR for typo and test part: #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:

  • one for check the expected coverage threshold.
  • second one to generate a json output.
    It is not an option for me.

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
Copy link
Owner

parroty commented Sep 10, 2022

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 RomanKotov closed this Sep 10, 2022
@RomanKotov
Copy link
Contributor Author

Yes, sure.

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.

None yet

2 participants